Bug 1584195: Break Debugger.Frame -> generator JSScript edges before finalization. r=jonco
authorJim Blandy <jimb@mozilla.com>
Thu, 03 Oct 2019 17:46:52 +0000
changeset 496398 ea0524a93cf3d94bf808c41ce879d14cdd12e04e
parent 496397 8c47c7026dac9810f8d9f16e0c3ead1e3baeb5c6
child 496399 ea19d193ebbbe579e517e7da0d26b6390eb011e2
push id97221
push userjblandy@mozilla.com
push dateFri, 04 Oct 2019 20:47:41 +0000
treeherderautoland@ea0524a93cf3 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjonco
bugs1584195
milestone71.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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
js/src/debugger/DebugAPI.h
js/src/debugger/Debugger.cpp
js/src/debugger/Frame.cpp
js/src/gc/GC.cpp
js/src/jit-test/tests/debug/bug-1584195.js
--- a/js/src/debugger/DebugAPI.h
+++ b/js/src/debugger/DebugAPI.h
@@ -93,17 +93,20 @@ class DebugAPI {
   static MOZ_MUST_USE bool markIteratively(GCMarker* marker);
 
   // Trace cross compartment edges in all debuggers relevant to the current GC.
   static void traceCrossCompartmentEdges(JSTracer* tracer);
 
   // 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.
   static MOZ_MUST_USE bool findSweepGroupEdges(JSRuntime* rt);
 
   // Sweep breakpoints in a script associated with any debugger.
   static inline void sweepBreakpoints(JSFreeOp* fop, JSScript* script);
 
--- a/js/src/debugger/Debugger.cpp
+++ b/js/src/debugger/Debugger.cpp
@@ -3869,19 +3869,37 @@ void Debugger::trace(JSTracer* trc) {
 
   forEachWeakMap([trc](auto& weakMap) { weakMap.trace(trc); });
 }
 
 /* static */
 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
     // object is finalized.
     bool debuggerDying = IsAboutToBeFinalized(&dbg->object);
     for (WeakGlobalObjectSet::Enum e(dbg->debuggees); !e.empty();
          e.popFront()) {
       GlobalObject* global = e.front().unbarrieredGet();
--- a/js/src/debugger/Frame.cpp
+++ b/js/src/debugger/Frame.cpp
@@ -370,17 +370,17 @@ bool DebuggerFrame::setGenerator(JSConte
 
 void DebuggerFrame::clearGenerator(JSFreeOp* fop) {
   if (!hasGenerator()) {
     return;
   }
 
   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
   // incremented across suspensions. This means that, whereas ordinary calls
   // never need to drop the stepper count from the D.F finalizer, generator
   // calls may.
   HeapPtr<JSScript*>& generatorScript = info->generatorScript();
   if (!IsAboutToBeFinalized(&generatorScript)) {
@@ -1065,18 +1065,22 @@ void DebuggerFrame::maybeDecrementFrameS
   handler->drop(fop, this);
   setReservedSlot(ONSTEP_HANDLER_SLOT, UndefinedValue());
 }
 
 /* static */
 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);
   }
   OnPopHandler* onPopHandler = frameobj.onPopHandler();
   if (onPopHandler) {
     onPopHandler->drop(fop, &frameobj);
   }
--- a/js/src/gc/GC.cpp
+++ b/js/src/gc/GC.cpp
@@ -5530,20 +5530,23 @@ void GCRuntime::beginSweepPhase(JS::GCRe
 
 bool ArenaLists::foregroundFinalize(JSFreeOp* fop, AllocKind thingKind,
                                     SliceBudget& sliceBudget,
                                     SortedArenaList& sweepList) {
   if (!arenaListsToSweep(thingKind) && incrementalSweptArenas.ref().isEmpty()) {
     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;
     incrementalSweptArenas = sweepList.toArenaList();
     return false;
   }
 
   // Clear any previous incremental sweep state we may have saved.
new file mode 100644
--- /dev/null
+++ b/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("");