Bug 1470558: Distinguish yields and awaits in completion values. r=jorendorff
authorJim Blandy <jimb@mozilla.com>
Sun, 07 Jul 2019 17:03:57 +0000
changeset 544417 f0aa53a43409f79f50fdb92aed3ec164749c3bee
parent 544416 9efab90160b46946e530e52d467bf00396c187ae
child 544418 3f47b78ffdc0e9cd4db77302d7f4143447f84be5
push id2131
push userffxbld-merge
push dateMon, 26 Aug 2019 18:30:20 +0000
treeherdermozilla-release@b19ffb3ca153 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjorendorff
bugs1470558
milestone69.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 1470558: Distinguish yields and awaits in completion values. r=jorendorff This patch introduces a new type to the debugger, `js::Completion`, describing how a given JavaScript evaluation completed. It's a wrapper around a `Variant` with alternatives: - Return - Throw - Terminate - InitialYield (the initial yield of a generator, returning the generator object) - Yield (subsequent yields of a generator) - Await (both initial and subsequent) We can construct a `Completion` in two ways: - From any JavaScript operation's result (a success value and a context carrying an exception value and stack). This only distinguishes between Return, Throw, and Terminate. - From a stack frame that's about to be popped. This allows us to identify yields and awaits. Given a `Completion` we can construct Debugger API 'completion values' to pass to hooks, as well as the resumption/value/context states that tell the engine how to continue execution. Within Debugger itself, `Completion` values are a convenient place to gather up various bits of logic: identifying suspensions, chaining resumption values from multiple Debugger hooks, and so on. Although `Completion` should be used throughout Debugger, this patch only uses it for the `onPop` hook. Subsequent patches in the series will apply it to other cases where Debugger can invoke JavaScript. Differential Revision: https://phabricator.services.mozilla.com/D24997
js/src/doc/Debugger/Conventions.md
js/src/doc/Debugger/Debugger.Frame.md
js/src/gc/Marking.cpp
js/src/jit-test/lib/match-debugger.js
js/src/jit-test/tests/debug/Frame-onPop-async-generators-01.js
js/src/jit-test/tests/debug/Frame-onPop-generators-06.js
js/src/jit-test/tests/debug/Frame-onPop-generators-07.js
js/src/vm/CommonPropertyNames.h
js/src/vm/Debugger.cpp
js/src/vm/Debugger.h
--- a/js/src/doc/Debugger/Conventions.md
+++ b/js/src/doc/Debugger/Conventions.md
@@ -61,38 +61,75 @@ Similarly:
 - a *debuggee environment* is an environment whose outermost
   enclosing environment is a debuggee global object; and
 
 - a *debuggee script* is a script containing debuggee code.
 
 
 ## Completion Values
 
-When a debuggee stack frame completes its execution, or when some sort
-of debuggee call initiated by the debugger finishes, the `Debugger`
-interface provides a value describing how the code completed; these are
-called *completion values*. A completion value has one of the
-following forms:
+The `Debugger` API often needs to convey the result of running some JS code. For example, suppose you get a `frame.onPop` callback telling you that a method in the debuggee just finished. Did it return successfully? Did it throw? What did it return? The debugger passes the `onPop` handler a *completion value* that tells what happened.
+
+A completion value is one of these:
 
 <code>{ return: <i>value</i> }</code>
 :   The code completed normally, returning <i>value</i>. <i>Value</i> is a
     debuggee value.
 
 <code>{ throw: <i>value</i>, stack: <i>stack</i> }</code>
 :   The code threw <i>value</i> as an exception. <i>Value</i> is a debuggee
     value.  <i>stack</i> is a `SavedFrame` representing the location from which
     the value was thrown, and may be missing.
 
 `null`
-:   The code was terminated, as if by the "slow script" dialog box.
+:   The code was terminated, as if by the "slow script" ribbon.
+
+Generators and async functions add a wrinkle: they can suspend themselves (with `yield` or `await`), which removes their frame from the stack. Later, the generator or async frame might be returned to the stack and continue running where it left off. Does it count as "completion" when a generator suspends itself?
+
+The `Debugger` API says yes. `yield` and `await` do trigger the `frame.onPop` handler, passing a completion value that explains why the frame is being suspended. The completion value gets an extra `.yield` or `.await` property, to distinguish this kind of completion from a normal `return`.
+
+<pre>
+{ return: *value*, yield: true }
+</pre>
+
+where *value* is a debuggee value for the iterator result object, like `{ value: 1, done: false }`, for the yield.
+
+When a generator function is called, it first evaluates any default argument
+expressions and destructures its arguments. Then its frame is suspended, and the
+new generator object is returned to the caller. This initial suspension is reported
+to any `onPop` handlers as a completion value of the form:
+
+<pre>
+{ return: *generatorObject*, yield: true, initial: true }
+</pre>
 
-If control reaches the end of a generator frame, the completion value is
-<code>{ throw: <i>stop</i> }</code> where <i>stop</i> is a
-`Debugger.Object` object representing the `StopIteration` object being
-thrown.
+where *generatorObject* is a debuggee value for the generator object being
+returned to the caller.
+
+When an async function awaits a promise, its suspension is reported to any
+`onPop` handlers as a completion value of the form:
+
+<pre>
+{ return: *promise*, await: true }
+</pre>
+
+where *promise* is a debuggee value for the promise being returned to the
+caller.
+
+The first time a call to an async function awaits, returns, or throws, a promise
+of its result is returned to the caller. Subsequent resumptions of the async
+call, if any, are initiated directly from the job queue's event loop, with no
+calling frame on the stack. Thus, if needed, an `onPop` handler can distinguish
+an async call's initial suspension, which returns the promise, from any
+subsequent suspensions by checking the `Debugger.Frame`'s `older` property: if
+that is `null`, the call was resumed directly from the event loop.
+
+Async generators are a combination of async functions and generators that can
+use both `yield` and `await` expressions. Suspensions of async generator frames
+are reported using any combination of the completion values above.
 
 
 ## Resumption Values
 
 As the debuggee runs, the `Debugger` interface calls various
 debugger-provided handler functions to report the debuggee's behavior.
 Some of these calls can return a value indicating how the debuggee's
 execution should continue; these are called *resumption values*. A
--- a/js/src/doc/Debugger/Debugger.Frame.md
+++ b/js/src/doc/Debugger/Debugger.Frame.md
@@ -112,16 +112,20 @@ SpiderMonkey uses the same `Debugger.Fra
 or async function call is put back onto the stack. This means that the
 `onStep` handler can be used to step over `yield` and `await`.
 
 The `frame.onPop` handler is called each time a frame is suspended, and
 the `Debugger.onEnterFrame` handler is called each time a frame is
 resumed. (This means these events can fire multiple times for the same
 `Frame` object, which is odd, but accurately conveys what's happening.)
 
+The [completion value][cv] passed to the `frame.onPop` handler for a suspension
+contains additional properties to clarify what's going on. See the documentation
+for completion values for details.
+
 
 ## Stepping Into Generators: The "Initial Yield"
 
 When a debuggee generator is called, something weird happens. The
 `.onEnterFrame` hook fires, as though we're stepping into the generator.
 But the code inside the generator doesn't run. Instead it immediately
 returns. Then we sometimes get *another* `.onEnterFrame` event for the
 same generator. What's going on?
@@ -274,29 +278,29 @@ the compartment to which the handler met
     resumption value other than `undefined`), any remaining debuggers'
     `onStep` handlers do not run.
 
     This property is ignored on frames that are not executing debuggee
     code, like `"call"` frames for calls to host functions and `"debugger"`
     frames.
 
 `onPop`
-:   This property must be either `undefined` or a function. If it is a
-    function, SpiderMonkey calls it just before this frame is popped,
-    passing a [completion value][cv] indicating how this frame's execution
-    completed, and providing this `Debugger.Frame` instance as the `this`
-    value. The function should return a [resumption value][rv] indicating
-    how execution should proceed. On newly created frames, this property's
-    value is `undefined`.
+:   This property must be either `undefined` or a function. If it is a function,
+    SpiderMonkey calls it just before this frame is popped or suspended, passing
+    a [completion value][cv] indicating the reason, and providing this
+    `Debugger.Frame` instance as the `this` value. The function should return a
+    [resumption value][rv] indicating how execution should proceed. On newly
+    created frames, this property's value is `undefined`.
 
     When this handler is called, this frame's current execution location, as
     reflected in its `offset` and `environment` properties, is the operation
-    which caused it to be unwound. In frames returning or throwing an
-    exception, the location is often a return or a throw statement. In frames
-    propagating exceptions, the location is a call.
+    which caused it to be unwound. In frames returning or throwing an exception,
+    the location is often a return or a throw statement. In frames propagating
+    exceptions, the location is a call. In generator or async function frames,
+    the location may be a `yield` or `await` expression.
 
     When an `onPop` call reports the completion of a construction call
     (that is, a function called via the `new` operator), the completion
     value passed to the handler describes the value returned by the
     function body. If this value is not an object, it may be different from
     the value produced by the `new` expression, which will be the value of
     the frame's `this` property. (In ECMAScript terms, the `onPop` handler
     receives the value returned by the `[[Call]]` method, not the value
@@ -315,25 +319,25 @@ the compartment to which the handler met
     termination process undisturbed.
 
     If multiple [`Debugger`][debugger-object] instances each have
     `Debugger.Frame` instances for a given stack frame with `onPop`
     handlers set, their handlers are run in an unspecified order. The
     resumption value each handler returns establishes the completion value
     reported to the next handler.
 
+    The `onPop` handler is typically called only once for a given
+    `Debugger.Frame`, after which the frame becomes inactive. However, in the
+    case of [generators and async functions](#suspended), `onPop` fires each
+    time the frame is suspended.
+
     This handler is not called on `"debugger"` frames. It is also not called
     when unwinding a frame due to an over-recursion or out-of-memory
     exception.
 
-    The `onPop` handler is typically called only once for a given frame,
-    after which the frame becomes inactive. However, in the case of
-    [generators and async functions](suspended), `onPop` fires each time
-    the frame is suspended.
-
 
 ## Function Properties of the Debugger.Frame Prototype Object
 
 The functions described below may only be called with a `this` value
 referring to a `Debugger.Frame` instance; they may not be used as
 methods of other kinds of objects.
 
 <code id="eval">eval(<i>code</i>, [<i>options</i>])</code>
--- a/js/src/gc/Marking.cpp
+++ b/js/src/gc/Marking.cpp
@@ -426,26 +426,33 @@ JS_PUBLIC_API void js::UnsafeTraceManual
 
 template <typename T>
 JS_PUBLIC_API void JS::UnsafeTraceRoot(JSTracer* trc, T* thingp,
                                        const char* name) {
   MOZ_ASSERT(thingp);
   js::TraceNullableRoot(trc, thingp, name);
 }
 
+namespace js {
+class SavedFrame;
+class AbstractGeneratorObject;
+}  // namespace js
+
 // Instantiate a copy of the Tracing templates for each public GC pointer type.
 #define INSTANTIATE_PUBLIC_TRACE_FUNCTIONS(type)                          \
   template JS_PUBLIC_API void JS::UnsafeTraceRoot<type>(JSTracer*, type*, \
                                                         const char*);     \
   template JS_PUBLIC_API void js::UnsafeTraceManuallyBarrieredEdge<type>( \
       JSTracer*, type*, const char*);                                     \
   template JS_PUBLIC_API void js::gc::TraceExternalEdge<type>(            \
       JSTracer*, type*, const char*);
 FOR_EACH_PUBLIC_GC_POINTER_TYPE(INSTANTIATE_PUBLIC_TRACE_FUNCTIONS)
 FOR_EACH_PUBLIC_TAGGED_GC_POINTER_TYPE(INSTANTIATE_PUBLIC_TRACE_FUNCTIONS)
+INSTANTIATE_PUBLIC_TRACE_FUNCTIONS(SavedFrame*);
+INSTANTIATE_PUBLIC_TRACE_FUNCTIONS(AbstractGeneratorObject*);
 #undef INSTANTIATE_PUBLIC_TRACE_FUNCTIONS
 
 namespace js {
 namespace gc {
 
 #define INSTANTIATE_INTERNAL_TRACE_FUNCTIONS(type)                      \
   template void TraceEdgeInternal<type>(JSTracer*, type*, const char*); \
   template void TraceRangeInternal<type>(JSTracer*, size_t len, type*,  \
--- a/js/src/jit-test/lib/match-debugger.js
+++ b/js/src/jit-test/lib/match-debugger.js
@@ -1,20 +1,49 @@
 // Debugger-oriented Pattern subclasses.
 
 if (typeof Match !== 'function') {
   load(libdir + 'match.js');
 }
 
 class DebuggerObjectPattern extends Match.Pattern {
-  constructor(className) {
+  constructor(className, props) {
     super();
     this.className = className;
+    if (props) {
+      this.props = Match.Pattern.OBJECT_WITH_EXACTLY(props);
+    }
   }
 
   match(actual) {
-    if (!(actual instanceof Debugger.Object) || actual.class !== this.className) {
-      throw new Match.MatchError(`Expected Debugger.Object of class ${this.className}, got ${actual}`);
+    if (!(actual instanceof Debugger.Object)) {
+      throw new Match.MatchError(`Expected Debugger.Object, got ${actual}`);
+    }
+
+    if (actual.class !== this.className) {
+      throw new Match.MatchError(`Expected Debugger.Object of class ${this.className}, got Debugger.Object of class ${actual.class}`);
     }
+
+    if (this.props !== undefined) {
+      const lifted = {};
+      for (const name of actual.getOwnPropertyNames()) {
+        const desc = actual.getOwnPropertyDescriptor(name);
+        if (!('value' in desc)) {
+          throw new Match.MatchError(`Debugger.Object referent has non-value property ${uneval(name)}`);
+        }
+        lifted[name] = desc.value;
+      }
+
+      try {
+        this.props.match(lifted);
+      } catch (inner) {
+        if (!(inner instanceof Match.MatchError)) {
+          throw inner;
+        }
+        inner.message = `matching Debugger.Object referent properties:\n${inner.message}`;
+        throw inner;
+      }
+    }
+
     return true;
   }
 }
 
--- 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
@@ -33,19 +33,19 @@ async function collect() {
   }
   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: new DebuggerObjectPattern("Promise"), await: true }),
+    EXACT({ return: 2, yield: true }),
+    EXACT({ return: new DebuggerObjectPattern("Promise"), await: true }),
+    EXACT({ return: 4, yield: true }),
+    EXACT({ return: "ok", await: true }),
     EXACT({ return: "ok" }),
   ]).assert(log);
 
   throw "all-jobs-completed-successfully";
 });
 
--- a/js/src/jit-test/tests/debug/Frame-onPop-generators-06.js
+++ b/js/src/jit-test/tests/debug/Frame-onPop-generators-06.js
@@ -18,11 +18,11 @@ g.eval(`
         capture();
         yield 3;
         return "ok";
     }
 `);
 
 assertDeepEq([... g.f()], [3]);
 Pattern([
-  EXACT({ return: new DebuggerObjectPattern("Object") }),
-  EXACT({ return: new DebuggerObjectPattern("Object") }),
+  EXACT({ return: new DebuggerObjectPattern("Object", { value: 3, done: false }), yield: true }),
+  EXACT({ return: new DebuggerObjectPattern("Object", { value: "ok", done: true }) }),
 ]).assert(log);
new file mode 100644
--- /dev/null
+++ b/js/src/jit-test/tests/debug/Frame-onPop-generators-07.js
@@ -0,0 +1,40 @@
+// Completion values for generators report yields and initial yields.
+
+load(libdir + "asserts.js");
+load(libdir + 'match.js');
+load(libdir + 'match-debugger.js');
+const { Pattern } = Match;
+const { OBJECT_WITH_EXACTLY: X } = Pattern;
+
+const g = newGlobal({ newCompartment: true });
+g.eval(`
+  function* f() {
+    yield "yielding";
+    return "returning";
+  }
+`);
+
+const dbg = new Debugger(g);
+const completions = [];
+dbg.onEnterFrame = frame => {
+  frame.onPop = completion => {
+    completions.push(completion);
+  };
+};
+
+assertDeepEq([... g.f()], ["yielding"]);
+print(uneval(completions));
+Pattern([
+  X({
+    return: new DebuggerObjectPattern("Generator", {}),
+    yield: true,
+    initial: true
+  }),
+  X({
+    return: new DebuggerObjectPattern("Object", { value: "yielding", done: false }),
+    yield: true
+  }),
+  X({
+    return: new DebuggerObjectPattern("Object", { value: "returning", done: true })
+  }),
+]).assert(completions);
--- a/js/src/vm/CommonPropertyNames.h
+++ b/js/src/vm/CommonPropertyNames.h
@@ -205,16 +205,17 @@
   MACRO(implements, implements, "implements")                                  \
   MACRO(import, import, "import")                                              \
   MACRO(in, in, "in")                                                          \
   MACRO(includes, includes, "includes")                                        \
   MACRO(incumbentGlobal, incumbentGlobal, "incumbentGlobal")                   \
   MACRO(index, index, "index")                                                 \
   MACRO(infinity, infinity, "infinity")                                        \
   MACRO(Infinity, Infinity, "Infinity")                                        \
+  MACRO(initial, initial, "initial")                                           \
   MACRO(InitializeCollator, InitializeCollator, "InitializeCollator")          \
   MACRO(InitializeDateTimeFormat, InitializeDateTimeFormat,                    \
         "InitializeDateTimeFormat")                                            \
   MACRO(InitializeNumberFormat, InitializeNumberFormat,                        \
         "InitializeNumberFormat")                                              \
   MACRO(InitializePluralRules, InitializePluralRules, "InitializePluralRules") \
   MACRO(InitializeRelativeTimeFormat, InitializeRelativeTimeFormat,            \
         "InitializeRelativeTimeFormat")                                        \
--- a/js/src/vm/Debugger.cpp
+++ b/js/src/vm/Debugger.cpp
@@ -11,16 +11,17 @@
 #include "mozilla/Sprintf.h"
 #include "mozilla/TypeTraits.h"
 
 #include <utility>
 
 #include "jsfriendapi.h"
 #include "jsnum.h"
 
+#include "builtin/Promise.h"
 #include "frontend/BytecodeCompilation.h"
 #include "frontend/Parser.h"
 #include "gc/FreeOp.h"
 #include "gc/HashUtil.h"
 #include "gc/Marking.h"
 #include "gc/Policy.h"
 #include "gc/PublicIterators.h"
 #include "jit/BaselineDebugModeOSR.h"
@@ -37,17 +38,16 @@
 #include "js/Wrapper.h"
 #include "proxy/ScriptedProxyHandler.h"
 #include "util/Text.h"
 #include "vm/ArgumentsObject.h"
 #include "vm/AsyncFunction.h"
 #include "vm/AsyncIteration.h"
 #include "vm/DebuggerMemory.h"
 #include "vm/GeckoProfiler.h"
-#include "vm/GeneratorObject.h"
 #include "vm/JSContext.h"
 #include "vm/JSObject.h"
 #include "vm/Realm.h"
 #include "vm/TraceLogging.h"
 #include "vm/WrapperObject.h"
 #include "wasm/WasmInstance.h"
 
 #include "gc/GC-inl.h"
@@ -977,64 +977,44 @@ class MOZ_RAII AutoSetGeneratorRunning {
  */
 /* static */
 bool Debugger::slowPathOnLeaveFrame(JSContext* cx, AbstractFramePtr frame,
                                     jsbytecode* pc, bool frameOk) {
   MOZ_ASSERT_IF(!frame.isWasmDebugFrame(), pc);
 
   mozilla::DebugOnly<Handle<GlobalObject*>> debuggeeGlobal = cx->global();
 
-  bool suspending = false;
+  // These are updated below, but consulted by the cleanup code we register now,
+  // so declare them here, initialized to quiescent values.
+  Rooted<Completion> completion(cx);
   bool success = false;
+
   auto frameMapsGuard = MakeScopeExit([&] {
-    // Clean up all Debugger.Frame instances on exit. On suspending, pass
-    // the flag that says to leave those frames `.live`. Note that if
-    // suspending && !success, the generator is closed, not suspended.
-    removeFromFrameMapsAndClearBreakpointsIn(cx, frame, suspending && success);
+    // Clean up all Debugger.Frame instances on exit. On suspending, pass the
+    // flag that says to leave those frames `.live`. Note that if the completion
+    // is a suspension but success is false, the generator gets closed, not
+    // suspended.
+    removeFromFrameMapsAndClearBreakpointsIn(
+        cx, frame, success && completion.get().suspending());
   });
 
   // The onPop handler and associated clean up logic should not run multiple
   // times on the same frame. If slowPathOnLeaveFrame has already been
   // called, the frame will not be present in the Debugger frame maps.
   Rooted<DebuggerFrameVector> frames(cx, DebuggerFrameVector(cx));
   if (!getDebuggerFrames(frame, &frames)) {
     return false;
   }
   if (frames.empty()) {
     return frameOk;
   }
 
-  // Determine if we are suspending this frame or popping it forever.
-  Rooted<AbstractGeneratorObject*> genObj(cx);
-  if (frame.isGeneratorFrame()) {
-    // Since generators are never wasm, we can assume pc is not nullptr, and
-    // that analyzing bytecode is meaningful.
-    MOZ_ASSERT(!frame.isWasmDebugFrame());
-
-    // If we're leaving successfully at a yield opcode, we're probably
-    // suspending; the `isClosed()` check detects a debugger forced return
-    // from an `onStep` handler, which looks almost the same.
-    //
-    // GetGeneratorObjectForFrame can return nullptr even when a generator
-    // object does exist, if the frame is paused between the GENERATOR and
-    // SETALIASEDVAR opcodes. But by checking the opcode first we eliminate that
-    // possibility, so it's fine to call genObj->isClosed().
-    genObj = GetGeneratorObjectForFrame(cx, frame);
-    suspending =
-        frameOk &&
-        (*pc == JSOP_INITIALYIELD || *pc == JSOP_YIELD || *pc == JSOP_AWAIT) &&
-        !genObj->isClosed();
-  }
-
-  // Save the frame's completion value.
-  ResumeMode resumeMode;
-  RootedValue value(cx);
-  RootedSavedFrame exnStack(cx);
-  Debugger::resultToCompletion(cx, frameOk, frame.returnValue(), &resumeMode,
-                               &value, &exnStack);
+  completion = Completion::fromJSFramePop(cx, frame, pc, frameOk);
+  Rooted<AbstractGeneratorObject*> genObj(
+      cx, completion.get().maybeGeneratorObject());
 
   // 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;
   if (!adjqi.init(cx)) {
     return false;
   }
@@ -1051,58 +1031,54 @@ bool Debugger::slowPathOnLeaveFrame(JSCo
       EnterDebuggeeNoExecute nx(cx, *dbg, adjqi);
 
       if (dbg->enabled && frameobj->isLive() && frameobj->onPopHandler()) {
         OnPopHandler* handler = frameobj->onPopHandler();
 
         Maybe<AutoRealm> ar;
         ar.emplace(cx, dbg->object);
 
-        RootedValue wrappedValue(cx, value);
-        RootedValue completion(cx);
-        if (!dbg->wrapDebuggeeValue(cx, &wrappedValue)) {
-          resumeMode = dbg->reportUncaughtException(ar);
-          break;
-        }
+        // The resumption requested by the onPop handler we're about to call.
+        ResumeMode nextResumeMode;
+        RootedValue nextValue(cx);
 
         // Call the onPop handler.
-        ResumeMode nextResumeMode = resumeMode;
-        RootedValue nextValue(cx, wrappedValue);
         bool success;
         {
           // Mark the generator as running, to prevent reentrance.
           //
           // At certain points in a generator's lifetime,
           // GetGeneratorObjectForFrame can return null even when the generator
           // exists, but at those points the generator has not yet been exposed
           // to JavaScript, so reentrance isn't possible anyway. So there's no
           // harm done if this has no effect in that case.
           AutoSetGeneratorRunning asgr(cx, genObj);
-          success = handler->onPop(cx, frameobj, nextResumeMode, &nextValue,
-                                   exnStack);
+          success = handler->onPop(cx, frameobj, completion, nextResumeMode,
+                                   &nextValue);
         }
         nextResumeMode = dbg->processParsedHandlerResult(
             ar, frame, pc, success, nextResumeMode, &nextValue);
         adjqi.runJobs();
 
         // At this point, we are back in the debuggee compartment, and
         // any error has been wrapped up as a completion value.
         MOZ_ASSERT(cx->compartment() == debuggeeGlobal->compartment());
         MOZ_ASSERT(!cx->isExceptionPending());
 
-        // ResumeMode::Continue means "make no change".
-        if (nextResumeMode != ResumeMode::Continue) {
-          resumeMode = nextResumeMode;
-          value = nextValue;
-        }
-      }
-    }
-  }
-
-  // Establish (resumeMode, value) as our resumption value.
+        completion.get().updateForNextHandler(nextResumeMode, nextValue);
+      }
+    }
+  }
+
+  // Now that we've run all the handlers, extract the final resumption mode. */
+  ResumeMode resumeMode;
+  RootedValue value(cx);
+  RootedSavedFrame exnStack(cx);
+  completion.get().toResumeMode(resumeMode, &value, &exnStack);
+
   switch (resumeMode) {
     case ResumeMode::Return:
       frame.setReturnValue(value);
       success = true;
       return true;
 
     case ResumeMode::Throw:
       // If we have a stack from the original throw, use it instead of
@@ -1891,16 +1867,274 @@ ResumeMode Debugger::processHandlerResul
   }
   return leaveDebugger(ar, frame, maybeThisv, CallUncaughtExceptionHook::Yes,
                        resumeMode, vp);
 }
 
 /*** Debuggee completion values *********************************************/
 
 /* static */
+Completion Completion::fromJSResult(JSContext* cx, bool ok, const Value& rv) {
+  MOZ_ASSERT_IF(ok, !cx->isExceptionPending());
+
+  if (ok) {
+    return Completion(Return(rv));
+  }
+
+  if (!cx->isExceptionPending()) {
+    return Completion(Terminate());
+  }
+
+  RootedValue exception(cx);
+  RootedSavedFrame stack(cx, cx->getPendingExceptionStack());
+  MOZ_ALWAYS_TRUE(cx->getPendingException(&exception));
+
+  cx->clearPendingException();
+
+  return Completion(Throw(exception, stack));
+}
+
+/* static */
+Completion Completion::fromJSFramePop(JSContext* cx, AbstractFramePtr frame,
+                                      const jsbytecode* pc, bool ok) {
+  // Only Wasm frames get a null pc.
+  MOZ_ASSERT_IF(!frame.isWasmDebugFrame(), pc);
+
+  // If this isn't a generator suspension, then that's already handled above.
+  if (!ok || !frame.isGeneratorFrame()) {
+    return fromJSResult(cx, ok, frame.returnValue());
+  }
+
+  // A generator is being suspended or returning.
+
+  // Since generators are never wasm, we can assume pc is not nullptr, and
+  // that analyzing bytecode is meaningful.
+  MOZ_ASSERT(!frame.isWasmDebugFrame());
+
+  // If we're leaving successfully at a yield opcode, we're probably
+  // suspending; the `isClosed()` check detects a debugger forced return from
+  // an `onStep` handler, which looks almost the same.
+  //
+  // GetGeneratorObjectForFrame can return nullptr even when a generator
+  // object does exist, if the frame is paused between the GENERATOR and
+  // SETALIASEDVAR opcodes. But by checking the opcode first we eliminate that
+  // possibility, so it's fine to call genObj->isClosed().
+  Rooted<AbstractGeneratorObject*> generatorObj(
+      cx, GetGeneratorObjectForFrame(cx, frame));
+  switch (*pc) {
+    case JSOP_INITIALYIELD:
+      MOZ_ASSERT(!generatorObj->isClosed());
+      return Completion(InitialYield(generatorObj));
+
+    case JSOP_YIELD:
+      MOZ_ASSERT(!generatorObj->isClosed());
+      return Completion(Yield(generatorObj, frame.returnValue()));
+
+    case JSOP_AWAIT:
+      MOZ_ASSERT(!generatorObj->isClosed());
+      return Completion(Await(generatorObj, frame.returnValue()));
+
+    default:
+      return Completion(Return(frame.returnValue()));
+  }
+}
+
+void Completion::trace(JSTracer* trc) {
+  variant.match([=](auto& var) { var.trace(trc); });
+}
+
+struct MOZ_STACK_CLASS Completion::BuildValueMatcher {
+  JSContext* cx;
+  Debugger* dbg;
+  MutableHandleValue result;
+
+  BuildValueMatcher(JSContext* cx, Debugger* dbg, MutableHandleValue result)
+      : cx(cx), dbg(dbg), result(result) {
+    cx->check(dbg->toJSObject());
+  }
+
+  bool operator()(const Completion::Return& ret) {
+    RootedNativeObject obj(cx, newObject());
+    RootedValue retval(cx, ret.value);
+    if (!obj || !wrap(&retval) || !add(obj, cx->names().return_, retval)) {
+      return false;
+    }
+    result.setObject(*obj);
+    return true;
+  }
+
+  bool operator()(const Completion::Throw& thr) {
+    RootedNativeObject obj(cx, newObject());
+    RootedValue exc(cx, thr.exception);
+    if (!obj || !wrap(&exc) || !add(obj, cx->names().throw_, exc)) {
+      return false;
+    }
+    if (thr.stack) {
+      RootedValue stack(cx, ObjectValue(*thr.stack));
+      if (!wrapStack(&stack) || !add(obj, cx->names().stack, stack)) {
+        return false;
+      }
+    }
+    result.setObject(*obj);
+    return true;
+  }
+
+  bool operator()(const Completion::Terminate& term) {
+    result.setNull();
+    return true;
+  }
+
+  bool operator()(const Completion::InitialYield& initialYield) {
+    RootedNativeObject obj(cx, newObject());
+    RootedValue gen(cx, ObjectValue(*initialYield.generatorObject));
+    if (!obj || !wrap(&gen) || !add(obj, cx->names().return_, gen) ||
+        !add(obj, cx->names().yield, TrueHandleValue) ||
+        !add(obj, cx->names().initial, TrueHandleValue)) {
+      return false;
+    }
+    result.setObject(*obj);
+    return true;
+  }
+
+  bool operator()(const Completion::Yield& yield) {
+    RootedNativeObject obj(cx, newObject());
+    RootedValue iteratorResult(cx, yield.iteratorResult);
+    if (!obj || !wrap(&iteratorResult) ||
+        !add(obj, cx->names().return_, iteratorResult) ||
+        !add(obj, cx->names().yield, TrueHandleValue)) {
+      return false;
+    }
+    result.setObject(*obj);
+    return true;
+  }
+
+  bool operator()(const Completion::Await& await) {
+    RootedNativeObject obj(cx, newObject());
+    RootedValue awaitee(cx, await.awaitee);
+    if (!obj || !wrap(&awaitee) || !add(obj, cx->names().return_, awaitee) ||
+        !add(obj, cx->names().await, TrueHandleValue)) {
+      return false;
+    }
+    result.setObject(*obj);
+    return true;
+  }
+
+ private:
+  NativeObject* newObject() const {
+    return NewBuiltinClassInstance<PlainObject>(cx);
+  }
+
+  bool add(HandleNativeObject obj, PropertyName* name,
+           HandleValue value) const {
+    return NativeDefineDataProperty(cx, obj, name, value, JSPROP_ENUMERATE);
+  }
+
+  bool wrap(MutableHandleValue v) const {
+    return dbg->wrapDebuggeeValue(cx, v);
+  }
+
+  // Saved stacks are wrapped for direct consumption by debugger code.
+  bool wrapStack(MutableHandleValue stack) const {
+    return cx->compartment()->wrap(cx, stack);
+  }
+};
+
+bool Completion::buildCompletionValue(JSContext* cx, Debugger* dbg,
+                                      MutableHandleValue result) const {
+  return variant.match(BuildValueMatcher(cx, dbg, result));
+}
+
+AbstractGeneratorObject* Completion::maybeGeneratorObject() const {
+  if (variant.is<InitialYield>()) {
+    return variant.as<InitialYield>().generatorObject;
+  }
+
+  if (variant.is<Yield>()) {
+    return variant.as<Yield>().generatorObject;
+  }
+
+  if (variant.is<Await>()) {
+    return variant.as<Await>().generatorObject;
+  }
+
+  return nullptr;
+}
+
+void Completion::updateForNextHandler(ResumeMode resumeMode,
+                                      HandleValue value) {
+  switch (resumeMode) {
+    case ResumeMode::Continue:
+      // No change to how we'll resume.
+      break;
+
+    case ResumeMode::Throw:
+      // Since this is a new exception, the stack for the old one may not apply.
+      // If we extend resumption values to specify stacks, we could revisit
+      // this.
+      variant = Variant(Throw(value, nullptr));
+      break;
+
+    case ResumeMode::Terminate:
+      variant = Variant(Terminate());
+      break;
+
+    case ResumeMode::Return:
+      variant = Variant(Return(value));
+      break;
+
+    default:
+      MOZ_CRASH("invalid resumeMode value");
+  }
+}
+
+struct MOZ_STACK_CLASS Completion::ToResumeModeMatcher {
+  MutableHandleValue value;
+  MutableHandleSavedFrame exnStack;
+  ToResumeModeMatcher(MutableHandleValue value,
+                      MutableHandleSavedFrame exnStack)
+      : value(value), exnStack(exnStack) {}
+
+  ResumeMode operator()(const Return& ret) {
+    value.set(ret.value);
+    return ResumeMode::Return;
+  }
+
+  ResumeMode operator()(const Throw& thr) {
+    value.set(thr.exception);
+    exnStack.set(thr.stack);
+    return ResumeMode::Throw;
+  }
+
+  ResumeMode operator()(const Terminate& term) {
+    value.setUndefined();
+    return ResumeMode::Terminate;
+  }
+
+  ResumeMode operator()(const InitialYield& initialYield) {
+    value.setObject(*initialYield.generatorObject);
+    return ResumeMode::Return;
+  }
+
+  ResumeMode operator()(const Yield& yield) {
+    value.set(yield.iteratorResult);
+    return ResumeMode::Return;
+  }
+
+  ResumeMode operator()(const Await& await) {
+    value.set(await.awaitee);
+    return ResumeMode::Return;
+  }
+};
+
+void Completion::toResumeMode(ResumeMode& resumeMode, MutableHandleValue value,
+                              MutableHandleSavedFrame exnStack) const {
+  resumeMode = variant.match(ToResumeModeMatcher(value, exnStack));
+}
+
+/* static */
 void Debugger::resultToCompletion(JSContext* cx, bool ok, const Value& rv,
                                   ResumeMode* resumeMode,
                                   MutableHandleValue value,
                                   MutableHandleSavedFrame exnStack) {
   MOZ_ASSERT_IF(ok, !cx->isExceptionPending());
 
   if (ok) {
     *resumeMode = ResumeMode::Return;
@@ -8918,49 +9152,29 @@ void ScriptedOnPopHandler::drop() {
   js_free(this);
 }
 
 void ScriptedOnPopHandler::trace(JSTracer* tracer) {
   TraceEdge(tracer, &object_, "OnStepHandlerFunction.object");
 }
 
 bool ScriptedOnPopHandler::onPop(JSContext* cx, HandleDebuggerFrame frame,
-                                 ResumeMode& resumeMode, MutableHandleValue vp,
-                                 HandleSavedFrame exnStack) {
-  Debugger* dbg = frame->owner();
-
-  // Make it possible to distinguish 'return' from 'await' completions.
-  // Bug 1470558 will investigate a more robust solution.
-  bool isAfterAwait = false;
-  AbstractFramePtr referent = DebuggerFrame::getReferent(frame);
-  if (resumeMode == ResumeMode::Return && referent &&
-      referent.isFunctionFrame() && referent.callee()->isAsync() &&
-      !referent.callee()->isGenerator()) {
-    AutoRealm ar(cx, referent.callee());
-    if (frame->hasGenerator()) {
-      AbstractGeneratorObject& genObj = frame->unwrappedGenerator();
-      isAfterAwait = !genObj.isClosed() && genObj.isRunning();
-    }
-  }
-
-  RootedValue completion(cx);
-  if (!dbg->newCompletionValue(cx, resumeMode, vp, exnStack, &completion)) {
-    return false;
-  }
-
-  if (isAfterAwait) {
-    RootedObject obj(cx, &completion.toObject());
-    if (!DefineDataProperty(cx, obj, cx->names().await, TrueHandleValue)) {
-      return false;
-    }
+                                 const Completion& completion,
+                                 ResumeMode& resumeMode,
+                                 MutableHandleValue vp) {
+  Debugger* dbg = Debugger::fromChildJSObject(frame);
+
+  RootedValue completionValue(cx);
+  if (!completion.buildCompletionValue(cx, dbg, &completionValue)) {
+    return false;
   }
 
   RootedValue fval(cx, ObjectValue(*object_));
   RootedValue rval(cx);
-  if (!js::Call(cx, fval, frame, completion, &rval)) {
+  if (!js::Call(cx, fval, frame, completionValue, &rval)) {
     return false;
   }
 
   return ParseResumptionValue(cx, rval, resumeMode, vp);
 };
 
 bool DebuggerFrame::resume(const FrameIter& iter) {
   FrameIter::Data* data = iter.copyData();
--- a/js/src/vm/Debugger.h
+++ b/js/src/vm/Debugger.h
@@ -16,25 +16,34 @@
 
 #include "ds/TraceableFifo.h"
 #include "gc/Barrier.h"
 #include "gc/WeakMap.h"
 #include "js/Debug.h"
 #include "js/GCVariant.h"
 #include "js/HashTable.h"
 #include "js/Promise.h"
+#include "js/RootingAPI.h"
 #include "js/Utility.h"
 #include "js/Wrapper.h"
 #include "proxy/DeadObjectProxy.h"
+#include "vm/GeneratorObject.h"
 #include "vm/GlobalObject.h"
 #include "vm/JSContext.h"
 #include "vm/Realm.h"
 #include "vm/SavedStacks.h"
 #include "vm/Stack.h"
 
+/*
+ * Windows 3.x used a cooperative multitasking model, with a Yield macro that
+ * let you relinquish control to other cooperative threads. Microsoft replaced
+ * it with an empty macro long ago. We should be free to use it in our code.
+ */
+#undef Yield
+
 namespace js {
 
 class AbstractGeneratorObject;
 class Breakpoint;
 class DebuggerMemory;
 class PromiseObject;
 class ScriptedOnStepHandler;
 class ScriptedOnPopHandler;
@@ -82,16 +91,175 @@ enum class ResumeMode {
   /**
    * Force the debuggee to return from the current frame.
    *
    * This corresponds to a resumption value of `{return: <value>}`.
    */
   Return,
 };
 
+/**
+ * A completion value, describing how some sort of JavaScript evaluation
+ * completed. This is used to tell an onPop handler what's going on with the
+ * frame, and to report the outcome of call, apply, setProperty, and getProperty
+ * operations.
+ *
+ * Local variables of type Completion should be held in Rooted locations,
+ * and passed using Handle and MutableHandle.
+ */
+class Completion {
+ public:
+  struct Return {
+    explicit Return(const Value& value) : value(value) {}
+    Value value;
+
+    void trace(JSTracer* trc) {
+      JS::UnsafeTraceRoot(trc, &value, "js::Completion::Return::value");
+    }
+  };
+
+  struct Throw {
+    Throw(const Value& exception, SavedFrame* stack)
+        : exception(exception), stack(stack) {}
+    Value exception;
+    SavedFrame* stack;
+
+    void trace(JSTracer* trc) {
+      JS::UnsafeTraceRoot(trc, &exception, "js::Completion::Throw::exception");
+      JS::UnsafeTraceRoot(trc, &stack, "js::Completion::Throw::stack");
+    }
+  };
+
+  struct Terminate {
+    void trace(JSTracer* trc) {}
+  };
+
+  struct InitialYield {
+    explicit InitialYield(AbstractGeneratorObject* generatorObject)
+        : generatorObject(generatorObject) {}
+    AbstractGeneratorObject* generatorObject;
+
+    void trace(JSTracer* trc) {
+      JS::UnsafeTraceRoot(trc, &generatorObject,
+                          "js::Completion::InitialYield::generatorObject");
+    }
+  };
+
+  struct Yield {
+    Yield(AbstractGeneratorObject* generatorObject, const Value& iteratorResult)
+        : generatorObject(generatorObject), iteratorResult(iteratorResult) {}
+    AbstractGeneratorObject* generatorObject;
+    Value iteratorResult;
+
+    void trace(JSTracer* trc) {
+      JS::UnsafeTraceRoot(trc, &generatorObject,
+                          "js::Completion::Yield::generatorObject");
+      JS::UnsafeTraceRoot(trc, &iteratorResult,
+                          "js::Completion::Yield::iteratorResult");
+    }
+  };
+
+  struct Await {
+    Await(AbstractGeneratorObject* generatorObject, const Value& awaitee)
+        : generatorObject(generatorObject), awaitee(awaitee) {}
+    AbstractGeneratorObject* generatorObject;
+    Value awaitee;
+
+    void trace(JSTracer* trc) {
+      JS::UnsafeTraceRoot(trc, &generatorObject,
+                          "js::Completion::Await::generatorObject");
+      JS::UnsafeTraceRoot(trc, &awaitee, "js::Completion::Await::awaitee");
+    }
+  };
+
+  // The JS::Result macros want to assign to an existing variable, so having a
+  // default constructor is handy.
+  Completion() : variant(Terminate()) {}
+
+  // Construct a completion from a specific variant.
+  //
+  // Unfortunately, using a template here would prevent the implicit definitions
+  // of the copy and move constructor and assignment operators, which is icky.
+  explicit Completion(Return&& variant)
+      : variant(std::forward<Return>(variant)) {}
+  explicit Completion(Throw&& variant)
+      : variant(std::forward<Throw>(variant)) {}
+  explicit Completion(Terminate&& variant)
+      : variant(std::forward<Terminate>(variant)) {}
+  explicit Completion(InitialYield&& variant)
+      : variant(std::forward<InitialYield>(variant)) {}
+  explicit Completion(Yield&& variant)
+      : variant(std::forward<Yield>(variant)) {}
+  explicit Completion(Await&& variant)
+      : variant(std::forward<Await>(variant)) {}
+
+  // Capture a JavaScript operation result as a Completion value. This clears
+  // any exception and stack from cx, taking ownership of them itself.
+  static Completion fromJSResult(JSContext* cx, bool ok, const Value& rv);
+
+  // Construct a completion given an AbstractFramePtr that is being popped. This
+  // clears any exception and stack from cx, taking ownership of them itself.
+  static Completion fromJSFramePop(JSContext* cx, AbstractFramePtr frame,
+                                   const jsbytecode* pc, bool ok);
+
+  template <typename V>
+  bool is() const {
+    return variant.template is<V>();
+  }
+
+  template <typename V>
+  V& as() {
+    return variant.template as<V>();
+  }
+
+  template <typename V>
+  const V& as() const {
+    return variant.template as<V>();
+  }
+
+  void trace(JSTracer* trc);
+
+  /* True if this completion is a suspension of a generator or async call. */
+  bool suspending() const {
+    return variant.is<InitialYield>() || variant.is<Yield>() ||
+           variant.is<Await>();
+  }
+
+  /*
+   * If this completion is a suspension of a generator or async call, return the
+   * call's generator object, nullptr otherwise.
+   */
+  AbstractGeneratorObject* maybeGeneratorObject() const;
+
+  /* Set `result` to a Debugger API completion value describing this completion.
+   */
+  bool buildCompletionValue(JSContext* cx, Debugger* dbg,
+                            MutableHandleValue result) const;
+
+  /*
+   * Set `resumeMode`, `value`, and `exnStack` to values describing this
+   * completion.
+   */
+  void toResumeMode(ResumeMode& resumeMode, MutableHandleValue value,
+                    MutableHandleSavedFrame exnStack) const;
+  /*
+   * Given a `ResumeMode` and value (typically derived from a resumption value
+   * returned by a Debugger hook), update this completion as requested.
+   */
+  void updateForNextHandler(ResumeMode resumeMode, HandleValue value);
+
+ private:
+  using Variant =
+      mozilla::Variant<Return, Throw, Terminate, InitialYield, Yield, Await>;
+  struct BuildValueMatcher;
+  struct ToResumeModeMatcher;
+
+  Variant variant;
+};
+
 typedef HashSet<WeakHeapPtrGlobalObject,
                 MovableCellHasher<WeakHeapPtrGlobalObject>, ZoneAllocPolicy>
     WeakGlobalObjectSet;
 
 #ifdef DEBUG
 extern void CheckDebuggeeThing(JSScript* script, bool invisibleOk);
 
 extern void CheckDebuggeeThing(LazyScript* script, bool invisibleOk);
@@ -1384,36 +1552,37 @@ class ScriptedOnStepHandler final : publ
 };
 
 /*
  * An OnPopHandler represents a handler function that is called just before a
  * frame is popped.
  */
 struct OnPopHandler : Handler {
   /*
-   * If a frame is about the be popped, this method is called with the frame
-   * as argument, and `resumeMode` and `vp` set to a completion value specifying
-   * how this frame's execution completed. If successful, this method should
-   * return true, with `resumeMode` and `vp` set to a resumption value
-   * specifying how execution should continue.
+   * The given `frame` is about to be popped; `completion` explains why.
+   *
+   * When this method returns true, it must set `resumeMode` and `vp` to a
+   * resumption value specifying how execution should continue.
+   *
+   * When this method returns false, it should set an exception on `cx`.
    */
   virtual bool onPop(JSContext* cx, HandleDebuggerFrame frame,
-                     ResumeMode& resumeMode, MutableHandleValue vp,
-                     HandleSavedFrame exnStack) = 0;
+                     const Completion& completion, ResumeMode& resumeMode,
+                     MutableHandleValue vp) = 0;
 };
 
 class ScriptedOnPopHandler final : public OnPopHandler {
  public:
   explicit ScriptedOnPopHandler(JSObject* object);
   virtual JSObject* object() const override;
   virtual void drop() override;
   virtual void trace(JSTracer* tracer) override;
   virtual bool onPop(JSContext* cx, HandleDebuggerFrame frame,
-                     ResumeMode& resumeMode, MutableHandleValue vp,
-                     HandleSavedFrame exnStack) override;
+                     const Completion& completion, ResumeMode& resumeMode,
+                     MutableHandleValue vp) override;
 
  private:
   HeapPtr<JSObject*> object_;
 };
 
 class DebuggerFrame : public NativeObject {
   friend class DebuggerArguments;
   friend class ScriptedOnStepHandler;