Bug 1777921 - Assert that StrongWorkerRefs are not lazily released during final GC/CC. r=dom-worker-reviewers,smaug,asuth

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

dom/workers/WorkerPrivate.cpp

@@ -3149,23 +3149,76 @@ void WorkerPrivate::DoRunLoop(JSContext* aCx) {
 
   InitializeGCTimers();
 
+  bool checkFinalGCCC =
+      StaticPrefs::dom_workers_GCCC_on_potentially_last_event();
+
+  bool debuggerRunnablesPending = false;
+  bool normalRunnablesPending = false;
+  auto noRunnablesPendingAndKeepAlive =
+      [&debuggerRunnablesPending, &normalRunnablesPending, &thread, this]()
+          MOZ_REQUIRES(mMutex) {
+            // We want to keep both pending flags always updated while looping.
+            debuggerRunnablesPending = !mDebuggerQueue.IsEmpty();
+            normalRunnablesPending = NS_HasPendingEvents(thread);
+
+            bool anyRunnablesPending = !mControlQueue.IsEmpty() ||
+                                       debuggerRunnablesPending ||
+                                       normalRunnablesPending;
+            bool keepWorkerAlive = mStatus == Running || HasActiveWorkerRefs();
+
+            return (!anyRunnablesPending && keepWorkerAlive);
+          };
+
   for (;;) {
     WorkerStatus currentStatus;
-    bool debuggerRunnablesPending = false;
-    bool normalRunnablesPending = false;
+
+    if (checkFinalGCCC) {
+      // If we get here after the last event ran but someone holds a WorkerRef
+      // and there is no other logic to release that WorkerRef than lazily
+      // through GC/CC, we might block forever on the next WaitForWorkerEvents.
+      // Every object holding a WorkerRef should really have a straight,
+      // deterministic line from the WorkerRef's callback being invoked to the
+      // WorkerRef being released which is supported by strong-references that
+      // can't form a cycle.
+      bool mayNeedFinalGCCC = false;
+      {
+        MutexAutoLock lock(mMutex);
+
+        currentStatus = mStatus;
+        mayNeedFinalGCCC =
+            (mStatus >= Canceling && HasActiveWorkerRefs() &&
+             !debuggerRunnablesPending && !normalRunnablesPending);
+      }
+      if (mayNeedFinalGCCC) {
+#ifdef MOZ_DIAGNOSTIC_ASSERT_ENABLED
+        // WorkerRef::ReleaseWorker will check this flag via
+        // AssertIsNotPotentiallyLastGCCCRunning
+        data->mIsPotentiallyLastGCCCRunning = true;
+#endif
+        // GarbageCollectInternal will trigger both GC and CC
+        GarbageCollectInternal(aCx, true /* aShrinking */,
+                               true /* aCollectChildren */);
+#ifdef MOZ_DIAGNOSTIC_ASSERT_ENABLED
+        data->mIsPotentiallyLastGCCCRunning = false;
+#endif
+      }
+    }
 
     {
       MutexAutoLock lock(mMutex);
 
+      if (checkFinalGCCC && currentStatus != mStatus) {
+        // Something moved our status while we were supposed to check for a
+        // potentially needed GC/CC. Just check again.
+        continue;
+      }
+
       // Wait for a runnable to arrive that we can execute, or for it to be okay
       // to shutdown this worker once all holders have been removed.
-      // Holders may be removed from inside normal runnables, but we don't check
-      // for that after processing normal runnables, so we need to let control
-      // flow to the shutdown logic without blocking.
-      while (mControlQueue.IsEmpty() &&
-             !(debuggerRunnablesPending = !mDebuggerQueue.IsEmpty()) &&
-             !(normalRunnablesPending = NS_HasPendingEvents(thread)) &&
-             !(mStatus != Running && !HasActiveWorkerRefs())) {
+      // Holders may be removed from inside normal runnables,  but we don't
+      // check for that after processing normal runnables, so we need to let
+      // control flow to the shutdown logic without blocking.
+      while (noRunnablesPendingAndKeepAlive()) {
         // We pop out to this loop when there are no pending events.
         // If we don't reset these, we may not re-enter ProcessNextEvent()
         // until we have events to process, and it may seem like we have

dom/workers/WorkerPrivate.h

@@ -548,6 +548,13 @@ class WorkerPrivate final
   }
 #endif
 
+  void AssertIsNotPotentiallyLastGCCCRunning() {
+#ifdef MOZ_DIAGNOSTIC_ASSERT_ENABLED
+    auto data = mWorkerThreadAccessible.Access();
+    MOZ_DIAGNOSTIC_ASSERT(!data->mIsPotentiallyLastGCCCRunning);
+#endif
+  }
+
   void SetWorkerScriptExecutedSuccessfully() {
     AssertIsOnWorkerThread();
     // Should only be called once!
@@ -1462,7 +1469,7 @@ class WorkerPrivate final
     // created with a nesting level one more than the current nesting level,
     // saturating at the kClampTimeoutNestingLevel.
     //
-    // When RunExpiredTimeouts is run, it sets this value to the
+    // When RunExpiredTimeouts is run,  it sets this value to the
     // TimeoutInfo::mNestingLevel for the duration of
     // the WorkerScriptTimeoutHandler::Call which will explicitly trigger a
     // microtask checkpoint so that any immediately-resolved promises will
@@ -1478,6 +1485,9 @@ class WorkerPrivate final
     bool mJSThreadExecutionGranted;
     bool mCCCollectedAnything;
     FlippedOnce<false> mDeletionScheduled;
+#ifdef MOZ_DIAGNOSTIC_ASSERT_ENABLED
+    bool mIsPotentiallyLastGCCCRunning = false;
+#endif
   };
   ThreadBound<WorkerThreadAccessible> mWorkerThreadAccessible;
 

dom/workers/WorkerRef.cpp

@@ -65,6 +65,9 @@ void WorkerRef::ReleaseWorker() {
   if (mHolding) {
     MOZ_ASSERT(mWorkerPrivate);
 
+    if (mIsPreventingShutdown) {
+      mWorkerPrivate->AssertIsNotPotentiallyLastGCCCRunning();
+    }
     mWorkerPrivate->RemoveWorkerRef(this);
     mWorkerPrivate = nullptr;
 

modules/libpref/init/StaticPrefList.yaml

@@ -3976,6 +3976,19 @@
   value: false
   mirror: always
 
+# Enable stronger diagnostics on worker shutdown.
+# If this is true, we will potentially run an extra GCCC when a  worker should
+# exit its DoRunLoop but holds any WorkerRef and we will MOZ_DIAGNOSTIC_ASSERT
+# if during that extra GCCC such a WorkerRef is freed.
+- name: dom.workers.GCCC_on_potentially_last_event
+  type: RelaxedAtomicBool
+#if defined(FUZZING) || defined(DEBUG)
+  value: true
+#else
+  value: false
+#endif
+  mirror: always
+
 - name: dom.sitepermsaddon-provider.enabled
   type: bool
   value: @IS_NOT_ANDROID@