Bug 1479391 - Fix crash after forced return from a generator. r=jimb

This patch switches from GeneratorObject::finalSuspend to setClosed in order to
dodge an assertion in finalSuspend that the Generator state machine is
transitioning along an expected edge. The way the Debugger manipulates
Generator state is decidedly unexpected, from the perspective of the normal
rules, and we've decided to accept that.

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

@@ -0,0 +1,14 @@
+// A generator is left closed after frame.onPop returns a {return:} resumption value.
+
+var g = newGlobal();
+var dbg = new Debugger;
+var gw = dbg.addDebuggee(g);
+dbg.onDebuggerStatement = frame => {
+    frame.onPop = completion => ({return: "ok"});
+};
+g.eval("function* g() { for (var i = 0; i < 10; i++) { debugger; yield i; } }");
+var it = g.g();
+var result = it.next();
+assertEq(result.value, "ok");
+assertEq(result.done, true);
+assertEq(it.next().value, undefined);

js/src/vm/Debugger.cpp

@@ -897,6 +897,47 @@ static void
 DebuggerFrame_maybeDecrementFrameScriptStepModeCount(FreeOp* fop, AbstractFramePtr frame,
                                                      NativeObject* frameobj);
 
+/*
+ * RAII class to mark a generator as "running" temporarily while running
+ * debugger code.
+ *
+ * When Debugger::slowPathOnLeaveFrame is called for a frame that is yielding
+ * or awaiting, its generator is in the "suspended" state. Letting script
+ * observe this state, with the generator on stack yet also reenterable, would
+ * be bad, so we mark it running while we fire events.
+ */
+class MOZ_RAII AutoSetGeneratorRunning
+{
+    int32_t yieldAwaitIndex_;
+    Rooted<GeneratorObject*> genObj_;
+
+  public:
+    AutoSetGeneratorRunning(JSContext* cx, Handle<GeneratorObject*> genObj)
+      : yieldAwaitIndex_(0),
+        genObj_(cx, genObj)
+    {
+        if (genObj) {
+            if (!genObj->isBeforeInitialYield() && !genObj->isClosed() && genObj->isSuspended()) {
+                yieldAwaitIndex_ =
+                    genObj->getFixedSlot(GeneratorObject::YIELD_AND_AWAIT_INDEX_SLOT).toInt32();
+                genObj->setRunning();
+            } else {
+                // We're returning or throwing, not yielding or awaiting. The
+                // generator is already closed, if it was ever exposed at all.
+                genObj_ = nullptr;
+            }
+        }
+    }
+
+    ~AutoSetGeneratorRunning() {
+        if (genObj_) {
+            MOZ_ASSERT(genObj_->isRunning());
+            genObj_->setFixedSlot(GeneratorObject::YIELD_AND_AWAIT_INDEX_SLOT,
+                                  Int32Value(yieldAwaitIndex_));
+        }
+    }
+};
+
 /*
  * Handle leaving a frame with debuggers watching. |frameOk| indicates whether
  * the frame is exiting normally or abruptly. Set |cx|'s exception and/or
@@ -926,32 +967,11 @@ Debugger::slowPathOnLeaveFrame(JSContext* cx, AbstractFramePtr frame, jsbytecode
     RootedValue value(cx);
     Debugger::resultToCompletion(cx, frameOk, frame.returnValue(), &resumeMode, &value);
 
-    // If we are yielding or awaiting, the generator is currently in the
-    // "suspended" state.  Letting script observe this state, with the
-    // generator on stack and yet also reenterable, would be bad, so mark
-    // it running while we fire events.
-    int32_t yieldAwaitIndex = 0;
+    // If we are yielding or awaiting, we'll need to mark the generator as
+    // "running" temporarily.
     Rooted<GeneratorObject*> genObj(cx);
-    if (frame.isFunctionFrame() && (frame.callee()->isGenerator() || frame.callee()->isAsync())) {
+    if (frame.isFunctionFrame() && (frame.callee()->isGenerator() || frame.callee()->isAsync()))
         genObj = GetGeneratorObjectForFrame(cx, frame);
-        if (genObj) {
-            if (!genObj->isBeforeInitialYield() && !genObj->isClosed() && genObj->isSuspended()) {
-                yieldAwaitIndex =
-                    genObj->getFixedSlot(GeneratorObject::YIELD_AND_AWAIT_INDEX_SLOT).toInt32();
-                genObj->setRunning();
-            } else {
-                // We're returning or throwing, not yielding or awaiting. The
-                // generator is already closed, if it was ever exposed at all.
-                genObj = nullptr;
-            }
-        }
-    }
-    auto restoreGenSuspended = MakeScopeExit([&] {
-        if (genObj) {
-            genObj->setFixedSlot(GeneratorObject::YIELD_AND_AWAIT_INDEX_SLOT,
-                                 Int32Value(yieldAwaitIndex));
-        }
-    });
 
     // This path can be hit via unwinding the stack due to over-recursion or
     // OOM. In those cases, don't fire the frames' onPop handlers, because
@@ -982,7 +1002,11 @@ Debugger::slowPathOnLeaveFrame(JSContext* cx, AbstractFramePtr frame, jsbytecode
                 // Call the onPop handler.
                 ResumeMode nextResumeMode = resumeMode;
                 RootedValue nextValue(cx, wrappedValue);
-                bool success = handler->onPop(cx, frameobj, nextResumeMode, &nextValue);
+                bool success;
+                {
+                    AutoSetGeneratorRunning asgr(cx, genObj);
+                    success = handler->onPop(cx, frameobj, nextResumeMode, &nextValue);
+                }
                 nextResumeMode = dbg->processParsedHandlerResult(ar, frame, pc, success,
                                                                  nextResumeMode, &nextValue);
 
@@ -1476,7 +1500,7 @@ AdjustGeneratorResumptionValue(JSContext* cx, AbstractFramePtr frame,
             vp.setObject(*pair);
 
             // 2.  The generator must be closed.
-            GeneratorObject::finalSuspend(genObj);
+            genObj->setClosed();
         } else {
             // We're before the initial yield. Carry on with the forced return.
             // The debuggee will see a call to a generator returning the