1471954 - Debugger {return:} resumption interacts badly with generators and async functions

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

Debugger callbacks may return an object `{return: val}` to force the current stack frame to return early. The sensible behavior is for `{return: val}` to behave as much like a `return` statement as possible, so

*   in a generator, before the initial yield, this should be an error.
    You can't return from a parameter default expression, and there's no
    reasonable behavior for it. The caller doesn't expect a value;
    it expects a generator object.

*   in a generator, after the initial yield, this should close the
    generator and the caller should receive `{done: true, value: val}`.

*   in an async function, before the initial suspend, this should
    cause the async function to return a resolved promise.

*   in an async function, after the initial suspend, this should
    cause the (already existing) promise to become resolved with
    the value `val`.

(Ideally, in all cases `finally` blocks would run, but that is a separate bug, bug 1470022.)

The current behavior is worse and pretty arbitrary. The fundamental underlying reason is that a `return` statement generates different bytecode when it's in a generator or async function, but js::Debugger makes no attempt to reproduce that distinctive behavior when you `{return:}`.

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: Change behavior of `{return:}` resumption values in generators

Before, the value had to conform to the iterator protocol. But this didn't
actually work; the generator was left in "running" state, so while you could
sort of simulate a `yield` by returning an object with `{done: true}`, the
generator would throw the next time it was used.

Now, `{return: value}` behaves as if the generator executed a `return`. The
generator is left in the "closed" state.

Before the initial yield, a forced return causes the generator to return a
value that may be silly. An enclosed test checks that it doesn't crash, but
please don't do that.

Comment 2

[Jason Orendorff [:jorendorff]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bb1843acb9cb1748c111e64908014061f9123447

Comment 3

[Jason Orendorff [:jorendorff]]

Attachment: Change behavior of `{return:}` resumption values in generators

Before, the value had to conform to the iterator protocol. But this didn't
actually work; the generator was left in "running" state, so while you could
sort of simulate a `yield` by returning an object with `{done: true}`, the
generator would throw the next time it was used.

Now, `{return: value}` behaves as if the generator executed a `return`. The
generator is left in the "closed" state.

Before the initial yield, a forced return causes the generator to return a
value that may be silly. An enclosed test checks that it doesn't crash, but
please don't do that.

Comment 4

[Jim Blandy :jimb]

Comment on attachment 8990840 [details] [diff] [review]
Change behavior of `{return:}` resumption values in generators

Review of attachment 8990840 [details] [diff] [review]:
-----------------------------------------------------------------

Really beautiful. I was in a daze yesterday and almost decided to just skim the tests; now I'm glad I didn't.

::: js/src/doc/Debugger/Conventions.md
@@ +142,5 @@
> +    <code>{ done: true, value: <i>value</i> }</code>.
> +
> +    Returning from a generator before the initial suspend—that is, from the
> +    first `onEnterFrame` event for a generator, or during evaluation of
> +    argument defaults for a generator—makes no sense. Don't do that.

I think this is unhelpful (and needlessly negative). We actually have a use case in mind for this, which is live function patching. Letting 'return' have this effect of returning a value untouched is actually exactly what's needed: if we grant that a generator function may only be patched by another generator function, then calling the replacement, and force-returning its generator object in place of the original's return value, has the desired effect.

So this should document the actual behavior: if the generator has completed its initial yield, this packs the given value in a { done, value } object; otherwise, it returns the given value directly.

Maybe we'd like to require that such a return value be a generator object, though.

::: js/src/jit-test/tests/debug/onEnterFrame-generator-resumption-05.js
@@ +1,5 @@
> +// Don't crash on {return:} from the initial onEnterFrame for a generator.
> +//
> +// We don't actually want this behavior. It's bizarre. But this is how it
> +// currently works. The generator returns the specified value instead of
> +// creating a new generator object.

I agree that the behavior *in this test* is undesirable, but if you were instead returning a different generator object, I don't see what's objectionable about that.

@@ +12,5 @@
> +
> +let dbg = Debugger(g);
> +
> +let hits = 0;
> +dbg.onEnterFrame = frame => {

Since onEnterFrame is so indiscriminate, would it be worth asserting that frame.callee.name === 'g'?

::: js/src/jit-test/tests/debug/onStep-generator-resumption-01.js
@@ +1,4 @@
> +// 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.

Excellent test!

@@ +12,5 @@
> +let dbg = Debugger(g);
> +
> +function test(ttl) {
> +    let hits = 0;
> +    dbg.onEnterFrame = frame => {

Similar thought about indiscriminate onEnterFrame

::: js/src/vm/Debugger.cpp
@@ +944,5 @@
>      Rooted<GeneratorObject*> genObj(cx);
>      if (frame.isFunctionFrame() && frame.callee()->isGenerator()) {
>          genObj = GetGeneratorObjectForFrame(cx, frame);
>          if (genObj) {
> +            if (!genObj->isBeforeInitialYield() && !genObj->isClosed() && genObj->isSuspended()) {

I feel like this condition has some redundancy; if the generator is suspended, it can't be closed, right? (Certainly the functions in GeneratorObject.h behave this way.)

@@ +1469,5 @@
> +        // that means doing the work below. It's only 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.

HEAR HEAR

Comment 5

[Jim Blandy :jimb]

Comment on attachment 8991146 [details] [diff] [review]
Change behavior of `{return:}` resumption values in generators

Review of attachment 8991146 [details] [diff] [review]:
-----------------------------------------------------------------

This one looks good too.

::: js/src/jit-test/tests/debug/onExceptionUnwind-resumption-generator.js
@@ +48,3 @@
>  dbg.onExceptionUnwind = function(frame) {
>      currentFrame = frame;
> +    return {declaim: "gadzooks"};

yay

Comment 6

[Jason Orendorff [:jorendorff]]

> > +            if (!genObj->isBeforeInitialYield() && !genObj->isClosed() && genObj->isSuspended()) {
>
> I feel like this condition has some redundancy; if the generator is suspended,
> it can't be closed, right? (Certainly the functions in GeneratorObject.h behave
> this way.)

Yes. However, the methods in GeneratorObject.h are chock full of assertions to punish you for running the state machine backwards or upside down--OR, asking a question that makes it sound suspiciously like you have no idea what you're doing or what state the generator is in, and you're flailing.

In our case, we are in both bad categories, but I think it's unavoidable. To placate the assertions, this code checks that it's safe to call isSuspended() before calling isSuspended().

Comment 7

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/09d4547a9714933fc3d2a51b499048dc0ae819c5

Comment 8

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/09d4547a9714