Bug 1261702 - Make nsPerformanceStatsService::Dispose() idempotent,r=froydnj
authorDavid Rajchenbach-Teller <dteller@mozilla.com>
Tue, 07 Jun 2016 10:45:44 +0200
changeset 300898 e4c8b7d0abede9f0495669e7fa0b5ae6f5b22a0c
parent 300897 95f9da5a14febf2a7c195453532fe9b696b20baa
child 300899 7ec7b62eb5bc420a2f940f89bc59075f78ce184d
push id78123
push userdteller@mozilla.com
push dateTue, 07 Jun 2016 16:43:34 +0000
treeherdermozilla-inbound@e4c8b7d0abed [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersfroydnj
bugs1261702
milestone50.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1261702 - Make nsPerformanceStatsService::Dispose() idempotent,r=froydnj Although I haven't been able to pinpoint why, it looks like nsPerformanceStatsService::Dispose() may be called twice, which in turn causes crashes. This patch makes sure that calling the method twice is idempotent. Also, just in case this was due to a typo in AddObserver/RemoveObserver, this patch replaces the literal strings used in both with constants. MozReview-Commit-ID: 8fXO20r5xvO
toolkit/components/perfmonitoring/nsPerformanceStats.cpp
toolkit/components/perfmonitoring/nsPerformanceStats.h
--- a/toolkit/components/perfmonitoring/nsPerformanceStats.cpp
+++ b/toolkit/components/perfmonitoring/nsPerformanceStats.cpp
@@ -131,16 +131,23 @@ GenerateUniqueGroupId(const JSRuntime* r
   groupId.AssignLiteral("process: ");
   groupId.AppendInt(processId);
   groupId.AppendLiteral(", thread: ");
   groupId.AppendInt(runtimeId);
   groupId.AppendLiteral(", group: ");
   groupId.AppendInt(uid);
 }
 
+static const char* TOPICS[] = {
+  "profile-before-change",
+  "quit-application",
+  "quit-application-granted",
+  "xpcom-will-shutdown"
+};
+
 } // namespace
 
 /* ------------------------------------------------------
  *
  * class nsPerformanceObservationTarget
  *
  */
 
@@ -633,16 +640,17 @@ PendingAlertsCollector::Dispose() {
  * class nsPerformanceStatsService
  *
  */
 
 NS_IMPL_ISUPPORTS(nsPerformanceStatsService, nsIPerformanceStatsService, nsIObserver)
 
 nsPerformanceStatsService::nsPerformanceStatsService()
   : mIsAvailable(false)
+  , mDisposed(false)
 #if defined(XP_WIN)
   , mProcessId(GetCurrentProcessId())
 #else
   , mProcessId(getpid())
 #endif
   , mRuntime(xpc::GetJSRuntime())
   , mUIdCounter(0)
   , mTopGroup(nsPerformanceGroup::Make(mRuntime,
@@ -700,23 +708,28 @@ nsPerformanceStatsService::~nsPerformanc
 void
 nsPerformanceStatsService::Dispose()
 {
   // Make sure that we do not accidentally destroy `this` while we are
   // cleaning up back references.
   RefPtr<nsPerformanceStatsService> kungFuDeathGrip(this);
   mIsAvailable = false;
 
+  if (mDisposed) {
+    // Make sure that we don't double-dispose.
+    return;
+  }
+  mDisposed = true;
+
   // Disconnect from nsIObserverService.
   nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService();
   if (obs) {
-    obs->RemoveObserver(this, "profile-before-change");
-    obs->RemoveObserver(this, "quit-application");
-    obs->RemoveObserver(this, "quit-application-granted");
-    obs->RemoveObserver(this, "xpcom-will-shutdown");
+    for (size_t i = 0; i < mozilla::ArrayLength(TOPICS); ++i) {
+      mozilla::Unused << obs->RemoveObserver(this, TOPICS[i]);
+    }
   }
 
   // Clear up and disconnect from JSAPI.
   js::DisposePerformanceMonitoring(mRuntime);
 
   mozilla::Unused << js::SetStopwatchIsMonitoringCPOW(mRuntime, false);
   mozilla::Unused << js::SetStopwatchIsMonitoringJank(mRuntime, false);
 
@@ -775,20 +788,19 @@ nsPerformanceStatsService::Init()
 nsresult
 nsPerformanceStatsService::InitInternal()
 {
   // Make sure that we release everything during shutdown.
   // We are a bit defensive here, as we know that some strange behavior can break the
   // regular shutdown order.
   nsCOMPtr<nsIObserverService> obs = mozilla::services::GetObserverService();
   if (obs) {
-    obs->AddObserver(this, "profile-before-change", false);
-    obs->AddObserver(this, "quit-application-granted", false);
-    obs->AddObserver(this, "quit-application", false);
-    obs->AddObserver(this, "xpcom-will-shutdown", false);
+    for (size_t i = 0; i < mozilla::ArrayLength(TOPICS); ++i) {
+      mozilla::Unused << obs->AddObserver(this, TOPICS[i], false);
+    }
   }
 
   // Connect to JSAPI.
   if (!js::SetStopwatchStartCallback(mRuntime, StopwatchStartCallback, this)) {
     return NS_ERROR_UNEXPECTED;
   }
   if (!js::SetStopwatchCommitCallback(mRuntime, StopwatchCommitCallback, this)) {
     return NS_ERROR_UNEXPECTED;
--- a/toolkit/components/perfmonitoring/nsPerformanceStats.h
+++ b/toolkit/components/perfmonitoring/nsPerformanceStats.h
@@ -138,16 +138,21 @@ protected:
   friend nsPerformanceGroup;
 
   /**
    * `false` until `Init()` and after `Dispose()`, `true` inbetween.
    */
   bool mIsAvailable;
 
   /**
+   * `true` once we have called `Dispose()`.
+   */
+  bool mDisposed;
+
+  /**
    * A unique identifier for the process.
    *
    * Process HANDLE under Windows, pid under Unix.
    */
   const uint64_t mProcessId;
 
   /**
    * The JS Runtime for the main thread.