Bug 2054145 - Apply PollJSSampling's JS engine calls without the thread data lock held, r=profiler-reviewers,canova.

A DOM Worker clearing its JSContext ran PollJSSampling() under its
ThreadRegistration data lock, and PollJSSampling calls into the JS engine
(js::EnableContextProfilingStack -> ReleaseAllJITCode ->
CancelOffThreadIonCompile), which blocks on the JS helper threads. The
SamplerThread holds gPSMutex and blocks acquiring that same data lock, so every
thread that needs gPSMutex to register/unregister -- including the helper the
worker waits on -- is stuck. Circular wait.

Split PollJSSampling into TakeJSSamplingChange() (flips mJSSampling under the
data lock) and ApplyJSSamplingChange() (makes the js::Enable* calls). The
current-thread poll path now takes the data lock only for the state flip and
applies the JS-engine change with no lock held, mirroring the existing 'drop
the global profiler mutex before calling into the JS engine' discipline.

Differential Revision: https://phabricator.services.mozilla.com/D311644

tools/profiler/core/ProfilerThreadRegistrationData.cpp

@@ -286,9 +286,13 @@ void ThreadRegistrationLockedRWOnThread::ClearCycleCollectedJSContext() {
              !!mJsFrameBuffer);
 }
 
-void ThreadRegistrationLockedRWOnThread::PollJSSampling() {
+ThreadRegistrationLockedRWOnThread::JSSamplingChange
+ThreadRegistrationLockedRWOnThread::TakeJSSamplingChange() {
+  JSSamplingChange change;
   // We can't start/stop profiling until we have the thread's JSContext.
   if (mCCJSContext) {
+    change.mContext = mCCJSContext->Context();
+    change.mAllocationsEnabled = JSAllocationsEnabled();
     // It is possible for mJSSampling to go through the following sequences.
     //
     // - INACTIVE, ACTIVE_REQUESTED, INACTIVE_REQUESTED, INACTIVE
@@ -298,29 +302,43 @@ void ThreadRegistrationLockedRWOnThread::PollJSSampling() {
     // Therefore, the if and else branches here aren't always interleaved.
     // This is ok because the JS engine can handle that.
     //
-    JSContext* cx = mCCJSContext->Context();
     if (mJSSampling == ACTIVE_REQUESTED) {
       mJSSampling = ACTIVE;
-      js::EnableContextProfilingStack(cx, true);
-
-      if (JSAllocationsEnabled()) {
-        // TODO - This probability should not be hardcoded. See Bug 1547284.
-        JS::EnableRecordingAllocations(cx, profiler_add_js_allocation_marker,
-                                       0.01);
-      }
-      js::RegisterContextProfilerMarkers(
-          cx, profiler_add_js_marker, profiler_add_js_interval,
-          profiler_add_js_flow, profiler_add_js_terminating_flow);
-
+      change.mAction = JSSamplingChange::Action::Start;
     } else if (mJSSampling == INACTIVE_REQUESTED) {
       mJSSampling = INACTIVE;
-      js::EnableContextProfilingStack(cx, false);
-
-      if (JSAllocationsEnabled()) {
-        JS::DisableRecordingAllocations(cx);
-      }
+      change.mAction = JSSamplingChange::Action::Stop;
     }
   }
+  return change;
+}
+
+/* static */ void ThreadRegistrationLockedRWOnThread::ApplyJSSamplingChange(
+    const JSSamplingChange& aChange) {
+  JSContext* cx = aChange.mContext;
+  if (aChange.mAction == JSSamplingChange::Action::Start) {
+    js::EnableContextProfilingStack(cx, true);
+
+    if (aChange.mAllocationsEnabled) {
+      // TODO - This probability should not be hardcoded. See Bug 1547284.
+      JS::EnableRecordingAllocations(cx, profiler_add_js_allocation_marker,
+                                     0.01);
+    }
+    js::RegisterContextProfilerMarkers(
+        cx, profiler_add_js_marker, profiler_add_js_interval,
+        profiler_add_js_flow, profiler_add_js_terminating_flow);
+
+  } else if (aChange.mAction == JSSamplingChange::Action::Stop) {
+    js::EnableContextProfilingStack(cx, false);
+
+    if (aChange.mAllocationsEnabled) {
+      JS::DisableRecordingAllocations(cx);
+    }
+  }
+}
+
+void ThreadRegistrationLockedRWOnThread::PollJSSampling() {
+  ApplyJSSamplingChange(TakeJSSamplingChange());
 }
 
 #ifdef NIGHTLY_BUILD

tools/profiler/core/platform.cpp

@@ -6120,17 +6120,23 @@ void profiler_init_signal_handlers() {
 #endif
 
 static void PollJSSamplingForCurrentThread() {
-  // Don't call into the JS engine with the global profiler mutex held as this
-  // can deadlock.
+  // Don't call into the JS engine with a profiler lock held as this can
+  // deadlock: the js::Enable* calls can block waiting on the JS helper threads,
+  // which in turn need the profiler locks. Besides the global profiler mutex
+  // asserted below, that includes the thread's own data lock -- so take that
+  // lock only to flip the sampling state (TakeJSSamplingChange), then apply the
+  // JS-engine change (ApplyJSSamplingChange) with no lock held.
   MOZ_ASSERT(!PSAutoLock::IsLockedOnCurrentThread());
 
+  ThreadRegistration::LockedRWOnThread::JSSamplingChange change;
   ThreadRegistration::WithOnThreadRef(
-      [](ThreadRegistration::OnThreadRef aOnThreadRef) {
+      [&change](ThreadRegistration::OnThreadRef aOnThreadRef) {
         aOnThreadRef.WithLockedRWOnThread(
-            [](ThreadRegistration::LockedRWOnThread& aThreadData) {
-              aThreadData.PollJSSampling();
+            [&change](ThreadRegistration::LockedRWOnThread& aThreadData) {
+              change = aThreadData.TakeJSSamplingChange();
             });
       });
+  ThreadRegistration::LockedRWOnThread::ApplyJSSamplingChange(change);
 }
 
 void profiler_init(void* aStackTop) {

tools/profiler/public/ProfilerThreadRegistrationData.h

@@ -539,9 +539,27 @@ class ThreadRegistrationLockedRWOnThread
   void SetCycleCollectedJSContext(CycleCollectedJSContext* aCCJSContext);
   void ClearCycleCollectedJSContext();
 
-  // Poll to see if JS sampling should be started/stopped.
+  // Poll to see if JS sampling should be started/stopped. Convenience wrapper
+  // that calls TakeJSSamplingChange() then ApplyJSSamplingChange() below; it
+  // must be called with no profiler lock held.
   void PollJSSampling();
 
+  // PollJSSampling(), split so a caller can drop the profiler locks before the
+  // JS-engine calls. TakeJSSamplingChange() flips mJSSampling for a pending
+  // request and returns what change to apply; it must run under the thread's
+  // data lock. ApplyJSSamplingChange() makes the js::Enable* calls and must run
+  // with NO profiler lock held: those calls can block waiting on the JS helper
+  // threads, which themselves need the profiler locks, so holding a profiler
+  // lock across them can deadlock.
+  struct JSSamplingChange {
+    enum class Action { None, Start, Stop };
+    Action mAction = Action::None;
+    JSContext* mContext = nullptr;
+    bool mAllocationsEnabled = false;
+  };
+  [[nodiscard]] JSSamplingChange TakeJSSamplingChange();
+  static void ApplyJSSamplingChange(const JSSamplingChange& aChange);
+
  public:
   ThreadRegistrationLockedRWOnThread(const char* aName, const void* aStackTop)
       : ThreadRegistrationLockedRWFromAnyThread(aName, aStackTop) {}