From 4d2921d595e36c44cfc35b45f859b91b2d28c4d3 Mon Sep 17 00:00:00 2001 From: Jens Stutte Date: Mon, 12 Jun 2023 18:58:50 +0000 Subject: [PATCH] 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 | 79 +++++++++++++++++++++--- dom/workers/WorkerPrivate.h | 10 +++ dom/workers/WorkerRef.cpp | 3 + modules/libpref/init/StaticPrefList.yaml | 13 ++++ 4 files changed, 95 insertions(+), 10 deletions(-) diff --git a/dom/workers/WorkerPrivate.cpp b/dom/workers/WorkerPrivate.cpp index a3bfca19d322..ebab2b6710c5 100644 --- a/dom/workers/WorkerPrivate.cpp +++ b/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; diff --git a/dom/workers/WorkerPrivate.h b/dom/workers/WorkerPrivate.h index 932f249fd0c1..e78423f757d8 100644 --- a/dom/workers/WorkerPrivate.h +++ b/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 mDeletionScheduled; FlippedOnce mCancelBeforeWorkerScopeConstructed; FlippedOnce mPerformedShutdownAfterLastContentTaskExecuted; +#ifdef MOZ_DIAGNOSTIC_ASSERT_ENABLED + bool mIsPotentiallyLastGCCCRunning = false; +#endif }; ThreadBound mWorkerThreadAccessible; diff --git a/dom/workers/WorkerRef.cpp b/dom/workers/WorkerRef.cpp index 9fcf329495b7..342d1b0f7cf4 100644 --- a/dom/workers/WorkerRef.cpp +++ b/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; diff --git a/modules/libpref/init/StaticPrefList.yaml b/modules/libpref/init/StaticPrefList.yaml index 8bdfd7615fee..67ba3f063794 100644 --- a/modules/libpref/init/StaticPrefList.yaml +++ b/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@