Bug 1448880 - Part 7: Fix hasAnyLiveHooks to include onStep handlers on suspended frames. r=sfink
authorJason Orendorff <jorendorff@mozilla.com>
Wed, 24 Oct 2018 01:08:19 +0000
changeset 491043 8ff4dac9916d9bd908611963de3ec8883befea5f
parent 491042 6c8949f053e0e9c6216329d9b3f081fc0b6b2936
child 491044 3b8a9abe2766c4bdc68c143416a3aafe85083be4
push id247
push userfmarier@mozilla.com
push dateSat, 27 Oct 2018 01:06:44 +0000
reviewerssfink
bugs1448880
milestone65.0a1
Bug 1448880 - Part 7: Fix hasAnyLiveHooks to include onStep handlers on suspended frames. r=sfink Depends on D6988 Differential Revision: https://phabricator.services.mozilla.com/D6990
js/src/jit-test/tests/debug/Frame-onStep-async-gc-01.js
js/src/jit-test/tests/debug/Frame-onStep-generators-gc-01.js
js/src/vm/Debugger.cpp
js/src/vm/Debugger.h
new file mode 100644
--- /dev/null
+++ b/js/src/jit-test/tests/debug/Frame-onStep-async-gc-01.js
@@ -0,0 +1,28 @@
+// An onStep handler on a suspended async function frame keeps a Debugger alive.
+
+let g = newGlobal();
+g.eval(`
+  async function f() {
+    debugger;
+    await Promise.resolve(0);
+    return 'ok';
+  }
+`);
+
+let dbg = Debugger(g);
+let hit = false;
+dbg.onDebuggerStatement = frame => {
+    frame.onPop = completion => {
+        frame.onStep = () => { hit = true; };
+        frame.onPop = undefined;
+    };
+    dbg.onDebuggerStatement = undefined;
+    dbg = null;
+};
+
+g.f();
+assertEq(dbg, null);
+gc();
+assertEq(hit, false);
+drainJobQueue();
+assertEq(hit, true);
new file mode 100644
--- /dev/null
+++ b/js/src/jit-test/tests/debug/Frame-onStep-generators-gc-01.js
@@ -0,0 +1,84 @@
+// onStep hooks on suspended Frames can keep Debuggers alive, even chaining them.
+
+// The path through the heap we're building and testing here is:
+//     gen0 (generator object) -> frame1 (suspended Frame with .onStep) -> dbg1 (Debugger object)
+//       -> gen1 -> frame2 -> dbg2
+// where everything after `gen1` is otherwise unreachable, and the edges
+// `frame1 -> dbg1` and `frames2 -> dbg2` are due to the .onStep handlers, not
+// strong refrences.
+//
+// There is no easy way to thread an event through this whole path; when we
+// call gen0.next(), it will fire frame1.onStep(), but from there, making sure
+// gen1.next() is called requires some minor heroics (see the WeakMap below).
+
+var gen0;
+
+var hits2 = 0;
+var resuming2 = false;
+
+function onStep2() {
+    if (resuming2) {
+        hits2++;
+        resuming2 = false;
+    }
+}
+
+function setup() {
+    let g1 = newGlobal();
+    g1.eval(`
+        function* gf1() {
+             debugger;
+             yield 1;
+             return 'done';
+        }
+    `);
+    gen0 = g1.gf1();
+
+    let g2 = newGlobal();
+    g2.eval(`
+        function* gf2() { debugger; yield 1; return 'done'; }
+
+        var resuming1 = false;
+
+        function makeOnStepHook1(dbg1) {
+            // We use this WeakMap as a weak reference from frame1.onStep to dbg1.
+            var weak = new WeakMap();
+            weak.set(dbg1, {});
+            return () => {
+                if (resuming1) {
+                    var dbg1Arr = nondeterministicGetWeakMapKeys(weak);
+                    assertEq(dbg1Arr.length, 1);
+                    dbg1Arr[0].gen1.next();
+                    resuming1 = false;
+                }
+            };
+        }
+
+        function test(g1, gen0) {
+            let dbg1 = Debugger(g1);
+            dbg1.onDebuggerStatement = frame1 => {
+                frame1.onStep = makeOnStepHook1(dbg1);
+                dbg1.onDebuggerStatement = undefined;
+            };
+            gen0.next();  // run to yield point, creating frame1 and setting its onStep hook
+            resuming1 = true;
+            dbg1.gen1 = gf2();
+            return dbg1.gen1;
+        }
+    `);
+
+    let dbg2 = Debugger(g2);
+    dbg2.onDebuggerStatement = frame2 => {
+        frame2.onStep = onStep2;
+        dbg2.onDebuggerStatement = undefined;
+    };
+    var gen1 = g2.test(g1, gen0);
+    gen1.next();  // run to yield point, creating frame2 and setting its onStep hook
+    resuming2 = true;
+}
+
+setup();
+gc();
+assertEq(hits2, 0);
+gen0.next();  // fires frame1.onStep, which calls gen1.next(), which fires frame2.onStep
+assertEq(hits2, 1);
--- a/js/src/vm/Debugger.cpp
+++ b/js/src/vm/Debugger.cpp
@@ -942,21 +942,31 @@ Debugger::hasAnyLiveHooks(JSRuntime* rt)
           case BreakpointSite::Type::Wasm:
             if (IsMarkedUnbarriered(rt, &bp->asWasm()->wasmInstance)) {
                 return true;
             }
             break;
         }
     }
 
+    // Check for hooks in live stack frames.
     for (FrameMap::Range r = frames.all(); !r.empty(); r.popFront()) {
-        NativeObject* frameObj = r.front().value();
-        if (!frameObj->getReservedSlot(JSSLOT_DEBUGFRAME_ONSTEP_HANDLER).isUndefined() ||
-            !frameObj->getReservedSlot(JSSLOT_DEBUGFRAME_ONPOP_HANDLER).isUndefined())
+        DebuggerFrame& frameObj = r.front().value()->as<DebuggerFrame>();
+        if (frameObj.hasAnyLiveHooks()) {
             return true;
+        }
+    }
+
+    // Check for hooks set on suspended generator frames.
+    for (GeneratorWeakMap::Range r = generatorFrames.all(); !r.empty(); r.popFront()) {
+        JSObject* key = r.front().key();
+        DebuggerFrame& frameObj = r.front().value()->as<DebuggerFrame>();
+        if (IsMarkedUnbarriered(rt, &key) && frameObj.hasAnyLiveHooks()) {
+            return true;
+        }
     }
 
     return false;
 }
 
 /* static */ ResumeMode
 Debugger::slowPathOnEnterFrame(JSContext* cx, AbstractFramePtr frame)
 {
@@ -8490,16 +8500,23 @@ DebuggerFrame::resume(const FrameIter& i
     FrameIter::Data* data = iter.copyData();
     if (!data) {
         return false;
     }
     setPrivate(data);
     return true;
 }
 
+bool
+DebuggerFrame::hasAnyLiveHooks() const
+{
+    return !getReservedSlot(JSSLOT_DEBUGFRAME_ONSTEP_HANDLER).isUndefined() ||
+           !getReservedSlot(JSSLOT_DEBUGFRAME_ONPOP_HANDLER).isUndefined();
+}
+
 /* static */ NativeObject*
 DebuggerFrame::initClass(JSContext* cx, HandleObject dbgCtor, Handle<GlobalObject*> global)
 {
     RootedObject objProto(cx, GlobalObject::getOrCreateObjectPrototype(cx, global));
 
     return InitClass(cx, dbgCtor, objProto, &class_, construct, 0, properties_,
                      methods_, nullptr, nullptr);
 }
--- a/js/src/vm/Debugger.h
+++ b/js/src/vm/Debugger.h
@@ -1490,16 +1490,18 @@ class DebuggerFrame : public NativeObjec
     void setOnPopHandler(OnPopHandler* handler);
 
     /*
      * Called after a generator/async frame is resumed, before exposing this
      * Debugger.Frame object to any hooks.
      */
     bool resume(const FrameIter& iter);
 
+    bool hasAnyLiveHooks() const;
+
   private:
     static const ClassOps classOps_;
 
     static const JSPropertySpec properties_[];
     static const JSFunctionSpec methods_[];
 
     static AbstractFramePtr getReferent(HandleDebuggerFrame frame);
     static MOZ_MUST_USE bool getFrameIter(JSContext* cx, HandleDebuggerFrame frame,