Bug 1330184 - Allow notifying observers for profiler state changes on background threads. r=njn draft
authorMarkus Stange <mstange@themasta.com>
Tue, 30 May 2017 17:07:56 -0400
changeset 586673 b04504b06aa80635a50d7ce1dcec43a994d6ba8d
parent 586049 34ac1a5d6576d6775491c8a882710a1520551da6
child 586674 8c5d2b4ccacbbec92711a9561d4166e1e7c376c7
push id61485
push userbmo:mstange@themasta.com
push dateTue, 30 May 2017 22:11:39 +0000
reviewersnjn
bugs1330184
milestone55.0a1
Bug 1330184 - Allow notifying observers for profiler state changes on background threads. r=njn MozReview-Commit-ID: GlkVwGTa2b4
tools/profiler/core/platform.cpp
--- a/tools/profiler/core/platform.cpp
+++ b/tools/profiler/core/platform.cpp
@@ -526,29 +526,16 @@ MOZ_THREAD_LOCAL(ThreadInfo*) TLSInfo::s
 // This second pointer isn't ideal, but does provide a way to satisfy those
 // constraints. TLSInfo manages it, except for the uses in
 // profiler_call_{enter,exit}.
 MOZ_THREAD_LOCAL(PseudoStack*) sPseudoStack;
 
 // The name of the main thread.
 static const char* const kMainThreadName = "GeckoMain";
 
-static bool
-CanNotifyObservers()
-{
-  MOZ_RELEASE_ASSERT(NS_IsMainThread());
-
-#if defined(GP_OS_android)
-  // Android ANR reporter uses the profiler off the main thread.
-  return NS_IsMainThread();
-#else
-  return true;
-#endif
-}
-
 ////////////////////////////////////////////////////////////////////////
 // BEGIN tick/unwinding code
 
 // TickSample contains all the information needed by Tick(). Some of it is
 // pointers to long-lived things, and some of it is sampled just before the
 // call to Tick().
 class TickSample {
 public:
@@ -1986,52 +1973,51 @@ locked_register_thread(PSLockRef aLock, 
       info->PollJSSampling();
     }
   }
 
   CorePS::LiveThreads(aLock).push_back(info);
 }
 
 static void
+NotifyObservers(const char* aTopic, nsISupports* aSubject = nullptr)
+{
+  if (!NS_IsMainThread()) {
+    // Dispatch a task to the main thread that notifies observers.
+    // If NotifyObservers is called both on and off the main thread within a
+    // short time, the order of the notifications can be different from the
+    // order of the calls to NotifyObservers.
+    // Getting the order 100% right isn't that important at the moment, because
+    // these notifications are only observed in the parent process, where the
+    // profiler_* functions are currently only called on the main thread.
+    nsCOMPtr<nsISupports> subject = aSubject;
+    NS_DispatchToMainThread(NS_NewRunnableFunction([=] {
+      NotifyObservers(aTopic, subject);
+    }));
+    return;
+  }
+
+  if (nsCOMPtr<nsIObserverService> os = services::GetObserverService()) {
+    os->NotifyObservers(aSubject, aTopic, nullptr);
+  }
+}
+
+static void
 NotifyProfilerStarted(const int aEntries, double aInterval, uint32_t aFeatures,
                       const char** aFilters, uint32_t aFilterCount)
 {
-  if (!CanNotifyObservers()) {
-    return;
-  }
-
-  nsCOMPtr<nsIObserverService> os = mozilla::services::GetObserverService();
-  if (!os) {
-    return;
-  }
-
   nsTArray<nsCString> filtersArray;
   for (size_t i = 0; i < aFilterCount; ++i) {
     filtersArray.AppendElement(aFilters[i]);
   }
 
   nsCOMPtr<nsIProfilerStartParams> params =
     new nsProfilerStartParams(aEntries, aInterval, aFeatures, filtersArray);
 
-  os->NotifyObservers(params, "profiler-started", nullptr);
-}
-
-static void
-NotifyObservers(const char* aTopic)
-{
-  if (!CanNotifyObservers()) {
-    return;
-  }
-
-  nsCOMPtr<nsIObserverService> os = mozilla::services::GetObserverService();
-  if (!os) {
-    return;
-  }
-
-  os->NotifyObservers(nullptr, aTopic, nullptr);
+  NotifyObservers("profiler-started", params);
 }
 
 static void
 locked_profiler_start(PSLockRef aLock, const int aEntries, double aInterval,
                       uint32_t aFeatures,
                       const char** aFilters, uint32_t aFilterCount);
 
 void
@@ -2503,18 +2489,20 @@ profiler_stop()
     if (!ActivePS::Exists(lock)) {
       return;
     }
 
     samplerThread = locked_profiler_stop(lock);
   }
 
   // We notify observers with gPSMutex unlocked. Otherwise we might get a
-  // deadlock, if code run by the observer calls a profiler function that locks
-  // gPSMutex. (This has been seen in practise in bug 1346356.)
+  // deadlock, if code run by these functions calls a profiler function that
+  // locks gPSMutex, for example when it wants to insert a marker.
+  // (This has been seen in practise in bug 1346356, when we were still firing
+  // these notifications synchronously.)
   NotifyObservers("profiler-stopped");
 
   // We delete with gPSMutex unlocked. Otherwise we would get a deadlock: we
   // would be waiting here with gPSMutex locked for SamplerThread::Run() to
   // return so the join operation within the destructor can complete, but Run()
   // needs to lock gPSMutex to return.
   //
   // Because this call occurs with gPSMutex unlocked, it -- including the final