Bug 1261702 - Make nsPerformanceStatsService::Dispose() idempotent,r=froydnj, a=sylvestre
authorDavid Rajchenbach-Teller <dteller@mozilla.com>
Tue, 07 Jun 2016 10:45:44 +0200
changeset 339605 2e5e0318f6e5f7db9f026dc4c9b656c64722ced6
parent 339604 e50976a962c4059618f0adcef93e49acf7264215
child 339606 1c3037d99f3af964fb481169d9eb6b65a81bd081
push id6249
push userjlund@mozilla.com
push dateMon, 01 Aug 2016 13:59:36 +0000
treeherdermozilla-beta@bad9d4f5bf7e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersfroydnj, sylvestre
bugs1261702
milestone49.0a2
Bug 1261702 - Make nsPerformanceStatsService::Dispose() idempotent,r=froydnj, a=sylvestre 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.