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

@@ -1722,14 +1722,20 @@ IncrementalFinalizeRunnable::Run() {
 
 void CycleCollectedJSRuntime::FinalizeDeferredThings(
     DeferredFinalizeType aType) {
-  /*
+  // If mFinalizeRunnable isn't null, we didn't finalize everything from the
-   * If the previous GC created a runnable to finalize objects
+  // previous GC.
-   * 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) {
+    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) {
+  if (aType == FinalizeNow) {
-    NS_DispatchToCurrentThreadQueue(do_AddRef(mFinalizeRunnable), 2500,
-                                    EventQueuePriority::Idle);
-  } else {
     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
+      DeferredFinalizeType finalizeType;
-      // finalizer later after we've returned to the event loop if any of
+      if (JS_IsExceptionPending(aContext)) {
-      // three conditions hold:
+        // There is a pending exception. The finalizers are not set up to run
-      // a) The GC is incremental. In this case, we probably care about pauses.
+        // in that state, so don't run the finalizer until we've returned to the
-      // b) There is a pending exception. The finalizers are not set up to run
+        // event loop.
-      // in that state.
+        finalizeType = FinalizeLater;
-      // c) The GC was triggered for internal JS engine reasons. If this is the
+      } else if (JS::InternalGCReason(aReason)) {
-      // case, then we may be in the middle of running some code that the JIT
+        if (aReason == JS::GCReason::DESTROY_RUNTIME) {
-      // has assumed can't have certain kinds of side effects. Finalizers can do
+          // We're shutting down, so we need to destroy things immediately.
-      // all sorts of things, such as run JS, so we want to run them later.
+          finalizeType = FinalizeNow;
-      // However, if we're shutting down, we need to destroy things immediately.
+        } else {
-      //
+          // We may be in the middle of running some code that the JIT has
-      // Why do we ever bother finalizing things immediately if that's so
+          // assumed can't have certain kinds of side effects. Finalizers can do
-      // questionable? In some situations, such as while testing or in low
+          // all sorts of things, such as run JS, so we want to run them later,
-      // memory situations, we really want to free things right away.
+          // after we've returned to the event loop.
-      bool finalizeIncrementally = JS::WasIncrementalGC(mJSRuntime) ||
+          finalizeType = FinalizeLater;
-                                   JS_IsExceptionPending(aContext) ||
+        }
-                                   (JS::InternalGCReason(aReason) &&
+      } else if (JS::WasIncrementalGC(mJSRuntime)) {
-                                    aReason != JS::GCReason::DESTROY_RUNTIME);
+        // The GC was incremental, so we probably care about pauses. Try to
-
+        // break up finalization, but it is okay if we do some now.
-      FinalizeDeferredThings(finalizeIncrementally ? FinalizeIncrementally
+        finalizeType = FinalizeIncrementally;
-                                                   : FinalizeNow);
+      } 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;
     }

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,
   };