From 3dbdc7f6ac6d1d498cdaf9f44cfd2dc6b7f42fcc Mon Sep 17 00:00:00 2001 From: Andrew McCreight Date: Fri, 3 Nov 2023 14:27:22 +0000 Subject: [PATCH] Bug 1862782, part 2 - Split out cases where we must never immediately finalize. r=smaug Differential Revision: https://phabricator.services.mozilla.com/D192639 --- xpcom/base/CycleCollectedJSRuntime.cpp | 80 +++++++++++++++----------- xpcom/base/CycleCollectedJSRuntime.h | 4 ++ 2 files changed, 51 insertions(+), 33 deletions(-) diff --git a/xpcom/base/CycleCollectedJSRuntime.cpp b/xpcom/base/CycleCollectedJSRuntime.cpp index 99abbe3a5e30..3813b12f17b7 100644 --- a/xpcom/base/CycleCollectedJSRuntime.cpp +++ b/xpcom/base/CycleCollectedJSRuntime.cpp @@ -1722,14 +1722,20 @@ IncrementalFinalizeRunnable::Run() { void CycleCollectedJSRuntime::FinalizeDeferredThings( DeferredFinalizeType aType) { - /* - * If the previous GC created a runnable to finalize objects - * incrementally, and if it hasn't finished yet, finish it now. We - * don't want these to build up. We also don't want to allow any - * existing incremental finalize runnables to run after a - * non-incremental GC, since they are often used to detect leaks. - */ + // If mFinalizeRunnable isn't null, we didn't finalize everything from the + // previous GC. if (mFinalizeRunnable) { + if (aType == FinalizeLater) { + // We need to defer all finalization until we return to the event loop, + // so leave things alone. Any new objects to be finalized from the current + // GC will be handled by the existing mFinalizeRunnable. + return; + } + MOZ_ASSERT(aType == FinalizeIncrementally || aType == FinalizeNow); + // If we're finalizing incrementally, we don't want finalizers to build up, + // so try to finish them off now. + // If we're finalizing synchronously, also go ahead and clear them out, + // so we make sure as much as possible is freed. mFinalizeRunnable->ReleaseNow(false); if (mFinalizeRunnable) { // If we re-entered ReleaseNow, we couldn't delete mFinalizeRunnable and @@ -1738,6 +1744,7 @@ void CycleCollectedJSRuntime::FinalizeDeferredThings( } } + // If there's nothing to finalize, don't create a new runnable. if (mDeferredFinalizerTable.Count() == 0) { return; } @@ -1748,12 +1755,13 @@ void CycleCollectedJSRuntime::FinalizeDeferredThings( // Everything should be gone now. MOZ_ASSERT(mDeferredFinalizerTable.Count() == 0); - if (aType == FinalizeIncrementally) { - NS_DispatchToCurrentThreadQueue(do_AddRef(mFinalizeRunnable), 2500, - EventQueuePriority::Idle); - } else { + if (aType == FinalizeNow) { mFinalizeRunnable->ReleaseNow(false); MOZ_ASSERT(!mFinalizeRunnable); + } else { + MOZ_ASSERT(aType == FinalizeIncrementally || aType == FinalizeLater); + NS_DispatchToCurrentThreadQueue(do_AddRef(mFinalizeRunnable), 2500, + EventQueuePriority::Idle); } } @@ -1808,28 +1816,34 @@ void CycleCollectedJSRuntime::OnGC(JSContext* aContext, JSGCStatus aStatus, OOMState::Recovered); } - // Do any deferred finalization of native objects. We will run the - // finalizer later after we've returned to the event loop if any of - // three conditions hold: - // a) The GC is incremental. In this case, we probably care about pauses. - // b) There is a pending exception. The finalizers are not set up to run - // in that state. - // c) The GC was triggered for internal JS engine reasons. If this is the - // case, then we may be in the middle of running some code that the JIT - // has assumed can't have certain kinds of side effects. Finalizers can do - // all sorts of things, such as run JS, so we want to run them later. - // However, if we're shutting down, we need to destroy things immediately. - // - // Why do we ever bother finalizing things immediately if that's so - // questionable? In some situations, such as while testing or in low - // memory situations, we really want to free things right away. - bool finalizeIncrementally = JS::WasIncrementalGC(mJSRuntime) || - JS_IsExceptionPending(aContext) || - (JS::InternalGCReason(aReason) && - aReason != JS::GCReason::DESTROY_RUNTIME); - - FinalizeDeferredThings(finalizeIncrementally ? FinalizeIncrementally - : FinalizeNow); + DeferredFinalizeType finalizeType; + if (JS_IsExceptionPending(aContext)) { + // There is a pending exception. The finalizers are not set up to run + // in that state, so don't run the finalizer until we've returned to the + // event loop. + finalizeType = FinalizeLater; + } else if (JS::InternalGCReason(aReason)) { + if (aReason == JS::GCReason::DESTROY_RUNTIME) { + // We're shutting down, so we need to destroy things immediately. + finalizeType = FinalizeNow; + } else { + // We may be in the middle of running some code that the JIT has + // assumed can't have certain kinds of side effects. Finalizers can do + // all sorts of things, such as run JS, so we want to run them later, + // after we've returned to the event loop. + finalizeType = FinalizeLater; + } + } else if (JS::WasIncrementalGC(mJSRuntime)) { + // The GC was incremental, so we probably care about pauses. Try to + // break up finalization, but it is okay if we do some now. + finalizeType = FinalizeIncrementally; + } else { + // If we're running a synchronous GC, we probably want to free things as + // quickly as possible. This can happen during testing or if memory is + // low. + finalizeType = FinalizeNow; + } + FinalizeDeferredThings(finalizeType); break; } diff --git a/xpcom/base/CycleCollectedJSRuntime.h b/xpcom/base/CycleCollectedJSRuntime.h index e33ea0e5ba20..6f03d3ee9968 100644 --- a/xpcom/base/CycleCollectedJSRuntime.h +++ b/xpcom/base/CycleCollectedJSRuntime.h @@ -302,7 +302,11 @@ class CycleCollectedJSRuntime { public: enum DeferredFinalizeType { + // Never finalize immediately, because it would be unsafe. + FinalizeLater, + // Finalize later if we can, but it is okay to do it immediately. FinalizeIncrementally, + // Finalize immediately, for shutdown or testing purposes. FinalizeNow, };