Bug 1477084 - Fix assertion with Debugger forcing return from an async generator before its initial yield. r=jimb

Differential Revision: https://phabricator.services.mozilla.com/D14789

--HG--
rename : js/src/jit-test/tests/debug/onStep-generator-resumption-01.js => js/src/jit-test/tests/debug/Frame-onStep-generator-resumption-03.js
extra : moz-landing-system : lando

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

@@ -1,4 +1,4 @@
-// The debugger can force an early return from any instruction before the initial yield.
+// The debugger can't force return from a generator before the initial yield.
 
 let g = newGlobal({newCompartment: true});
 g.eval(`
@@ -7,36 +7,36 @@ g.eval(`
   }
 `);
 
-function test(ttl) {
-    let dbg = new Debugger(g);
-    let exiting = false;  // we ran out of time-to-live and have forced return
-    let done = false;  // we reached the initial yield without forced return
-    dbg.onEnterFrame = frame => {
-        assertEq(frame.callee.name, "f");
-        frame.onEnterFrame = undefined;
-        frame.onStep = () => {
-            if (ttl == 0) {
-                exiting = true;
-                // Forced return here causes the generator object, if any, not
-                // to be exposed.
-                return {return: "ponies"};
-            }
-            ttl--;
-        };
-        frame.onPop = completion => {
-            if (!exiting)
-                done = true;
-        };
-    };
+let dbg = new Debugger(g);
+let steps = 0;
+let uncaughtErrorsReported = 0;
+dbg.onEnterFrame = frame => {
+  assertEq(frame.callee.name, "f");
+  dbg.onEnterFrame = undefined;
+  frame.onStep = () => {
+    steps++;
 
-    let result = g.f();
-    if (done)
-        assertEq(result instanceof g.f, true);
-    else
-        assertEq(result, "ponies");
+    // This test case never resumes the generator after the initial
+    // yield. Therefore the initial yield has not happened yet. So this
+    // force-return will be an error.
+    return {return: "ponies"};
+  };
 
-    dbg.enabled = false;
-    return done;
-}
+  // Having an onPop hook exercises some assertions that don't happen
+  // otherwise.
+  frame.onPop = completion => {};
+};
 
-for (let ttl = 0; !test(ttl); ttl++) {}
+dbg.uncaughtExceptionHook = (reason) => {
+  // When onEnterFrame returns an invalid resumption value,
+  // the error is reported here.
+  assertEq(reason instanceof TypeError, true);
+  uncaughtErrorsReported++;
+  return undefined;  // Cancel the force-return. Let the debuggee continue.
+};
+
+let result = g.f();
+assertEq(result instanceof g.f, true);
+
+assertEq(steps > 0, true);
+assertEq(uncaughtErrorsReported, steps);

js/src/jit-test/tests/debug/Frame-onStep-generator-resumption-02.js

@@ -0,0 +1,78 @@
+// Like Frame-onStep-generator-resumption-01.js, but bail out by throwing.
+
+let g = newGlobal({newCompartment: true});
+g.eval(`
+  function* f() {
+    yield 1;
+  }
+`);
+
+// Try force-returning from one of the instructions in `f` before the initial
+// yield. In detail:
+//
+// *   This test calls `g.f()` under the Debugger.
+// *   It uses the Debugger to step `ttl` times.
+//     If we reach the initial yield before stepping the `ttl`th time, we're done.
+// *   Otherwise, the test tries to force-return from `f`.
+// *   That's an error, so the uncaughtExceptionHook is called.
+// *   The uncaughtExceptionHook returns a `throw` completion value.
+//
+// Returns `true` if we reached the initial yield, false otherwise.
+//
+// Note that this function is called in a loop so that every possible relevant
+// value of `ttl` is tried once.
+function test(ttl) {
+  let dbg = new Debugger(g);
+  let exiting = false;  // we ran out of time-to-live and have forced return
+  let done = false;  // we reached the initial yield without forced return
+  let reported = false;  // a TypeError was reported.
+
+  dbg.onEnterFrame = frame => {
+    assertEq(frame.callee.name, "f");
+    dbg.onEnterFrame = undefined;
+    frame.onStep = () => {
+      if (ttl == 0) {
+        exiting = true;
+        // This test case never resumes the generator after the initial
+        // yield. Therefore the initial yield has not happened yet. So this
+        // force-return will be an error.
+        return {return: "ponies"};
+      }
+      ttl--;
+    };
+    frame.onPop = completion => {
+      if (!exiting)
+        done = true;
+    };
+  };
+
+  dbg.uncaughtExceptionHook = (exc) => {
+    // When onStep returns an invalid resumption value,
+    // the error is reported here.
+    assertEq(exc instanceof TypeError, true);
+    reported = true;
+    return {throw: "FAIL"};  // Bail out of the test.
+  };
+
+  let result;
+  let caught = undefined;
+  try {
+    result = g.f();
+  } catch (exc) {
+    caught = exc;
+  }
+
+  if (done) {
+    assertEq(reported, false);
+    assertEq(result instanceof g.f, true);
+    assertEq(caught, undefined);
+  } else {
+    assertEq(reported, true);
+    assertEq(caught, "FAIL");
+  }
+
+  dbg.enabled = false;
+  return done;
+}
+
+for (let ttl = 0; !test(ttl); ttl++) {}

js/src/jit-test/tests/debug/Frame-onStep-generator-resumption-03.js

@@ -0,0 +1,49 @@
+// Don't crash on {return:} from onStep in a generator, before the initial suspend.
+
+// This test tries to force-return from each bytecode instruction in a
+// generator, up to the initial suspend.
+
+load(libdir + "asserts.js");
+
+let g = newGlobal({newCompartment: true});
+g.values = [1, 2, 3];
+g.eval(`function* f(arr=values) { yield* arr; }`);
+
+let dbg = Debugger(g);
+
+function test(ttl) {
+    let hits = 0;
+    dbg.onEnterFrame = frame => {
+        assertEq(frame.callee.name, "f");
+        frame.onStep = () => {
+            if (--ttl === 0)
+                return {return: 123};
+        };
+    };
+    dbg.uncaughtExceptionHook = exc => {
+      return {throw: "debugger error: " + exc};
+    };
+
+    let val = undefined;
+    let caught = undefined;
+    try {
+        val = g.f();
+    } catch (exc) {
+        caught = exc;
+    }
+
+    if (val === undefined) {
+        // Tried to force-return before the initial suspend.
+        assertEq(caught, "debugger error: TypeError: can't force return from a generator before the initial yield");
+        assertEq(ttl, 0);
+        return "pass";
+    } else {
+        // Reached the initial suspend without forcing a return.
+        assertEq(typeof val, "object");
+        assertEq(val instanceof g.f, true);
+        assertEq(ttl, 1);
+        return "done";
+    }
+}
+
+for (let i = 1; test(i) === "pass"; i++) {}

js/src/jit-test/tests/debug/bug-1477084.js

@@ -0,0 +1,30 @@
+// Don't assert trying to force return before the initial yield of an async function.
+
+var g = newGlobal({newCompartment: true});
+g.parent = this;
+g.parentExc = new Error("pants");
+g.eval(`
+  var dbg = new Debugger;
+  var pw = dbg.addDebuggee(parent);
+  var hits = 0;
+  dbg.onExceptionUnwind = function (frame) {
+    dbg.onExceptionUnwind = undefined;
+    return {return: undefined};
+  };
+  dbg.uncaughtExceptionHook = exc => {
+    hits++;
+    assertEq(exc instanceof TypeError, true);
+    assertEq(/force return.*before the initial yield/.test(exc.message), true);
+    return {throw: pw.makeDebuggeeValue(parentExc)};
+  };
+`);
+
+async function* method({ x: callbackfn = unresolvableReference }) {}
+try {
+  method();
+} catch (exc) {
+  g.dbg.enabled = false;
+  assertEq(exc, g.parentExc);
+}
+
+assertEq(g.hits, 1);

js/src/jit-test/tests/debug/onEnterFrame-async-resumption-05.js

@@ -1,32 +1,41 @@
-// A Debugger can {return:} from the first onEnterFrame for an async generator.
-// (The exact behavior is undocumented; we're testing that it doesn't crash.)
+// A Debugger can't force-return from the first onEnterFrame for an async generator.
 
 ignoreUnhandledRejections();
 
 let g = newGlobal({newCompartment: true});
-g.hit2 = false;
 g.eval(`async function* f(x) { await x; return "ponies"; }`);
 
 let dbg = new Debugger;
 let gw = dbg.addDebuggee(g);
-let hits = 0;
+let log = "";
+let completion = undefined;
 let resumption = undefined;
+dbg.uncaughtExceptionHook = exc => {
+    log += "2";
+    assertEq(exc.message, "can't force return from a generator before the initial yield");
+    assertEq(exc.constructor, TypeError);
+    return undefined;  // Squelch the error and let the debuggee continue.
+};
 dbg.onEnterFrame = frame => {
     if (frame.type == "call" && frame.callee.name === "f") {
-        frame.onPop = completion => {
-            assertEq(completion.return, resumption.return);
-            hits++;
+        frame.onPop = c => {
+            // We get here after the uncaughtExcpetionHook fires
+            // and the debuggee frame has run to the first await.
+            completion = c;
+            assertEq(completion.return.class, "AsyncGenerator");
+            assertEq(completion.return !== resumption.return, true);
+            log += "3";
         };
 
-        // If we force-return a generator object here, the caller will never
-        // receive an async generator object.
-        resumption = frame.eval(`(function* f2() { hit2 = true; })()`);
-        assertEq(resumption.return.class, "Generator");
+        // Try force-returning an actual object of the expected type.
+        dbg.onEnterFrame = undefined;  // don't recurse
+        resumption = frame.eval('f(0)');
+        assertEq(resumption.return.class, "AsyncGenerator");
+        log += "1";
         return resumption;
     }
 };
 
 let it = g.f(0);
-assertEq(hits, 1);
-assertEq(gw.makeDebuggeeValue(it), resumption.return);
-assertEq(g.hit2, false);
+assertEq(log, "123");
+assertEq(gw.makeDebuggeeValue(it), completion.return);

js/src/jit-test/tests/debug/onEnterFrame-async-resumption-07.js

@@ -1,54 +1,62 @@
-// A Debugger can {return:} from the first onEnterFrame for an async generator.
-// (The exact behavior is undocumented; we're testing that it doesn't crash.)
+// A Debugger can't force-return from the first onEnterFrame for an async generator.
 
 ignoreUnhandledRejections();
 
 let g = newGlobal({newCompartment: true});
 g.eval(`async function* f(x) { await x; return "ponies"; }`);
-g.eval(`async function* f2(x) { await x; return "moar ponies"; }`);
 
 let dbg = new Debugger;
+dbg.uncaughtExceptionHook = exc => {}; // ignore errors
 let gw = dbg.addDebuggee(g);
 let hits = 0;
 let resumption = undefined;
+
+dbg.onEnterFrame = frame => {
+    dbg.onEnterFrame = undefined;
+    assertEq(frame.type, "call");
+    assertEq(frame.callee.name, "f");
+    frame.onPop = completion => {
+        hits++;
+    };
+
+    // Try to force-return. It's too early. This results in a call to the
+    // uncaughtExceptionHook but is otherwise ignored.
+    return {return: "rainbows"};
+};
+let it = g.f(0);    // onPop #1: the initial yield
+assertEq(hits, 1);
+let p = it.next();  // onPop #2: await x
+assertEq(hits, 2);
+drainJobQueue();    // onPop #3: return "ponies", #4: the final yield
+assertEq(hits, 4);
+
+let pw = gw.makeDebuggeeValue(p);
+assertEq(pw.isPromise, true);
+assertEq(pw.promiseState, "fulfilled");
+assertEq(pw.promiseValue.getProperty("value").return, "ponies");
+assertEq(pw.promiseValue.getProperty("done").return, true);
+
+// ----
+
+g.eval(`async function* f2(x) { await x; return "moar ponies"; }`);
+
 let savedAsyncGen = undefined;
 dbg.onEnterFrame = frame => {
-    if (frame.type == "call" && frame.callee.name === "f2") {
-        frame.onPop = completion => {
-            if (savedAsyncGen === undefined) {
-                savedAsyncGen = completion.return;
-            }
-        };
-    }
-    if (frame.type == "call" && frame.callee.name === "f") {
-        frame.onPop = completion => {
-            hits++;
-        };
-
-        return {return: savedAsyncGen};
-    }
+    dbg.onEnterFrame = undefined;
+    assertEq(frame.type, "call");
+    assertEq(frame.callee.name, "f2");
+    frame.onPop = completion => {
+        if (savedAsyncGen === undefined) {
+            savedAsyncGen = completion.return;
+        }
+    };
 };
-
 let it2 = g.f2(123);
-let it = g.f(0);
-
 let p2 = it2.next();
-let p = it.next();
-
-assertEq(hits, 1);
-
 drainJobQueue();
 
-assertEq(hits, 1);
-
 let pw2 = gw.makeDebuggeeValue(p2);
 assertEq(pw2.isPromise, true);
 assertEq(pw2.promiseState, "fulfilled");
 assertEq(pw2.promiseValue.getProperty("value").return, "moar ponies");
 assertEq(pw2.promiseValue.getProperty("done").return, true);
-
-let pw = gw.makeDebuggeeValue(p);
-assertEq(pw.isPromise, true);
-assertEq(pw.promiseState, "fulfilled");
-assertEq(pw.promiseValue.getProperty("value").return, undefined);
-assertEq(pw.promiseValue.getProperty("done").return, true);

js/src/jit-test/tests/debug/onEnterFrame-generator-resumption-05.js

@@ -1,5 +1,4 @@
-// {return:} from the initial onEnterFrame for a generator replaces the
-// generator object that's normally returned to the caller.
+// {return:} from the initial onEnterFrame for a generator is an error.
 
 load(libdir + "asserts.js");
 
@@ -15,5 +14,9 @@ dbg.onEnterFrame = frame => {
     hits++;
     return {return: 123};
 };
-assertEq(g.f(), 123);
+dbg.uncaughtExceptionHook = exc => {
+  assertEq(exc instanceof TypeError, true);
+  return {throw: "REJECTED"};
+}
+assertThrowsValue(g.f, "REJECTED");
 assertEq(hits, 1);

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

@@ -1,36 +0,0 @@
-// Don't crash on {return:} from onStep in a generator, before the initial suspend.
-
-// This test forces a return from each bytecode instruction in a generator, up
-// to the initial suspend.
-
-load(libdir + "asserts.js");
-
-let g = newGlobal({newCompartment: true});
-g.values = [1, 2, 3];
-g.eval(`function* f(arr=values) { yield* arr; }`);
-
-let dbg = Debugger(g);
-
-function test(ttl) {
-    let hits = 0;
-    dbg.onEnterFrame = frame => {
-        assertEq(frame.callee.name, "f");
-        frame.onStep = () => {
-            if (--ttl === 0)
-                return {return: 123};
-        };
-    };
-    let val = g.f();
-    if (typeof val === "object") {
-        // Reached the initial suspend without forcing a return.
-        assertEq(ttl, 1);
-        return "done";
-    }
-
-    // Forced a return before the initial suspend.
-    assertEq(val, 123);
-    assertEq(ttl, 0);
-    return "pass";
-}
-
-for (let i = 1; test(i) === "pass"; i++) {}

js/src/js.msg

@@ -488,6 +488,7 @@ MSG_DEF(JSMSG_DEBUG_PROTO,             2, JSEXN_TYPEERR, "{0}.prototype is not a
 MSG_DEF(JSMSG_DEBUG_WRONG_OWNER,       1, JSEXN_TYPEERR, "{0} belongs to a different Debugger")
 MSG_DEF(JSMSG_DEBUG_OPTIMIZED_OUT,     1, JSEXN_ERR, "variable `{0}' has been optimized out")
 MSG_DEF(JSMSG_DEBUG_OPTIMIZED_OUT_FUN, 0, JSEXN_ERR, "function is optimized out")
+MSG_DEF(JSMSG_DEBUG_FORCED_RETURN_DISALLOWED, 0, JSEXN_TYPEERR, "can't force return from a generator before the initial yield")
 MSG_DEF(JSMSG_DEBUG_RESUMPTION_VALUE_DISALLOWED, 0, JSEXN_TYPEERR, "resumption values are disallowed in this hook")
 MSG_DEF(JSMSG_DEBUG_VARIABLE_NOT_FOUND,0, JSEXN_TYPEERR, "variable not found in environment")
 MSG_DEF(JSMSG_DEBUG_WRAPPER_IN_WAY,    3, JSEXN_TYPEERR, "{0} is {1}{2}a global object, but a direct reference is required")

js/src/vm/Debugger.cpp

@@ -963,7 +963,8 @@ class MOZ_RAII AutoSetGeneratorRunning {
         asyncGenState_(static_cast<AsyncGeneratorObject::State>(0)),
         genObj_(cx, genObj) {
     if (genObj) {
-      if (!genObj->isClosed() && genObj->isSuspended()) {
+      if (!genObj->isClosed() && !genObj->isBeforeInitialYield() &&
+          genObj->isSuspended()) {
         // Yielding or awaiting.
         resumeIndex_ = genObj->resumeIndex();
         genObj->setRunning();
@@ -1577,6 +1578,7 @@ static bool CheckResumptionValue(JSContext* cx, AbstractFramePtr frame,
   if (maybeThisv.isSome()) {
     const HandleValue& thisv = maybeThisv.ref();
     if (resumeMode == ResumeMode::Return && vp.isPrimitive()) {
+      // Forcing return from a class constructor. There are rules.
       if (vp.isUndefined()) {
         if (thisv.isMagic(JS_UNINITIALIZED_LEXICAL)) {
           return ThrowUninitializedThis(cx, frame);
@@ -1590,9 +1592,34 @@ static bool CheckResumptionValue(JSContext* cx, AbstractFramePtr frame,
       }
     }
   }
+
+  // Check for forcing return from a generator before the initial yield. This
+  // is not supported because some engine-internal code assumes a call to a
+  // generator will return a GeneratorObject; see bug 1477084.
+  if (resumeMode == ResumeMode::Return && frame && frame.isFunctionFrame() &&
+      frame.callee()->isGenerator()) {
+    Rooted<AbstractGeneratorObject*> genObj(cx);
+    {
+      AutoRealm ar(cx, frame.callee());
+      genObj = GetGeneratorObjectForFrame(cx, frame);
+    }
+
+    if (!genObj || genObj->isBeforeInitialYield()) {
+      JS_ReportErrorNumberASCII(cx, GetErrorMessage, nullptr,
+                                JSMSG_DEBUG_FORCED_RETURN_DISALLOWED);
+      return false;
+    }
+  }
+
   return true;
 }
 
+// Last-minute sanity adjustments to resumption.
+//
+// This is called last, as we leave the debugger. It must happen outside the
+// control of the uncaughtExceptionHook, because this code assumes we won't
+// change our minds and continue execution--we must not close the generator
+// object unless we're really going to force-return.
 static void AdjustGeneratorResumptionValue(JSContext* cx,
                                            AbstractFramePtr frame,
                                            ResumeMode& resumeMode,
@@ -1612,10 +1639,11 @@ static void AdjustGeneratorResumptionValue(JSContext* cx,
   };
 
   // Treat `{return: <value>}` like a `return` statement. Simulate what the
-  // debuggee would do for an ordinary `return` statement--using a few bytecode
-  // instructions--and it's simpler to do the work manually than to count on
-  // that bytecode sequence existing in the debuggee, somehow jump to it, and
-  // then avoid re-entering the debugger from it.
+  // debuggee would do for an ordinary `return` statement, using a few bytecode
+  // instructions. It's simpler to do the work manually than to count on that
+  // bytecode sequence existing in the debuggee, somehow jump to it, and then
+  // avoid re-entering the debugger from it.
+  //
   // Similarly treat `{throw: <value>}` like a `throw` statement.
   if (frame.callee()->isGenerator()) {
     // Throw doesn't require any special processing for (async) generators.
@@ -1623,30 +1651,34 @@ static void AdjustGeneratorResumptionValue(JSContext* cx,
       return;
     }
 
-    // For (async) generators, that means doing the work below.
+    // Forcing return from a (possibly async) generator.
     Rooted<AbstractGeneratorObject*> genObj(
         cx, GetGeneratorObjectForFrame(cx, frame));
-    if (genObj) {
-      // 1.  `return <value>` creates and returns a new object in non-async
-      //     generators, `{value: <value>, done: true}`.
-      if (!frame.callee()->isAsync() && !genObj->isBeforeInitialYield()) {
-        JSObject* pair = CreateIterResultObject(cx, vp, true);
-        if (!pair) {
-          getAndClearExceptionThenThrow();
-          return;
-        }
-        vp.setObject(*pair);
-      }
 
-      // 2.  The generator must be closed.
-      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
-      // non-generator value *vp.
+    // We already went through CheckResumptionValue, which would have replaced
+    // this invalid resumption value with an error if we were trying to force
+    // return before the initial yield.
+    MOZ_RELEASE_ASSERT(genObj && !genObj->isBeforeInitialYield());
+
+    // 1.  `return <value>` creates and returns a new object,
+    //     `{value: <value>, done: true}`.
+    //
+    // For non-async generators, the iterator result object is created in
+    // bytecode, so we have to simulate that here. For async generators, our
+    // C++ implementation of AsyncGeneratorResolve will do this. So don't do it
+    // twice:
+    if (!frame.callee()->isAsync()) {
+      JSObject* pair = CreateIterResultObject(cx, vp, true);
+      if (!pair) {
+        getAndClearExceptionThenThrow();
+        return;
+      }
+      vp.setObject(*pair);
     }
+
+    // 2.  The generator must be closed.
+    genObj->setClosed();
   } else if (frame.callee()->isAsync()) {
-    // For async functions, that means doing the work below.
     if (AbstractGeneratorObject* genObj =
             GetGeneratorObjectForFrame(cx, frame)) {
       // Throw doesn't require any special processing for async functions when