Bug 1584195: Break Debugger.Frame -> generator JSScript edges before finalization. r=jonco

When a Debugger.Frame refers to a generator or async function call, the script
must be compiled with instrumentation to notify the Debugger API when the call
is resumed. To accomplish this, a generator's JSScript's DebugScript's
generatorObserverCount tracks the number of Debugger.Frames referring to calls
running the script. When the count is non-zero, instrumentation is required.

When a Debugger.Frame for a suspended generator call is garbage collected, its
JSScript's generatorObserverCount must be decremented. However, if the JSScript
is alo being garbage collected, it may have already been finalized, and should
not be touched.

The prior code had js::DebuggerFrame::finalize use IsAboutToBeFinalized to check
whether the JSScript was healthy enough to have its count decremented. Since the
garbage collector always handles debuggers and debuggees in the same GC cycle,
it was believed that the JSScript's mark bit (what IsAboutToBeFinalized checks)
would be accurate.

Unfortunately, it is possible for a JSScript to be finalized in one GC slice,
and then for the mutator to run, and then for the DebuggerFrame to be finalized
in the next slice, with the JSScript's arena available for new allocations in
the middle. When an arena is made available for allocation during an incremental
GC, all its mark bits are set. As a result, the DebuggerFrame finalizer believes
that the JSScript it was referring to is still alive, even thought it has been
finalized. IsAboutToBeFinalized is only safe to use on edges between cells of
the same alloc kind, since we do promise to run all finalizers for a given alloc
kind before putting those arenas up for reuse. It's not safe to use
IsAboutToBeFinalized to inspect edges between cells of differing alloc kinds,
like DebuggerFrame JSObjects and generator JSScripts.

Fortunately, there is another approach we can take. The garbage collector calls
`DebugAPI::sweepAll` after all marking is done, but before any finalization
takes place. At this point, all cells' mark bits are accurate (assuming they're
in a zone being GC'd), but nothing has been finalized. We can disconnect
unmarked DebuggerFrames from their scripts now, knowing that both parties are
still fully initialized.

Differential Revision: https://phabricator.services.mozilla.com/D47721

--HG--
extra : moz-landing-system : lando

js/src/debugger/DebugAPI.h

@@ -98,7 +98,10 @@ class DebugAPI {
   // Trace all debugger-owned GC things unconditionally, during a moving GC.
   static void traceAllForMovingGC(JSTracer* trc);
 
-  // Sweep dying debuggers, and detach edges to dying debuggees.
+  // The garbage collector calls this after everything has been marked, but
+  // before anything has been finalized. We use this to clear Debugger /
+  // debuggee edges at a point where the parties concerned are all still
+  // initialized.
   static void sweepAll(JSFreeOp* fop);
 
   // Add sweep group edges due to the presence of any debuggers.

js/src/debugger/Debugger.cpp

@@ -3874,9 +3874,27 @@ void Debugger::trace(JSTracer* trc) {
 void DebugAPI::sweepAll(JSFreeOp* fop) {
   JSRuntime* rt = fop->runtime();
 
-  Debugger* dbg = rt->debuggerList().getFirst();
-  while (dbg) {
-    Debugger* next = dbg->getNext();
+  Debugger* next;
+  for (Debugger* dbg = rt->debuggerList().getFirst(); dbg; dbg = next) {
+    next = dbg->getNext();
+
+    // Debugger.Frames for generator calls bump the JSScript's
+    // generatorObserverCount, so the JIT will instrument the code to notify
+    // Debugger when the generator is resumed. When a Debugger.Frame gets GC'd,
+    // generatorObserverCount needs to be decremented. It's much easier to do
+    // this when we know that all parties involved - the Debugger.Frame, the
+    // generator object, and the JSScript - have not yet been finalized.
+    //
+    // Since DebugAPI::sweepAll is called after everything is marked, but before
+    // anything has been finalized, this is the perfect place to drop the count.
+    if (dbg->zone()->isGCSweeping()) {
+      for (Debugger::GeneratorWeakMap::Enum e(dbg->generatorFrames); !e.empty(); e.popFront()) {
+        DebuggerFrame* frameObj = &e.front().value()->as<DebuggerFrame>();
+        if (IsAboutToBeFinalizedUnbarriered(&frameObj)) {
+          frameObj->clearGenerator(fop, dbg, &e);
+        }
+      }
+    }
 
     // Detach dying debuggers and debuggees from each other. Since this
     // requires access to both objects it must be done before either

js/src/debugger/Frame.cpp

@@ -375,7 +375,7 @@ void DebuggerFrame::clearGenerator(JSFreeOp* fop) {
 
   GeneratorInfo* info = generatorInfo();
 
-  // 4) The generator's script's observer count must be dropped.
+  // 3) The generator's script's observer count must be dropped.
   //
   // For ordinary calls, Debugger.Frame objects drop the script's stepper count
   // when the frame is popped, but for generators, they leave the stepper count
@@ -1070,8 +1070,12 @@ void DebuggerFrame::maybeDecrementFrameScriptStepperCount(
 void DebuggerFrame::finalize(JSFreeOp* fop, JSObject* obj) {
   MOZ_ASSERT(fop->onMainThread());
   DebuggerFrame& frameobj = obj->as<DebuggerFrame>();
+
+  // Connections between dying Debugger.Frames and their
+  // AbstractGeneratorObjects should have been broken in DebugAPI::sweepAll.
+  MOZ_ASSERT(!frameobj.hasGenerator());
+
   frameobj.freeFrameIterData(fop);
-  frameobj.clearGenerator(fop);
   OnStepHandler* onStepHandler = frameobj.onStepHandler();
   if (onStepHandler) {
     onStepHandler->drop(fop, &frameobj);

js/src/gc/GC.cpp

@@ -5535,10 +5535,13 @@ bool ArenaLists::foregroundFinalize(JSFreeOp* fop, AllocKind thingKind,
     return true;
   }
 
-  // Empty arenas are not released until all foreground finalized GC things in
-  // the current sweep group have been finalized.  This allows finalizers for
-  // other cells in the same sweep group to call IsAboutToBeFinalized on cells
-  // in this arena.
+  // Arenas are released for use for new allocations as soon as the finalizers
+  // for that allocation kind have run. This means that a cell's finalizer can
+  // safely use IsAboutToBeFinalized to check other cells of the same alloc
+  // kind, but not of different alloc kinds: the other arena may have already
+  // had new objects allocated in it, and since we allocate black,
+  // IsAboutToBeFinalized will return false even though the referent we intended
+  // to check is long gone.
   if (!FinalizeArenas(fop, &arenaListsToSweep(thingKind), sweepList, thingKind,
                       sliceBudget)) {
     incrementalSweptArenaKind = thingKind;

js/src/jit-test/tests/debug/bug-1584195.js

@@ -0,0 +1,31 @@
+// |jit-test| skip-if: !('gczeal' in this)
+// Bug 1584195: Debugger.Frame finalizer should't try to apply
+// IsAboutToBeFinalized to cells of other alloc kinds, whose arenas may have
+// been turned over to fresh allocations.
+
+try {
+  evaluate(`
+    var g9 = newGlobal({newCompartment: true});
+    g9.parent = this;
+    g9.eval(\`
+      var dbg = new Debugger(parent);
+      dbg.onEnterFrame = frame => {};
+    \`);
+    function lfPromise(x) {
+        return new Promise(resolve => {});
+    }
+    async function lfAsync() {
+        await lfPromise();
+    } lfAsync();
+  `);
+  gczeal(20, 2);
+  evaluate(`
+    async function lfAsync() {} lfAsync();
+  `);
+  gczeal(12, 7);
+  drainJobQueue();
+  evaluate(`
+    new { ...  v22 => { 
+  `);
+} catch (exc) {}
+evaluate("");