Backed out changeset b5217784536c (bug 1777921) for causing assertion failures at WorkerPrivate.h. CLOSED TREE

dom/workers/RuntimeService.cpp

@@ -2115,8 +2115,6 @@ WorkerThreadPrimaryRunnable::Run() {
       return NS_ERROR_FAILURE;
     }
 
-    nsWeakPtr globalScopeSentinel;
-    nsWeakPtr debuggerScopeSentinel;
     {
       nsCycleCollector_startup();
 
@@ -2172,8 +2170,9 @@ WorkerThreadPrimaryRunnable::Run() {
 
       // At this point we expect the scopes to be alive if they were ever
       // created successfully, keep weak references.
-      globalScopeSentinel = do_GetWeakReference(mWorkerPrivate->GlobalScope());
-      debuggerScopeSentinel =
+      nsWeakPtr globalScopeSentinel =
+          do_GetWeakReference(mWorkerPrivate->GlobalScope());
+      nsWeakPtr debuggerScopeSentinel =
           do_GetWeakReference(mWorkerPrivate->DebuggerGlobalScope());
       MOZ_ASSERT(!mWorkerPrivate->GlobalScope() || globalScopeSentinel);
       MOZ_ASSERT(!mWorkerPrivate->DebuggerGlobalScope() ||
@@ -2204,29 +2203,29 @@ WorkerThreadPrimaryRunnable::Run() {
       // If ever the CC shutdown run caused side effects, process them.
       NS_ProcessPendingEvents(nullptr);
 
+      // Check sentinels if we actually removed all global scope references.
+      nsCOMPtr<DOMEventTargetHelper> globalScopeAlive =
+          do_QueryReferent(globalScopeSentinel);
+      MOZ_ASSERT(!globalScopeAlive);
+      nsCOMPtr<DOMEventTargetHelper> debuggerScopeAlive =
+          do_QueryReferent(debuggerScopeSentinel);
+      MOZ_ASSERT(!debuggerScopeAlive);
+
+      // Guard us against further usage of scopes' mWorkerPrivate in non-debug.
+      if (globalScopeAlive) {
+        static_cast<WorkerGlobalScopeBase*>(globalScopeAlive.get())
+            ->NoteWorkerTerminated();
+        globalScopeAlive = nullptr;
+      }
+      if (debuggerScopeAlive) {
+        static_cast<WorkerGlobalScopeBase*>(debuggerScopeAlive.get())
+            ->NoteWorkerTerminated();
+        debuggerScopeAlive = nullptr;
+      }
+
       // Now WorkerJSContext goes out of scope. Do not use any cycle
       // collectable objects nor JS after this point!
     }
-
-    // Check sentinels if we actually removed all global scope references.
-    nsCOMPtr<DOMEventTargetHelper> globalScopeAlive =
-        do_QueryReferent(globalScopeSentinel);
-    MOZ_ASSERT(!globalScopeAlive);
-    nsCOMPtr<DOMEventTargetHelper> debuggerScopeAlive =
-        do_QueryReferent(debuggerScopeSentinel);
-    MOZ_ASSERT(!debuggerScopeAlive);
-
-    // Guard us against further usage of scopes' mWorkerPrivate in case.
-    if (globalScopeAlive) {
-      static_cast<WorkerGlobalScopeBase*>(globalScopeAlive.get())
-          ->NoteWorkerTerminated();
-      globalScopeAlive = nullptr;
-    }
-    if (debuggerScopeAlive) {
-      static_cast<WorkerGlobalScopeBase*>(debuggerScopeAlive.get())
-          ->NoteWorkerTerminated();
-      debuggerScopeAlive = nullptr;
-    }
   }
 
   mWorkerPrivate->ScheduleDeletion(WorkerPrivate::WorkerRan);

dom/workers/WorkerPrivate.cpp

@@ -3150,76 +3150,23 @@ 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;
-
-    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
-      }
-    }
+    bool debuggerRunnablesPending = false;
+    bool normalRunnablesPending = false;
 
     {
       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 (noRunnablesPendingAndKeepAlive()) {
+      // 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())) {
         // 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,13 +548,6 @@ 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!
@@ -1469,7 +1462,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
@@ -1485,9 +1478,6 @@ 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,9 +65,6 @@ void WorkerRef::ReleaseWorker() {
   if (mHolding) {
     MOZ_ASSERT(mWorkerPrivate);
 
-    if (mIsPreventingShutdown) {
-      mWorkerPrivate->AssertIsNotPotentiallyLastGCCCRunning();
-    }
     mWorkerPrivate->RemoveWorkerRef(this);
     mWorkerPrivate = nullptr;
 

modules/libpref/init/StaticPrefList.yaml

@@ -3985,19 +3985,6 @@
   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@