From f6fc9fdcc0700518eab6f8671724c84884c5ff62 Mon Sep 17 00:00:00 2001 From: Jim Blandy Date: Mon, 10 Jun 2019 20:06:34 +0000 Subject: [PATCH] Bug 1539654: Ensure generator scripts observed by Debugger.Frames are marked as debuggees. r=jorendorff When a Debugger.Frame refers to a generator/async call, the generator's script must be marked as a debuggee, so that if the call is resumed, the JSOP_AFTERYIELD bytecode is compiled to the instrumentation that re-associates the extant Debugger.Frame with the new concrete frame. This extends DebugScript with a new field `generatorObserverCount`, akin to `stepperCount`, which tracks the number of Debugger.Frames referring to the script if it is a generator script. This count is adjusted when the `GeneratorInfo` structure is attached to a `DebuggerFrame`, cleared, and finalized. Differential Revision: https://phabricator.services.mozilla.com/D32394 --HG-- extra : moz-landing-system : lando --- js/src/jit-test/lib/match-debugger.js | 20 ++++++++ .../tests/debug/Debugger-debuggees-31.js | 26 ++++++++++ .../tests/debug/Debugger-debuggees-32.js | 13 +++++ js/src/jit-test/tests/debug/Frame-onPop-15.js | 2 +- .../tests/debug/Frame-onPop-async-01.js | 32 +++++++----- .../tests/debug/Frame-onPop-async-02.js | 36 +++++++++++++ .../debug/Frame-onPop-async-generators-01.js | 51 +++++++++++++++++++ .../tests/debug/Frame-onPop-generators-06.js | 28 ++++++++++ js/src/vm/Debugger.cpp | 49 ++++++++++++------ js/src/vm/Debugger.h | 5 +- js/src/vm/JSScript.cpp | 34 +++++++++++++ js/src/vm/JSScript.h | 25 ++++++++- 12 files changed, 289 insertions(+), 32 deletions(-) create mode 100644 js/src/jit-test/lib/match-debugger.js create mode 100644 js/src/jit-test/tests/debug/Debugger-debuggees-31.js create mode 100644 js/src/jit-test/tests/debug/Debugger-debuggees-32.js create mode 100644 js/src/jit-test/tests/debug/Frame-onPop-async-02.js create mode 100644 js/src/jit-test/tests/debug/Frame-onPop-async-generators-01.js create mode 100644 js/src/jit-test/tests/debug/Frame-onPop-generators-06.js diff --git a/js/src/jit-test/lib/match-debugger.js b/js/src/jit-test/lib/match-debugger.js new file mode 100644 index 000000000000..431279c4af26 --- /dev/null +++ b/js/src/jit-test/lib/match-debugger.js @@ -0,0 +1,20 @@ +// Debugger-oriented Pattern subclasses. + +if (typeof Match !== 'function') { + load(libdir + 'match.js'); +} + +class DebuggerObjectPattern extends Match.Pattern { + constructor(className) { + super(); + this.className = className; + } + + match(actual) { + if (!(actual instanceof Debugger.Object) || actual.class !== this.className) { + throw new Match.MatchError(`Expected Debugger.Object of class ${this.className}, got ${actual}`); + } + return true; + } +} + diff --git a/js/src/jit-test/tests/debug/Debugger-debuggees-31.js b/js/src/jit-test/tests/debug/Debugger-debuggees-31.js new file mode 100644 index 000000000000..efeca7f87bbe --- /dev/null +++ b/js/src/jit-test/tests/debug/Debugger-debuggees-31.js @@ -0,0 +1,26 @@ +// |jit-test| skip-if: !('gcstate' in this) +// Removing and adding debuggees during an incremental sweep should not confuse +// the generatorFrames map. + +// Let any ongoing incremental GC finish, and then clear any ambient zeal +// settings established by the harness (the JS_GC_ZEAL env var, shell arguments, +// etc.) +gczeal(0); + +let g = newGlobal({newCompartment: true}); +g.eval('function* f() { yield 123; }'); + +let dbg = Debugger(g); +dbg.onEnterFrame = frame => { + dbg.removeDebuggee(g); + dbg.addDebuggee(g); +}; + +// Select GCZeal mode 10 (IncrementalMultipleSlices: Incremental GC in many +// slices) and use long slices, to make sure that the debuggee removal occurs +// during a slice. +gczeal(10, 0); +gcslice(1000000); +let genObj = g.f(); +genObj.return(); +assertEq(gcstate(), "Sweep"); diff --git a/js/src/jit-test/tests/debug/Debugger-debuggees-32.js b/js/src/jit-test/tests/debug/Debugger-debuggees-32.js new file mode 100644 index 000000000000..4d5bab9e1ad8 --- /dev/null +++ b/js/src/jit-test/tests/debug/Debugger-debuggees-32.js @@ -0,0 +1,13 @@ +let g = newGlobal({newCompartment: true}); +g.eval('function* f() { yield 123; }'); + +let dbg = Debugger(g); +let genObj = g.f(); + +dbg.onEnterFrame = frame => { + frame.onStep = () => {}; + dbg.removeDebuggee(g); + dbg.addDebuggee(g); +}; + +genObj.return(); diff --git a/js/src/jit-test/tests/debug/Frame-onPop-15.js b/js/src/jit-test/tests/debug/Frame-onPop-15.js index 85e1c06d4cad..6b76723cb2b3 100644 --- a/js/src/jit-test/tests/debug/Frame-onPop-15.js +++ b/js/src/jit-test/tests/debug/Frame-onPop-15.js @@ -28,4 +28,4 @@ dbg.onDebuggerStatement = function handleDebugger(frame) { g.eval("function* g() { for (var i = 0; i < 10; i++) { debugger; yield i; } }"); log =''; assertEq(g.eval("var t = 0; for (j of g()) t += j; t;"), 45); -assertEq(log, "d)0d)0d)0d)3d)3d)3d)6d)6d)6d)9"); +assertEq(log, "d)0d)0d)0d)3d)3d)3d)6d)6d)6d)9)9"); diff --git a/js/src/jit-test/tests/debug/Frame-onPop-async-01.js b/js/src/jit-test/tests/debug/Frame-onPop-async-01.js index 9d4191a366fc..8d491f3f7fb4 100644 --- a/js/src/jit-test/tests/debug/Frame-onPop-async-01.js +++ b/js/src/jit-test/tests/debug/Frame-onPop-async-01.js @@ -6,30 +6,34 @@ let g = newGlobal({newCompartment: true}); g.log = ""; g.eval(` - async function f() { - log += "1"; - debugger; - log += "2"; - await Promise.resolve(3); - log += "3"; - return "ok"; - } + async function f() { + log += "1"; + debugger; + log += "2"; + await Promise.resolve(3); + log += "3"; + return "ok"; + } `); let dbg = Debugger(g); dbg.onDebuggerStatement = frame => { + frame.onPop = completion => { + // What we are really testing is that when onPop is called, we have not + // yet thrown this async function activation back into the hopper. + g.log += 'A'; + drainJobQueue(); + g.log += 'B'; + frame.onPop = completion => { - // What we are really testing is that when onPop is called, we have not - // yet thrown this async function activation back into the hopper. - g.log += 'A'; - drainJobQueue(); - g.log += 'B'; + g.log += 'C'; }; + }; }; let status = "FAIL - g.f() did not resolve"; g.f().then(value => { status = value; }); assertEq(g.log, "12AB"); drainJobQueue(); -assertEq(g.log, "12AB3"); +assertEq(g.log, "12AB3C"); assertEq(status, "ok"); diff --git a/js/src/jit-test/tests/debug/Frame-onPop-async-02.js b/js/src/jit-test/tests/debug/Frame-onPop-async-02.js new file mode 100644 index 000000000000..8e579b588b6b --- /dev/null +++ b/js/src/jit-test/tests/debug/Frame-onPop-async-02.js @@ -0,0 +1,36 @@ +// |jit-test| error:all-jobs-completed-successfully + +load(libdir + 'match.js'); +load(libdir + 'match-debugger.js'); +const { Pattern } = Match; +const { OBJECT_WITH_EXACTLY: EXACT } = Pattern; + +let g = newGlobal({newCompartment: true}); +let dbg = Debugger(g); +const log = []; +g.capture = function () { + dbg.getNewestFrame().onPop = completion => { + log.push(completion); + }; +}; + +g.eval(` + async function f() { + capture(); + await Promise.resolve(3); + return "ok"; + } +`); + + +const promise = g.f(); +promise.then(value => { + assertEq(value, "ok"); + + Pattern([ + EXACT({ return: new DebuggerObjectPattern("Promise"), await:true }), + EXACT({ return: new DebuggerObjectPattern("Promise") }), + ]).assert(log); + + throw "all-jobs-completed-successfully"; +}); diff --git a/js/src/jit-test/tests/debug/Frame-onPop-async-generators-01.js b/js/src/jit-test/tests/debug/Frame-onPop-async-generators-01.js new file mode 100644 index 000000000000..322ce88f7a14 --- /dev/null +++ b/js/src/jit-test/tests/debug/Frame-onPop-async-generators-01.js @@ -0,0 +1,51 @@ +// |jit-test| error:all-jobs-completed-successfully + +load(libdir + "asserts.js"); +load(libdir + 'match.js'); +load(libdir + 'match-debugger.js'); +const { Pattern } = Match; +const { OBJECT_WITH_EXACTLY: EXACT } = Pattern; + +let g = newGlobal({newCompartment: true}); +let dbg = Debugger(g); +const log = []; +g.capture = function () { + dbg.getNewestFrame().onPop = completion => { + log.push(completion); + }; +}; + +g.eval(` + async function* asyncgen() { + capture(); + await Promise.resolve(1); + yield 2; + await Promise.resolve(3); + yield 4; + return "ok"; + } +`); + +async function collect() { + let items = []; + for await (let item of g.asyncgen()) { + items.push(item); + } + return items; +} + +collect().then(value => { + assertDeepEq(value, [2, 4]); + + Pattern([ + EXACT({ return: new DebuggerObjectPattern("Promise") }), + EXACT({ return: 2 }), + EXACT({ return: new DebuggerObjectPattern("Promise") }), + EXACT({ return: 4 }), + EXACT({ return: "ok" }), + EXACT({ return: "ok" }), + ]).assert(log); + + throw "all-jobs-completed-successfully"; +}); + diff --git a/js/src/jit-test/tests/debug/Frame-onPop-generators-06.js b/js/src/jit-test/tests/debug/Frame-onPop-generators-06.js new file mode 100644 index 000000000000..6ada610b1332 --- /dev/null +++ b/js/src/jit-test/tests/debug/Frame-onPop-generators-06.js @@ -0,0 +1,28 @@ +load(libdir + "asserts.js"); +load(libdir + 'match.js'); +load(libdir + 'match-debugger.js'); +const { Pattern } = Match; +const { OBJECT_WITH_EXACTLY: EXACT } = Pattern; + +let g = newGlobal({newCompartment: true}); +let dbg = Debugger(g); +const log = []; +g.capture = function () { + dbg.getNewestFrame().onPop = completion => { + log.push(completion); + }; +}; + +g.eval(` + function* f() { + capture(); + yield 3; + return "ok"; + } +`); + +assertDeepEq([... g.f()], [3]); +Pattern([ + EXACT({ return: new DebuggerObjectPattern("Object") }), + EXACT({ return: new DebuggerObjectPattern("Object") }), +]).assert(log); diff --git a/js/src/vm/Debugger.cpp b/js/src/vm/Debugger.cpp index dbc2b6eeaff1..d1c286b95f3b 100644 --- a/js/src/vm/Debugger.cpp +++ b/js/src/vm/Debugger.cpp @@ -3587,7 +3587,8 @@ void Debugger::sweepAll(FreeOp* fop) { e.popFront()) { GlobalObject* global = e.front().unbarrieredGet(); if (debuggerDying || IsAboutToBeFinalizedUnbarriered(&global)) { - dbg->removeDebuggeeGlobal(fop, e.front().unbarrieredGet(), &e); + dbg->removeDebuggeeGlobal(fop, e.front().unbarrieredGet(), &e, + FromSweep::Yes); } } @@ -3604,7 +3605,8 @@ void Debugger::detachAllDebuggersFromGlobal(FreeOp* fop, GlobalObject* global) { const GlobalObject::DebuggerVector* debuggers = global->getDebuggers(); MOZ_ASSERT(!debuggers->empty()); while (!debuggers->empty()) { - debuggers->back()->removeDebuggeeGlobal(fop, global, nullptr); + debuggers->back()->removeDebuggeeGlobal(fop, global, nullptr, + FromSweep::No); } } @@ -4098,7 +4100,8 @@ bool Debugger::removeDebuggee(JSContext* cx, unsigned argc, Value* vp) { ExecutionObservableRealms obs(cx); if (dbg->debuggees.has(global)) { - dbg->removeDebuggeeGlobal(cx->runtime()->defaultFreeOp(), global, nullptr); + dbg->removeDebuggeeGlobal(cx->runtime()->defaultFreeOp(), global, nullptr, + FromSweep::No); // Only update the realm if there are no Debuggers left, as it's // expensive to check if no other Debugger has a live script or frame @@ -4123,7 +4126,8 @@ bool Debugger::removeAllDebuggees(JSContext* cx, unsigned argc, Value* vp) { for (WeakGlobalObjectSet::Enum e(dbg->debuggees); !e.empty(); e.popFront()) { Rooted global(cx, e.front()); - dbg->removeDebuggeeGlobal(cx->runtime()->defaultFreeOp(), global, &e); + dbg->removeDebuggeeGlobal(cx->runtime()->defaultFreeOp(), global, &e, + FromSweep::No); // See note about adding to the observable set in removeDebuggee. if (global->getDebuggers()->empty() && !obs.add(global->realm())) { @@ -4475,7 +4479,8 @@ static WeakHeapPtr* findDebuggerInVector( } void Debugger::removeDebuggeeGlobal(FreeOp* fop, GlobalObject* global, - WeakGlobalObjectSet::Enum* debugEnum) { + WeakGlobalObjectSet::Enum* debugEnum, + FromSweep fromSweep) { // The caller might have found global by enumerating this->debuggees; if // so, use HashSet::Enum::removeFront rather than HashSet::remove below, // to avoid invalidating the live enumerator. @@ -4502,13 +4507,17 @@ void Debugger::removeDebuggeeGlobal(FreeOp* fop, GlobalObject* global, // Clear this global's generators from generatorFrames as well. // - // This method can be called either from script (dbg.removeDebuggee) or - // from an awkward time during GC sweeping. In the latter case, skip this - // loop to avoid touching dead objects. It's correct because, when we're - // called from GC, all `global`'s generators are guaranteed to be dying: - // live generators would keep the global alive and we wouldn't be here. GC - // will sweep dead keys from the weakmap. - if (!global->zone()->isGCSweeping()) { + // This method can be called either from script (dbg.removeDebuggee) or during + // GC sweeping, because the Debugger, debuggee global, or both are being GC'd. + // + // When called from script, it's okay to iterate over generatorFrames and + // touch its keys and values (even when an incremental GC is in progress). + // When called from GC, it's not okay; the keys and values may be dying. But + // in that case, we can actually just skip the loop entirely! If the Debugger + // is going away, it doesn't care about the state of its generatorFrames + // table, and the Debugger.Frame finalizer will fix up the generator observer + // counts. + if (fromSweep == FromSweep::No) { generatorFrames.removeIf([global, fop](JSObject* key, JSObject* value) { auto& genObj = key->as(); auto& frameObj = value->as(); @@ -9092,6 +9101,14 @@ bool DebuggerFrame::setGenerator( return false; } + { + AutoRealm ar(cx, script); + if (!script->incrementGeneratorObserverCount(cx)) { + js_delete(info); + return false; + } + } + setReservedSlot(GENERATOR_INFO_SLOT, PrivateValue(info)); return true; } @@ -9102,14 +9119,16 @@ void DebuggerFrame::clearGenerator(FreeOp* fop) { } GeneratorInfo* info = generatorInfo(); - if (!IsAboutToBeFinalized(&info->generatorScript())) { + HeapPtr& generatorScript = info->generatorScript(); + if (!IsAboutToBeFinalized(&generatorScript)) { + generatorScript->decrementGeneratorObserverCount(fop); bool isStepper = !getReservedSlot(ONSTEP_HANDLER_SLOT).isUndefined(); if (isStepper) { - info->generatorScript()->decrementStepperCount(fop); + generatorScript->decrementStepperCount(fop); } } - fop->delete_(generatorInfo()); + fop->delete_(info); setReservedSlot(GENERATOR_INFO_SLOT, UndefinedValue()); } diff --git a/js/src/vm/Debugger.h b/js/src/vm/Debugger.h index b16ae3483161..67e4350acd38 100644 --- a/js/src/vm/Debugger.h +++ b/js/src/vm/Debugger.h @@ -571,9 +571,12 @@ class Debugger : private mozilla::LinkedListElement { class SourceQuery; class ObjectQuery; + enum class FromSweep { No, Yes }; + MOZ_MUST_USE bool addDebuggeeGlobal(JSContext* cx, Handle obj); void removeDebuggeeGlobal(FreeOp* fop, GlobalObject* global, - WeakGlobalObjectSet::Enum* debugEnum); + WeakGlobalObjectSet::Enum* debugEnum, + FromSweep fromSweep); enum class CallUncaughtExceptionHook { No, Yes }; diff --git a/js/src/vm/JSScript.cpp b/js/src/vm/JSScript.cpp index 6bc1db9b2d6e..18a09c1886c1 100644 --- a/js/src/vm/JSScript.cpp +++ b/js/src/vm/JSScript.cpp @@ -4732,6 +4732,40 @@ DebugScript* JSScript::getOrCreateDebugScript(JSContext* cx) { return borrowed; } +bool JSScript::incrementGeneratorObserverCount(JSContext* cx) { + cx->check(this); + MOZ_ASSERT(cx->realm()->isDebuggee()); + + AutoRealm ar(cx, this); + + DebugScript* debug = getOrCreateDebugScript(cx); + if (!debug) { + return false; + } + + debug->generatorObserverCount++; + + // It is our caller's responsibility, before bumping the generator observer + // count, to make sure that the baseline code includes the necessary + // JS_AFTERYIELD instrumentation by calling + // {ensure,update}ExecutionObservabilityOfScript. + MOZ_ASSERT_IF(hasBaselineScript(), baseline->hasDebugInstrumentation()); + + return true; +} + +void JSScript::decrementGeneratorObserverCount(js::FreeOp* fop) { + DebugScript* debug = debugScript(); + MOZ_ASSERT(debug); + MOZ_ASSERT(debug->generatorObserverCount > 0); + + debug->generatorObserverCount--; + + if (!debug->needed()) { + fop->free_(releaseDebugScript()); + } +} + bool JSScript::incrementStepperCount(JSContext* cx) { cx->check(this); MOZ_ASSERT(cx->realm()->isDebuggee()); diff --git a/js/src/vm/JSScript.h b/js/src/vm/JSScript.h index f3ee60d8cd07..86d04c898a20 100644 --- a/js/src/vm/JSScript.h +++ b/js/src/vm/JSScript.h @@ -256,6 +256,18 @@ class DebugScript { friend class ::JSScript; friend class JS::Realm; + /* + * If this is a generator script, this is the number of Debugger.Frames + * referring to calls to this generator, whether live or suspended. Closed + * generators do not contribute a count. + * + * When greater than zero, this script should be compiled with debug + * instrumentation to call Debugger::onResumeFrame at each resumption site, so + * that Debugger can reconnect any extant Debugger.Frames with the new + * concrete frame. + */ + uint32_t generatorObserverCount; + /* * The number of Debugger.Frame objects that refer to frames running this * script and that have onStep handlers. When nonzero, the interpreter and JIT @@ -282,7 +294,9 @@ class DebugScript { * True if this DebugScript carries any useful information. If false, it * should be removed from its JSScript. */ - bool needed() const { return stepperCount > 0 || numSites > 0; } + bool needed() const { + return generatorObserverCount > 0 || stepperCount > 0 || numSites > 0; + } }; using UniqueDebugScript = js::UniquePtr; @@ -2932,6 +2946,15 @@ class JSScript : public js::gc::TenuredCell { } #endif + /* + * Increment or decrement the generator observer count. If the count is + * non-zero then the script reports resumptions to the debugger. + * + * Only incrementing is fallible, as it could allocate a DebugScript. + */ + bool incrementGeneratorObserverCount(JSContext* cx); + void decrementGeneratorObserverCount(js::FreeOp* fop); + void finalize(js::FreeOp* fop); static const JS::TraceKind TraceKind = JS::TraceKind::Script;