Bug 1501666: Include suspended generators in count of Debugger.Frames with onStep handlers. r=jorendorff
authorJim Blandy <jimb@mozilla.com>
Wed, 08 May 2019 06:14:24 +0000
changeset 534893 b7ce8f5ea1689a9ac21cb34a39bae369a9cfe0df
parent 534892 8bde97e9f598f2bea1c0182de7d0b7f52126c933
child 534894 8e38a89a6d652b199a048628d7d1aa633ae930bd
push id2082
push userffxbld-merge
push dateMon, 01 Jul 2019 08:34:18 +0000
treeherdermozilla-release@2fb19d0466d2 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjorendorff
bugs1501666
milestone68.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 1501666: Include suspended generators in count of Debugger.Frames with onStep handlers. r=jorendorff Debugger::onSingleStep asserts that the script's step mode count is fully accounted for by Debugger.Frame instances with onStep handlers. The existing assertion only takes into account live Debugger.Frames --- it simply looks up the current frame's AbstractFramePtr in existing Debuggers' frame maps. But Debugger.Frames referring to suspended generator calls also contribute to the generator's script's step mode count, and need to be included. Differential Revision: https://phabricator.services.mozilla.com/D27846
js/src/jit-test/tests/debug/Frame-onStep-async-03.js
js/src/vm/Debugger.cpp
js/src/vm/GeneratorObject.h
new file mode 100644
--- /dev/null
+++ b/js/src/jit-test/tests/debug/Frame-onStep-async-03.js
@@ -0,0 +1,18 @@
+// Bug 1501666: assertions about the script's step mode count must take
+// suspended calls into account. This should not crash.
+
+var g = newGlobal({ newCompartment: true });
+g.eval(`
+    async function f(y) {
+        await true;
+        await true;
+    };
+`);
+
+g.f();
+g.f();
+
+var dbg = Debugger(g);
+dbg.onEnterFrame = function(frame) {
+    frame.onStep = function() {}
+}
--- a/js/src/vm/Debugger.cpp
+++ b/js/src/vm/Debugger.cpp
@@ -2360,37 +2360,84 @@ ResumeMode Debugger::onSingleStep(JSCont
   // Validate the single-step count on this frame's script, to ensure that
   // we're not receiving traps we didn't ask for. Even when frames is
   // non-empty (and thus we know this trap was requested), do the check
   // anyway, to make sure the count has the correct non-zero value.
   //
   // The converse --- ensuring that we do receive traps when we should --- can
   // be done with unit tests.
   if (iter.hasScript()) {
-    uint32_t stepperCount = 0;
+    uint32_t liveStepperCount = 0;
+    uint32_t suspendedStepperCount = 0;
     JSScript* trappingScript = iter.script();
     GlobalObject* global = cx->global();
     if (GlobalObject::DebuggerVector* debuggers = global->getDebuggers()) {
       for (auto p = debuggers->begin(); p != debuggers->end(); p++) {
         Debugger* dbg = *p;
         for (FrameMap::Range r = dbg->frames.all(); !r.empty(); r.popFront()) {
           AbstractFramePtr frame = r.front().key();
           NativeObject* frameobj = r.front().value();
           if (frame.isWasmDebugFrame()) {
             continue;
           }
           if (frame.script() == trappingScript &&
               !frameobj->getReservedSlot(JSSLOT_DEBUGFRAME_ONSTEP_HANDLER)
                    .isUndefined()) {
-            stepperCount++;
+            liveStepperCount++;
           }
         }
-      }
-    }
-    MOZ_ASSERT(stepperCount == trappingScript->stepModeCount());
+
+        // Also count hooks set on suspended generator frames.
+        for (GeneratorWeakMap::Range r = dbg->generatorFrames.all(); !r.empty();
+             r.popFront()) {
+          AbstractGeneratorObject& genObj =
+              r.front().key()->as<AbstractGeneratorObject>();
+          DebuggerFrame& frameObj = r.front().value()->as<DebuggerFrame>();
+
+          // Live Debugger.Frames were already counted in dbg->frames loop.
+          if (frameObj.isLive()) {
+            continue;
+          }
+
+          // It is possible to have entries in generatorFrames that are not
+          // live, but also not suspended:
+          //
+          // In a generator script prologue, between the 'GENERATOR'
+          // instruction, which creates the generator object, and the
+          // 'SETALIASEDVAR .generator' instruction, which stores it in its
+          // designated spot in the frame, we are in an odd state. 'GENERATOR'
+          // has called onNewGenerator, adding an entry to generatorFrames
+          // mapping the generator object to its Debugger.Frame; but
+          // GetGeneratorObjectForFrame cannot yet find that generator object
+          // given a frame pointer. This means that if Debugger forces a return
+          // between those two instructions, slowPathOnLeaveFrame cannot find
+          // the generator object in order to clean up its generatorFramse
+          // entry.
+          //
+          // When this has occurred, the table entry will get cleaned up once
+          // the generator object gets GC'd, which should be soon since nobody
+          // got a chance to look at it. But it does mean that we need to verify
+          // that the generator is actually suspended before we attribute a step
+          // count to it.
+          if (!genObj.isSuspended()) {
+            continue;
+          }
+
+          if (!genObj.callee().isInterpretedLazy() &&
+              genObj.callee().nonLazyScript() == trappingScript &&
+              !frameObj.getReservedSlot(JSSLOT_DEBUGFRAME_ONSTEP_HANDLER)
+                   .isUndefined()) {
+            suspendedStepperCount++;
+          }
+        }
+      }
+    }
+
+    MOZ_ASSERT(liveStepperCount + suspendedStepperCount ==
+               trappingScript->stepModeCount());
   }
 #endif
 
   if (frames.length() > 0) {
     // Preserve the debuggee's microtask event queue while we run the hooks, so
     // the debugger's microtask checkpoints don't run from the debuggee's
     // microtasks, and vice versa.
     JS::AutoDebuggerJobQueueInterruption adjqi;
--- a/js/src/vm/GeneratorObject.h
+++ b/js/src/vm/GeneratorObject.h
@@ -200,18 +200,27 @@ class GeneratorObject : public AbstractG
 };
 
 bool GeneratorThrowOrReturn(JSContext* cx, AbstractFramePtr frame,
                             Handle<AbstractGeneratorObject*> obj,
                             HandleValue val, GeneratorResumeKind resumeKind);
 
 /**
  * Return the generator object associated with the given frame. The frame must
- * be a call frame for a generator. If the generator object hasn't been created
- * yet, or hasn't been stored in the stack slot yet, this returns null.
+ * be a call frame for a generator.
+ *
+ * This may return nullptr at certain points in the generator lifecycle:
+ *
+ * - While a generator call evaluates default argument values and performs
+ *   destructuring, which occurs before the generator object is created.
+ *
+ * - Between the `GENERATOR` instruction and the `SETALIASEDVAR .generator`
+ *   instruction, at which point the generator object does exist, but is held
+ *   only on the stack, and not the `.generator` pseudo-variable this function
+ *   consults.
  */
 AbstractGeneratorObject* GetGeneratorObjectForFrame(JSContext* cx,
                                                     AbstractFramePtr frame);
 
 void SetGeneratorClosed(JSContext* cx, AbstractFramePtr frame);
 
 }  // namespace js