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

@@ -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;
+  }
+}
+

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");

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();

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");

js/src/jit-test/tests/debug/Frame-onPop-async-01.js

@@ -24,6 +24,10 @@ dbg.onDebuggerStatement = frame => {
     g.log += 'A';
     drainJobQueue();
     g.log += 'B';
+
+    frame.onPop = completion => {
+      g.log += 'C';
+    };
   };
 };
 
@@ -31,5 +35,5 @@ 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");

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";
+});

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";
+});
+

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);

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<GlobalObject*> 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<Debugger*>* 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<AbstractGeneratorObject>();
       auto& frameObj = value->as<DebuggerFrame>();
@@ -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<JSScript*>& 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());
 }
 

js/src/vm/Debugger.h

@@ -571,9 +571,12 @@ class Debugger : private mozilla::LinkedListElement<Debugger> {
   class SourceQuery;
   class ObjectQuery;
 
+  enum class FromSweep { No, Yes };
+
   MOZ_MUST_USE bool addDebuggeeGlobal(JSContext* cx, Handle<GlobalObject*> obj);
   void removeDebuggeeGlobal(FreeOp* fop, GlobalObject* global,
-                            WeakGlobalObjectSet::Enum* debugEnum);
+                            WeakGlobalObjectSet::Enum* debugEnum,
+                            FromSweep fromSweep);
 
   enum class CallUncaughtExceptionHook { No, Yes };
 

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());

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<DebugScript, JS::FreePolicy>;
@@ -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;