Bug 1448880 - Part 6: Re-enable stepping when an async or generator frame with an .onStep hook is resumed. r=jimb
authorJason Orendorff <jorendorff@mozilla.com>
Tue, 23 Oct 2018 23:24:11 +0000
changeset 491042 6c8949f053e0e9c6216329d9b3f081fc0b6b2936
parent 491041 27b0121d9e314b26e2c11857b18409f5dabbd75b
child 491043 8ff4dac9916d9bd908611963de3ec8883befea5f
push id247
push userfmarier@mozilla.com
push dateSat, 27 Oct 2018 01:06:44 +0000
reviewersjimb
bugs1448880
milestone65.0a1
Bug 1448880 - Part 6: Re-enable stepping when an async or generator frame with an .onStep hook is resumed. r=jimb That is, don't put it off until Debugger::getFrame() is called. The effect is subtle, as indicated by the test changes: the onEnterFrame hooks in those tests were causing getFrame to be called very early during generator resumption, which made the tests pass. With this patch, we no longer adjust the step mode count when suspending or resuming. This change is necessary to make the frame->isDebuggee() call in Debugger::onResumeFrame the right criterion for calling slowPathOnResumeFrame. It's true if the step mode count on the script is nonzero. (This approach also simplifies error handling, as resuming a Debugger.Frame is now idempotent: we don't have to worry about adjusting the step mode count too much or not enough on error.) Depends on D6986 Differential Revision: https://phabricator.services.mozilla.com/D6988
js/src/jit-test/tests/debug/Frame-onStep-async-01.js
js/src/jit-test/tests/debug/Frame-onStep-generators-04.js
js/src/jit-test/tests/debug/onEnterFrame-generator-05.js
js/src/vm/Debugger.cpp
js/src/vm/Debugger.h
--- a/js/src/jit-test/tests/debug/Frame-onStep-async-01.js
+++ b/js/src/jit-test/tests/debug/Frame-onStep-async-01.js
@@ -20,15 +20,16 @@ dbg.onEnterFrame = frame => {
     frame.onStep = function () {
         assertEq(this, frame);
         let line = frame.script.getOffsetLocation(frame.offset).lineNumber;
         if (previousLine != line) {
             g.log += line; // We stepped to a new line.
             previousLine = line;
         }
     };
+    dbg.onEnterFrame = undefined;
 };
 
 // Run.
 g.aloop();
 drainJobQueue();
 
 assertEq(g.log, "2345 345 345 37^");
--- a/js/src/jit-test/tests/debug/Frame-onStep-generators-04.js
+++ b/js/src/jit-test/tests/debug/Frame-onStep-generators-04.js
@@ -20,16 +20,17 @@ dbg.onEnterFrame = frame => {
     frame.onStep = function () {
         assertEq(this, frame);
         let line = frame.script.getOffsetLocation(frame.offset).lineNumber;
         if (previousLine != line) {
             g.log += line; // We stepped to a new line.
             previousLine = line;
         }
     };
+    dbg.onEnterFrame = undefined;
 };
 
 // Run.
 for (let value of g.range(3)) {
     g.log += "*";
 }
 
 assertEq(g.log, "234*5 34*5 34*5 37^");
new file mode 100644
--- /dev/null
+++ b/js/src/jit-test/tests/debug/onEnterFrame-generator-05.js
@@ -0,0 +1,32 @@
+// When resuming a generator triggers one Debugger's onEnterFrame handler,
+// all Debuggers' Debugger.Frames become usable at once.
+
+let g = newGlobal();
+g.eval(`
+    function* f() {
+       yield 1;
+    }
+`);
+let dbg1 = new Debugger(g);
+let dbg2 = new Debugger(g);
+
+let hits = 0;
+let savedFrame1;
+let savedFrame2;
+dbg1.onEnterFrame = frame => {
+    if (savedFrame1 === undefined) {
+        savedFrame1 = frame;
+        savedFrame2 = dbg2.getNewestFrame();
+    } else {
+        hits++;
+        assertEq(savedFrame1, frame);
+        for (let f of [savedFrame2, savedFrame1]) {
+            assertEq(f.type, "call");
+            assertEq(f.callee.name, "f");
+        }
+    }
+};
+
+let values = [...g.f()];
+assertEq(hits, 2);
+assertEq(values.toSource(), "[1]");
--- a/js/src/vm/Debugger.cpp
+++ b/js/src/vm/Debugger.cpp
@@ -801,50 +801,39 @@ Debugger::getFrame(JSContext* cx, const 
     }
 
     FrameMap::AddPtr p = frames.lookupForAdd(referent);
     if (!p) {
         RootedDebuggerFrame frame(cx);
 
         // If this is a generator frame, there may be an existing
         // Debugger.Frame object that isn't in `frames` because the generator
-        // was suspended, popping the stack frame, and later resumed.
+        // was suspended, popping the stack frame, and later resumed (and we
+        // were not stepping, so did not pass through slowPathOnResumeFrame).
         Rooted<GeneratorObject*> genObj(cx);
         GeneratorWeakMap::AddPtr gp;
         if (referent.isGeneratorFrame()) {
             {
                 AutoRealm ar(cx, referent.callee());
                 genObj = GetGeneratorObjectForFrame(cx, referent);
             }
             if (genObj) {
                 gp = generatorFrames.lookupForAdd(genObj);
                 if (gp) {
                     frame = &gp->value()->as<DebuggerFrame>();
 
                     // We have found an existing Debugger.Frame object. But
                     // since it was previously popped (see comment above), it
                     // is not currently "live". We must revive it.
-                    MOZ_ASSERT(!frame->isLive());
-                    FrameIter::Data* data = iter.copyData();
-                    if (!data) {
+                    if (!frame->resume(iter)) {
                         return false;
                     }
-                    frame->setPrivate(data);
-
                     if (!ensureExecutionObservabilityOfFrame(cx, referent)) {
                         return false;
                     }
-
-                    RootedValue onStep(cx, frame->getReservedSlot(JSSLOT_DEBUGFRAME_ONSTEP_HANDLER));
-                    if (!onStep.isUndefined()) {
-                        AutoRealm ar(cx, genObj);
-                        if (!referent.script()->incrementStepModeCount(cx)) {
-                            return false;
-                        }
-                    }
                 }
             }
 
             // If no GeneratorObject exists yet, we create a Debugger.Frame
             // below anyway, and Debugger::onNewGenerator() will associate it
             // with the GeneratorObject later when we hit JSOP_GENERATOR.
         }
 
@@ -1005,16 +994,46 @@ Debugger::slowPathOnEnterFrame(JSContext
 }
 
 /* static */ ResumeMode
 Debugger::slowPathOnResumeFrame(JSContext* cx, AbstractFramePtr frame)
 {
     // Don't count on this method to be called every time a generator is
     // resumed! This is called only if the frame's debuggee bit is set,
     // i.e. the script has breakpoints or the frame is stepping.
+    MOZ_ASSERT(frame.isGeneratorFrame());
+    MOZ_ASSERT(frame.isDebuggee());
+
+    Rooted<GeneratorObject*> genObj(cx, GetGeneratorObjectForFrame(cx, frame));
+    MOZ_ASSERT(genObj);
+
+    // For each debugger, if there is an existing Debugger.Frame object for the
+    // resumed `frame`, update it with the new frame pointer and make sure the
+    // frame is observable.
+    if (GlobalObject::DebuggerVector* debuggers = frame.global()->getDebuggers()) {
+        for (Debugger* dbg : *debuggers) {
+            if (GeneratorWeakMap::Ptr entry = dbg->generatorFrames.lookup(genObj)) {
+                DebuggerFrame* frameObj = &entry->value()->as<DebuggerFrame>();
+                if (!dbg->frames.putNew(frame, frameObj)) {
+                    ReportOutOfMemory(cx);
+                    return ResumeMode::Throw;
+                }
+
+                FrameIter iter(cx);
+                MOZ_ASSERT(iter.abstractFramePtr() == frame);
+                if (!frameObj->resume(iter)) {
+                    return ResumeMode::Throw;
+                }
+                if (!ensureExecutionObservabilityOfFrame(cx, frame)) {
+                    return ResumeMode::Throw;
+                }
+            }
+        }
+    }
+
     return slowPathOnEnterFrame(cx, frame);
 }
 
 
 static void
 DebuggerFrame_maybeDecrementFrameScriptStepModeCount(FreeOp* fop, AbstractFramePtr frame,
                                                      NativeObject* frameobj);
 
@@ -7299,23 +7318,32 @@ Debugger::inFrameMaps(AbstractFramePtr f
     return foundAny;
 }
 
 /* static */ void
 Debugger::removeFromFrameMapsAndClearBreakpointsIn(JSContext* cx, AbstractFramePtr frame,
                                                    bool suspending)
 {
     forEachDebuggerFrame(frame, [&](DebuggerFrame* frameobj) {
-        Debugger* dbg = Debugger::fromChildJSObject(frameobj);
-
         FreeOp* fop = cx->runtime()->defaultFreeOp();
         frameobj->freeFrameIterData(fop);
-        DebuggerFrame_maybeDecrementFrameScriptStepModeCount(fop, frame, frameobj);
-
+        if (!suspending) {
+            DebuggerFrame_maybeDecrementFrameScriptStepModeCount(fop, frame, frameobj);
+        }
+
+        Debugger* dbg = Debugger::fromChildJSObject(frameobj);
         dbg->frames.remove(frame);
+
+        if (!suspending && frame.isGeneratorFrame()) {
+            // Terminally exiting a generator.
+            GeneratorObject* genObj = GetGeneratorObjectForFrame(cx, frame);
+            if (GeneratorWeakMap::Ptr p = dbg->generatorFrames.lookup(genObj)) {
+                dbg->generatorFrames.remove(p);
+            }
+        }
     });
 
     // If this is an eval frame, then from the debugger's perspective the
     // script is about to be destroyed. Remove any breakpoints in it.
     if (frame.isEvalFrame()) {
         RootedScript script(cx, frame.script());
         script->clearBreakpointsIn(cx->runtime()->defaultFreeOp(), nullptr, nullptr);
     }
@@ -8451,16 +8479,27 @@ ScriptedOnPopHandler::onPop(JSContext* c
     RootedValue rval(cx);
     if (!js::Call(cx, fval, frame, completion, &rval)) {
         return false;
     }
 
     return ParseResumptionValue(cx, rval, resumeMode, vp);
 };
 
+bool
+DebuggerFrame::resume(const FrameIter& iter)
+{
+    FrameIter::Data* data = iter.copyData();
+    if (!data) {
+        return false;
+    }
+    setPrivate(data);
+    return true;
+}
+
 /* 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
@@ -158,16 +158,17 @@ class DebuggerWeakMap : private WeakMap<
     typedef typename Base::Ptr Ptr;
     typedef typename Base::AddPtr AddPtr;
     typedef typename Base::Range Range;
     typedef typename Base::Enum Enum;
     typedef typename Base::Lookup Lookup;
 
     /* Expose WeakMap public interface */
 
+    using Base::lookup;
     using Base::lookupForAdd;
     using Base::remove;
     using Base::all;
     using Base::trace;
 
     template<typename KeyInput, typename ValueInput>
     bool relookupOrAdd(AddPtr& p, const KeyInput& k, const ValueInput& v) {
         MOZ_ASSERT(v->compartment() == this->compartment);
@@ -1483,16 +1484,22 @@ class DebuggerFrame : public NativeObjec
                                   const EvalOptions& options, ResumeMode& resumeMode,
                                   MutableHandleValue value);
 
     bool isLive() const;
     OnStepHandler* onStepHandler() const;
     OnPopHandler* onPopHandler() const;
     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);
+
   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,