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

@@ -21,6 +21,7 @@
 #include "mozilla/BasePrincipal.h"
 #include "mozilla/CycleCollectedJSContext.h"
 #include "mozilla/ExtensionPolicyService.h"
+#include "mozilla/Mutex.h"
 #include "mozilla/ProfilerLabels.h"
 #include "mozilla/Result.h"
 #include "mozilla/ScopeExit.h"
@@ -54,6 +55,7 @@
 #include "mozilla/dom/TimeoutHandler.h"
 #include "mozilla/dom/WorkerBinding.h"
 #include "mozilla/dom/WorkerScope.h"
+#include "mozilla/dom/WorkerStatus.h"
 #include "mozilla/dom/WebTaskScheduler.h"
 #include "mozilla/dom/JSExecutionManager.h"
 #include "mozilla/dom/WindowContext.h"
@@ -63,6 +65,7 @@
 #include "mozilla/StoragePrincipalHelper.h"
 #include "mozilla/Telemetry.h"
 #include "mozilla/ThreadEventQueue.h"
+#include "mozilla/ThreadSafety.h"
 #include "mozilla/ThrottledEventQueue.h"
 #include "mozilla/TimelineConsumers.h"
 #include "mozilla/WorkerTimelineMarker.h"
@@ -83,6 +86,7 @@
 #include "nsQueryObject.h"
 #include "nsRFPService.h"
 #include "nsSandboxFlags.h"
+#include "nsThreadUtils.h"
 #include "nsUTF8Utils.h"
 
 #include "RuntimeService.h"
@@ -3186,10 +3190,61 @@ 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 &&
+             data->mPerformedShutdownAfterLastContentTaskExecuted);
+      }
+      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);
@@ -3198,16 +3253,18 @@ void WorkerPrivate::DoRunLoop(JSContext* aCx) {
           ("WorkerPrivate::DoRunLoop [%p] mStatus %u before getting events"
            " to run",
            this, (uint8_t)mStatus));
+      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
@@ -3219,7 +3276,9 @@ void WorkerPrivate::DoRunLoop(JSContext* aCx) {
 
       auto result = ProcessAllControlRunnablesLocked();
       if (result != ProcessAllControlRunnablesResult::Nothing) {
-        continue;
+        // Update all saved runnable flags for side effect for the
+        // loop check about transitioning to Killing below.
+        (void)noRunnablesPendingAndKeepAlive();
       }
 
       currentStatus = mStatus;

dom/workers/WorkerPrivate.h

@@ -543,6 +543,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!
@@ -1479,6 +1486,9 @@ class WorkerPrivate final
     FlippedOnce<false> mDeletionScheduled;
     FlippedOnce<false> mCancelBeforeWorkerScopeConstructed;
     FlippedOnce<false> mPerformedShutdownAfterLastContentTaskExecuted;
+#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

@@ -3995,6 +3995,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
+# when 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@