1448880 - Preserve Debugger.Frame identity across suspend/resume (yield or await)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P1)

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: Part 0: Add some passing tests

Also deletes two tests that are completely redundant ever since we removed
legacy generators.

Comment 2

[Jason Orendorff [:jorendorff]]

Comment on attachment 8991779 [details] [diff] [review]
Part 0: Add some passing tests

This patch was meant for another bug.

Comment 3

[Jason Orendorff [:jorendorff]]

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

Comment 4

[Jason Orendorff [:jorendorff]]

Attachment: Persist Debugger.Frame objects for generators across yield/await

Comment 5

[Jason Orendorff [:jorendorff]]

Attachment: Part 1: Always close a generator on early forced return

The previous code failed to close the generator in the case where
JSOP_GENERATOR had run but JSOP_INITIAL_YIELD had not, a bit of sloppiness that
created yet another special case. Things will get more complicated when we
start keeping frames live while suspended; it's better to not have this special
case.

Comment 6

[Jason Orendorff [:jorendorff]]

Attachment: Part 1: Always close a generator on early forced return

The previous code failed to close the generator in the case where
JSOP_GENERATOR had run but JSOP_INITIAL_YIELD had not, a bit of sloppiness that
created yet another special case. Things will get more complicated when we
start keeping frames live while suspended; it's better to not have this special
case.

Comment 7

[Jason Orendorff [:jorendorff]]

Attachment: Part 3: AbstractFramePtr::isGeneratorFrame()

This proves handy in several places, later in the stack.

Comment 8

[Jason Orendorff [:jorendorff]]

Attachment: Part 2: Persist Debugger.Frame objects for generators across yield/await

This is the minimal patch, but it leaves two bugs:

1.  .onStep and .onPop hooks on suspended Frames do not survive GC if there are
    no other references to the Frame or the Debugger object. The behavior is safe,
    but the hooks can just mysteriously stop firing when GC happens.

2.  When a generator or async function is resumed, stepping is reenabled in
    Debugger::getFrame, which isn't necessarily called. The onStep tests in
    this patch work because they all use an onEnterFrame hook, which causes
    getFrame to be called as soon as the generator is resumed.

The remaining patches in this stack fix these two bugs.

Comment 9

[Jason Orendorff [:jorendorff]]

Attachment: Part 2: Persist Debugger.Frame objects for generators across yield/await

This is the minimal patch, but it leaves two bugs:

1.  When a generator or async function is resumed, stepping is reenabled in
    Debugger::getFrame, which isn't necessarily called. The onStep tests in
    this patch work because they all use an onEnterFrame hook, which causes
    getFrame to be called as soon as the generator is resumed.

2.  .onStep and .onPop hooks on suspended Frames do not survive GC if there are
    no other references to the Frame or the Debugger object. The behavior is safe,
    but the hooks can just mysteriously stop firing when GC happens.

The next three patches in this stack lay the groundwork for fixing these bugs,
without changing behavior; part 6 fixes the first bug; and part 7 fixes the second.

Comment 10

[Jason Orendorff [:jorendorff]]

Attachment: Part 3: AbstractFramePtr::isGeneratorFrame()

This proves handy in several places, later in the stack.

Comment 11

[Jason Orendorff [:jorendorff]]

Attachment: Part 4: Split Debugger::onResumeFrame from onEnterFrame

Pure refactoring, no change in behavior. This is in anticipation of doing
additional work in onResumeFrame.

Comment 12

[Jason Orendorff [:jorendorff]]

Attachment: Part 5: Tell removeFromFrameMapsAndClearBreakpointsIn() if we are suspending

Since the argument is not used yet, this too is a pure refactoring, with no
change in behavior yet.

Comment 13

[Jim Blandy :jimb]

Comment on attachment 8992109 [details] [diff] [review]
Persist Debugger.Frame objects for generators across yield/await

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

This one is obsolete, but all these comments are on the tests, so I'll post them right now. (i.e. this is not an r+)

How about testing onExceptionUnwind and getNewestFrame?

We should document that initial onEnterFrame/onPop that both generators and async functions cause, for default argument evaluation (and destructuring, I guess?). We should also document that async functions then immediately re-enter the frame and begin executing the body, whereas generators do not.

We should request fuzzer attention, once this lands.

::: js/src/jit-test/tests/debug/Frame-identity-06.js
@@ +1,2 @@
> +// Debugger.Frames for async functions are not GC'd while they're suspended.
> +// The event queue (...or something) keeps them gc-alive.

This seems like a step away from what you actually want to say. They only stay alive as long as the generator object is alive, right? That is, if an async function awaits a promise which is dropped without ever being resolved, then the Debugger.Frame could be GC'd, right?

@@ +4,5 @@
> +var g = newGlobal();
> +g.eval(`
> +    async function f() {
> +        gc(); debugger;
> +        gc(); await 1;

In this case, it's awaiting a resolved promise, so it's immediately dispatched into the job queue, which is what keeps it alive. Seems worth commenting.

@@ +5,5 @@
> +g.eval(`
> +    async function f() {
> +        gc(); debugger;
> +        gc(); await 1;
> +        gc(); await Promise.resolve(1);

This amounts to exactly what you did on the previous line, with just another dependent promise in the mix. It would be a better test to actually await a non-resolved promise across a GC.

::: js/src/jit-test/tests/debug/Frame-identity-07.js
@@ +34,5 @@
> +    log += " ";
> +}
> +
> +assertEq(log,
> +         "0[]00[1[]11[2[]22[3[]33[4[]44[5[]55[]5]4]3]2]1]0 " +

Could we break the first 0[]0 onto its own line, put a comment like the following here?

Calling a generator function just returns a generator object without running the body at all; hence "0[]". However, this call does evaluate default argument values, if any, so we do report an onEnterFrame / onPop for it.

Then, the second 0[...]0 should have the comment:

Demanding the first value from the top generator forces SpiderMonkey to create all five generator objects (the empty "n[]n" pairs) and then demand a value from them (the longer "n[...]n" substrings).

::: js/src/jit-test/tests/debug/Frame-older-generators-01.js
@@ +1,5 @@
> +// Generator/async frames can be created by following .older.
> +//
> +// The goal here is to get some test coverage creating generator Frame objects
> +// at some time other than when firing onEnterFrame. Here they're created after
> +// the initial yield.

Just for kicks, can we call f from an async or generator function's default arguments?

@@ +13,5 @@
> +        f();
> +        yield 1;
> +        f();
> +    }
> +    async function af() {

"How 'async function' is this? It's ..."

(I blame social media, not my own choices about how to use the web)

::: js/src/jit-test/tests/debug/Frame-onStep-async-01.js
@@ +3,5 @@
> +// Set up debuggee.
> +var g = newGlobal();
> +g.log = "";
> +g.eval(`                              // line 1
> +async function aloop() {              // 2

This is really nice.

::: js/src/jit-test/tests/debug/Frame-onStep-async-02.js
@@ +36,5 @@
> +    assertEq(frame.type, "call");
> +
> +    // If we're entering this frame for the first time, push it to the async
> +    // stack.
> +    if (!("seen" in frame)) {

I think simply `if (frame.seen)` is fine, isn't it?

@@ +44,5 @@
> +    }
> +
> +    frame.onStep = () => {
> +        // When stepping, we sometimes pause at opcodes in older frames (!)
> +        // where all that's happening is async function administrivia.

I think it'd be okay to go into a little more detail here, especially if you want this to serve as an illustration. I'm really curious now about this "administriva" and why Debugger reveals it; if it's stuff that is actually well-motivated once you think through what an async call really means, then that's very much worth explaining.

I wish we had a better place for explanations like this. The Debugger docs are really mostly just reference.

@@ +57,5 @@
> +        }
> +    };
> +
> +    frame.onPop = completion => {
> +        if (typeof completion.return === "string") {

I think `if ('return' in completion)` would be a little better.

::: js/src/jit-test/tests/debug/onEnterFrame-async-01.js
@@ +28,5 @@
> +drainJobQueue();
> +assertEq(log,
> +         "(job)(job(t5)(t5)(t3)(t3))" +
> +         "(t5)(t3)".repeat(3) + "(job)" +
> +         "(t5)(t5)(job)");

Really interesting test.

Comment 14

[Jason Orendorff [:jorendorff]]

Attachment: Part 6: Re-enable stepping when an async or generator frame with an .onStep hook is resumed

That is, don't put it off until Debugger::getFrame() is called. The effect is
subtle, as indicated by the test changes: the onEnterFrame hooks in those tests
were causing getFrame to be called very early during generator resumption,
which made the tests pass.

With this patch, we no longer adjust the step mode count when suspending or
resuming. This change is necessary to make the frame->isDebuggee() call in
Debugger::onResumeFrame the right criterion for calling slowPathOnResumeFrame.
It's true if the step mode count on the script is nonzero. (This approach also
simplifies error handling, as resuming a Debugger.Frame is now idempotent: we
don't have to worry about adjusting the step mode count too much or not enough
on error.)

Comment 15

[Jason Orendorff [:jorendorff]]

Attachment: Part 7: Fix hasAnyLiveHooks to include onStep handlers on suspended frames

Comment 16

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8998019 [details] [diff] [review]
Part 7: Fix hasAnyLiveHooks to include onStep handlers on suspended frames

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

Lusciously delicious tests.

::: js/src/vm/Debugger.cpp
@@ +934,1 @@
>              return true;

I agree with your comment that this should be able to use Range and const.

You could patch it here with

  JSObject* key = const_cast<JSObject*>(e.front().key());
  if (IsMarked(rt, &key) && frameObj.hasAnyLiveHooks())
    return true;
  MOZ_ASSERT(key == e.front().key()); // Validate the const_cast above.

and that's perhaps the path of least resistance. We will not modify the key because we only do that when the key is moved, and in that case Debugger::traceForMovingGC will have marked everything indiscriminately and updated them so that markIteratively will always see everything in its new location.

Some form of that should be boiled down to a comment longer than the "// Validate the const_cast above" that I gave. Perhaps just

  // Relocated WeakMap keys are only observed during markIteratively in their new locations.

? Though a grammar nazi friend of mine would complain that 'only' is referring to 'observed'...

Comment 17

[Jason Orendorff [:jorendorff]]

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

Comment 18

[Jason Orendorff [:jorendorff]]

Attachment: Part 2a: Documentation

This will be folded into part 2.

Comment 19

[Jason Orendorff [:jorendorff]]

(In reply to Steve Fink [:sfink] [:s:] from comment #16)
> You could patch it here with
> 
>   JSObject* key = const_cast<JSObject*>(e.front().key());
>   if (IsMarked(rt, &key) && frameObj.hasAnyLiveHooks())

The first line works fine--even without the const_cast, it turns out--but:

In file included from /Users/jorendorff/work/gecko/js/src/build_DBG.OBJ/js/src/Unified_cpp_js_src32.cpp:47:
/Users/jorendorff/work/gecko/js/src/vm/Debugger.cpp:936:13: error: no matching function for call to 'IsMarked'
        if (IsMarked(rt, &key) && frameObj->hasAnyLiveHooks())
            ^~~~~~~~
/Users/jorendorff/work/gecko/js/src/jit/IonCode.h:774:1: note: candidate function not viable: no known conversion from 'JSObject **' to 'const jit::VMFunction *' for 2nd
      argument
IsMarked(JSRuntime* rt, const jit::VMFunction*)
^
/Users/jorendorff/work/gecko/js/src/gc/Marking.h:93:1: note: candidate template ignored: could not match 'WriteBarrieredBase<type-parameter-0-0>' against 'JSObject *'
IsMarked(JSRuntime* rt, WriteBarrieredBase<T>* thingp)

Comment 20

[Jason Orendorff [:jorendorff]]

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

Comment 21

[Steve Fink [:sfink] [:s:]]

Right, sorry, I guess that'd be IsMarkedUnbarriered().

Comment 22

[Jim Blandy :jimb]

Comment on attachment 8997580 [details] [diff] [review]
Part 1: Always close a generator on early forced return

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

::: js/src/jit-test/tests/debug/Frame-onStep-generator-resumption-01.js
@@ +19,5 @@
> +            if (ttl == 0) {
> +                exiting = true;
> +                // Forced return here causes the generator object, if any, not
> +                // to be exposed.
> +                return {return: gw};

Is there some reason this return value must be an object? If not, I think it would be better to return some distinctive value, like a string, not something that could plausibly arise via a bug, like g.

@@ +35,5 @@
> +        assertEq(result instanceof g.f, true);
> +    else
> +        assertEq(result, g);
> +    
> +

extra blank line, stray tab

Comment 23

[Jason Orendorff [:jorendorff]]

(In reply to Jim Blandy :jimb from comment #22)
> ::: js/src/jit-test/tests/debug/Frame-onStep-generator-resumption-01.js
> > +                return {return: gw};
> 
> Is there some reason this return value must be an object?

No. I thought it had to be an object when I wrote the test. Changed to a distinctive string.

> extra blank line, stray tab

Fixed.

Comment 24

[Jason Orendorff [:jorendorff]]

> ::: js/src/jit-test/tests/debug/Frame-onStep-async-02.js
> > +    // If we're entering this frame for the first time, push it to the async
> > +    // stack.
> > +    if (!("seen" in frame)) {
> 
> I think simply `if (frame.seen)` is fine, isn't it?

Switched to `if (!frame.seen)`.

> @@ +44,5 @@
> > +    frame.onStep = () => {
> > +        // When stepping, we sometimes pause at opcodes in older frames (!)
> > +        // where all that's happening is async function administrivia.
> 
> I think it'd be okay to go into a little more detail here, especially if you
> want this to serve as an illustration. I'm really curious now about this
> "administriva" and why Debugger reveals it; if it's stuff that is actually
> well-motivated once you think through what an async call really means, then
> that's very much worth explaining.
> 
> I wish we had a better place for explanations like this. The Debugger docs
> are really mostly just reference.

Revised:

        // When stepping, we sometimes pause at opcodes in older frames (!)
        // where all that's happening is async function administrivia.
        //
        // For example, the first time `leaf()` yields, `inner()` and
        // `outer()` are still on the stack; they haven't awaited yet because
        // control has not returned from `leaf()` to them yet. So stepping will
        // hop from line 8 to line 5 to line 2 as we unwind the stack, then
        // resume on line 8.
        //
        // Anyway: skip that noise.

> > +    frame.onPop = completion => {
> > +        if (typeof completion.return === "string") {
> 
> I think `if ('return' in completion)` would be a little better.

This isn't possible because of bug 1470558.

More tomorrow.

Comment 25

[Jim Blandy :jimb]

Comment on attachment 8997886 [details] [diff] [review]
Part 2: Persist Debugger.Frame objects for generators across yield/await

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

Okay, I've finished looking this over. Most of it looks great. Most important concerns:
- the wrapper map not being updated in the onNewGenerator case
- the potential missed cleanup in Debugger::getFrame

::: js/src/jit-test/tests/debug/onEnterFrame-generator-01.js
@@ +25,5 @@
>      // `arguments` elements don't work in resumed generator frames,
>      // because generators don't keep the arguments around.
> +    // However, some of this is initialized when the frame.arguments object is
> +    // created, so if they are created during the first onEnterFrame or onPop
> +    // event, the properties exist, and those events can also see the values.

So if we touch D.F.p.arguments in the onEnterFrame handler, then later handlers can see them, but otherwise they might be missing?? People are going to love coding to those rules. This definitely needs to go into the documentation.

If Debugger can fake up arguments for ordinary functions that never use `arguments`, why can't it do it for generators? Is it because they don't save that portion of their stack, the way they do for GeneratorObject::EXPRESSION_STACK_SLOT?

::: js/src/vm/Debugger.cpp
@@ +804,5 @@
> +                gp = generatorFrames.lookupForAdd(genObj);
> +                if (gp) {
> +                    frame = &gp->value()->as<DebuggerFrame>();
> +
> +                    // This frame needs to be revived.

I think I would like this comment expanded a bit. It took me a bit to realize that, since !p, this is the first time this Debugger has ever encountered this AbstractFramePtr (as opposed to this 'frame'), and therefore the D.F's prior referent must have been popped, and therefore the D.F must be dead.

@@ +817,5 @@
> +
> +                    RootedValue onStep(cx, frame->getReservedSlot(JSSLOT_DEBUGFRAME_ONSTEP_HANDLER));
> +                    if (!onStep.isUndefined()) {
> +                        AutoRealm ar(cx, genObj);
> +                        if (!ensureExecutionObservabilityOfScript(cx, referent.script()))

Suggestion only: Could this be wrapped up somewhere in a "Congratulations, now you've got an onStep handler!" function? It might be nice to share this code with DebuggerFrame::setOnStepHandler.

@@ +819,5 @@
> +                    if (!onStep.isUndefined()) {
> +                        AutoRealm ar(cx, genObj);
> +                        if (!ensureExecutionObservabilityOfScript(cx, referent.script()))
> +                            return false;
> +                        if (!referent.script()->incrementStepModeCount(cx))

Do we need to undo this if the insertion into Debugger::frames below fails?

I think I might be more comfortable with the entire 'reviving extant D.F' path in a separate function, sharing no common tail with the 'creating new D.F' path. Things like `ensureExecutionObservabilityOfFrame` need to be separated out anyway. And it might make it easier to manage reverting the fallible operations.

@@ +1110,5 @@
>  }
>  
> +/* static */ bool
> +Debugger::slowPathOnNewGenerator(JSContext* cx, AbstractFramePtr frame,
> +                                 Handle<GeneratorObject*> genObj)

Does it matter that this function doesn't put the CrossCompartmentKey::DebuggerGeneratorFrame entry in the compartment's wrapper table, the way getFrame does? If not, why not? Needs a comment, at least.

@@ +1122,5 @@
> +        // expressions are evaluated and well after onEnterFrame, so
> +        // Debugger.Frame objects for `frame` may already have been exposed to
> +        // debugger code. The GeneratorObject for this generator call, though,
> +        // has just been created. It must be associated with any existing
> +        // Debugger.Frames.

It feels like this comment belongs at the head of the function, above the forEachDebuggerFrame call.

::: js/src/vm/Debugger.h
@@ +195,5 @@
>      }
>  
> +    // Remove entries whose keys satisfy the given predicate.
> +    template <typename Predicate>
> +    void removeIf(Predicate test) {

This made me think it removed a specific element. `removeAll` would be consistent with how Rust iterators and JavaScript array methods `all`... just a suggestion.

@@ +561,5 @@
> +     *
> +     * An entry is present in this table when:
> +     * -   both the debuggee generator object and the Debugger.Frame object exist
> +     * -   the Debugger.Frame's owner is still an enabled debugger of
> +     *     the debuggee compartment

Note that the comment on DebuggerWeakMap, in the last paragraph, says that entries cannot be deleted when their keys' compartments' are not debuggees, in the case of add/remove/add transitions. I know we've always made Debugger.Frame a special case, in that we do lose their identity when we remove their referent compartment as a debuggee, so I think the behavior described here is fine. But the comment on DebuggerWeakMap should probably be updated to note the exception.

Comment 26

[Jim Blandy :jimb]

(In reply to Jim Blandy :jimb from comment #25)
> > +                    RootedValue onStep(cx, frame->getReservedSlot(JSSLOT_DEBUGFRAME_ONSTEP_HANDLER));
> > +                    if (!onStep.isUndefined()) {
> > +                        AutoRealm ar(cx, genObj);
> > +                        if (!ensureExecutionObservabilityOfScript(cx, referent.script()))
> 
> Suggestion only: Could this be wrapped up somewhere in a "Congratulations,
> now you've got an onStep handler!" function? It might be nice to share this
> code with DebuggerFrame::setOnStepHandler.

I see that later patches do gather this stuff up.

Comment 27

[Jim Blandy :jimb]

(In reply to Jim Blandy :jimb from comment #25)
> Comment on attachment 8997886 [details] [diff] [review]
> Part 2: Persist Debugger.Frame objects for generators across yield/await
> 
> @@ +819,5 @@
> > +                    if (!onStep.isUndefined()) {
> > +                        AutoRealm ar(cx, genObj);
> > +                        if (!ensureExecutionObservabilityOfScript(cx, referent.script()))
> > +                            return false;
> > +                        if (!referent.script()->incrementStepModeCount(cx))
> 
> Do we need to undo this if the insertion into Debugger::frames below fails?

Oh, it seems like Part 6 corrects this problem (assuming it is indeed a problem), in that scripts' step mode counts are not adjusted as generator frames are pushed and popped.

Comment 28

[Jim Blandy :jimb]

Comment on attachment 8997962 [details] [diff] [review]
Part 6: Re-enable stepping when an async or generator frame with an .onStep hook is resumed

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

Do we test anywhere that, when resuming a generator causes us to call one Debugger's onEnterFrame handler, another Debugger's Debugger.Frame gets marked as live, and becomes usable? That is, do we check for the existence of that loop in slowPathOnResumeFrame?

::: js/src/jit-test/tests/debug/Frame-onStep-async-01.js
@@ +24,5 @@
>              g.log += line; // We stepped to a new line.
>              previousLine = line;
>          }
>      };
> +    dbg.onEnterFrame = undefined;

So the idea here is that once we've set the onStep handler, we can drop the onEnterFrame handler and the frames will still be recognized as debuggee frames?

::: js/src/vm/Debugger.cpp
@@ +982,5 @@
> +
> +    // For each debugger, if there is an existing Debugger.Frame object for the
> +    // resumed `frame`, update it with the new frame pointer and make sure the
> +    // frame is observable.
> +    if (GlobalObject::DebuggerVector* debuggers = frame.callee()->global().getDebuggers()) {

I think `AbstractFramePtr` has a `global` method you can call directly, instead of `frame.callee()->global()`.

Comment 29

[Jason Orendorff [:jorendorff]]

Memo to myself: I want to try to write a test that fails because of the `!genObj->isBeforeInitialYield()` on this line of code:

                if (!genObj->isBeforeInitialYield() && !genObj->isClosed() && genObj->isSuspended()) {

I think it's wrong, because (I think) the `.onPop` event's argument will expose the generator object to the debugger. It should be deleted (leaving the other two conditions).

I don't remember why I wrote it like that. Probably to silence some test failure that I later had to fix some other, better way. I'm so bad at this.

Comment 30

[Jim Blandy :jimb]

You're the best!

Comment 31

[Jason Orendorff [:jorendorff]]

(In reply to Jim Blandy :jimb from comment #13)
> How about testing onExceptionUnwind and getNewestFrame?
> 
> We should document that initial onEnterFrame/onPop that both generators and
> async functions cause, for default argument evaluation (and destructuring, I
> guess?). We should also document that async functions then immediately
> re-enter the frame and begin executing the body, whereas generators do not.
> 
> We should request fuzzer attention, once this lands.

I've put all of this onto my TODO checklist for this bug.

> ::: js/src/jit-test/tests/debug/Frame-identity-06.js
> > +// Debugger.Frames for async functions are not GC'd while they're suspended.
> > +// The event queue (...or something) keeps them gc-alive.
> 
> This seems like a step away from what you actually want to say. They only
> stay alive as long as the generator object is alive, right? That is, if an
> async function awaits a promise which is dropped without ever being
> resolved, then the Debugger.Frame could be GC'd, right?

I changed this comment to read:

    // Debugger.Frames for async functions are not GC'd while they're suspended.
    // The awaited promise keeps the generator alive, via its reaction lists.

> > +var g = newGlobal();
> > +g.eval(`
> > +    async function f() {
> > +        gc(); debugger;
> > +        gc(); await 1;
> 
> In this case, it's awaiting a resolved promise, so it's immediately
> dispatched into the job queue, which is what keeps it alive. Seems worth
> commenting.
>
> @@ +5,5 @@
> > +g.eval(`
> > +    async function f() {
> > +        gc(); debugger;
> > +        gc(); await 1;
> > +        gc(); await Promise.resolve(1);
> 
> This amounts to exactly what you did on the previous line, with just another
> dependent promise in the mix. It would be a better test to actually await a
> non-resolved promise across a GC.

Agreed. After realizing that the async function doesn't even suspend in this case, I rewrote the test (Frame-identity-06.js) entirely. Now it creates nontrivial promises and explicitly resolves them one by one.

> ::: js/src/jit-test/tests/debug/Frame-identity-07.js
> @@ +34,5 @@
> > +    log += " ";
> > +}
> > +
> > +assertEq(log,
> > +         "0[]00[1[]11[2[]22[3[]33[4[]44[5[]55[]5]4]3]2]1]0 " +
> 
> Could we break the first 0[]0 onto its own line, put a comment like the
> following here?

Done.

> Calling a generator function just returns a generator object without running
> the body at all; hence "0[]". However, this call does evaluate default
> argument values, if any, so we do report an onEnterFrame / onPop for it.
> 
> Then, the second 0[...]0 should have the comment:
> 
> Demanding the first value from the top generator forces SpiderMonkey to
> create all five generator objects (the empty "n[]n" pairs) and then demand a
> value from them (the longer "n[...]n" substrings).

Added.

> ::: js/src/jit-test/tests/debug/Frame-older-generators-01.js
> @@ +1,5 @@
> > +// Generator/async frames can be created by following .older.
> > +//
> > +// The goal here is to get some test coverage creating generator Frame objects
> > +// at some time other than when firing onEnterFrame. Here they're created after
> > +// the initial yield.
> 
> Just for kicks, can we call f from an async or generator function's default
> arguments?

Added.

> ::: js/src/jit-test/tests/debug/Frame-onStep-async-02.js
> > +    if (!("seen" in frame)) {
> 
> I think simply `if (frame.seen)` is fine, isn't it?

Changed to `if (!frame.seen)`.

> @@ +44,5 @@
> > +    frame.onStep = () => {
> > +        // When stepping, we sometimes pause at opcodes in older frames (!)
> > +        // where all that's happening is async function administrivia.
> 
> I think it'd be okay to go into a little more detail here, especially if you
> want this to serve as an illustration. I'm really curious now about this
> "administriva" and why Debugger reveals it; if it's stuff that is actually
> well-motivated once you think through what an async call really means, then
> that's very much worth explaining.
> 
> I wish we had a better place for explanations like this. The Debugger docs
> are really mostly just reference.

Now it says:

    frame.onStep = () => {
        // When stepping, we sometimes pause at opcodes in older frames (!)
        // where all that's happening is async function administrivia.
        //
        // For example, the first time `leaf()` yields, `inner()` and
        // `outer()` are still on the stack; they haven't awaited yet because
        // control has not returned from `leaf()` to them yet. So stepping will
        // hop from line 8 to line 5 to line 2 as we unwind the stack, then
        // resume on line 8.
        //
        // Anyway: skip that noise.
        if (frame !== asyncStack[asyncStack.length - 1])
            return;


> @@ +57,5 @@
> > +    frame.onPop = completion => {
> > +        if (typeof completion.return === "string") {
> 
> I think `if ('return' in completion)` would be a little better.

Unfortunately this doesn't work, because of bug 1470558 (read the description closely...). I wrote a longer comment:

    frame.onPop = completion => {
        // Popping the frame. But async function frames are popped multiple
        // times: for the "initial suspend", at each await, and on return. The
        // debugger offers no easy way to distinguish them (bug 1470558).
        if (typeof completion.return === "string") {
            // Returning (not awaiting or at initial suspend).
            assertEq(asyncStack.pop(), frame);
            log += ")";
        }
    };

Comment 32

[Jason Orendorff [:jorendorff]]

(In reply to Jim Blandy :jimb from comment #25)
> Comment on attachment 8997886 [details] [diff] [review]
> Part 2: Persist Debugger.Frame objects for generators across yield/await
[...]
> ::: js/src/jit-test/tests/debug/onEnterFrame-generator-01.js
> @@ +25,5 @@
> >      // `arguments` elements don't work in resumed generator frames,
> >      // because generators don't keep the arguments around.
> > +    // However, some of this is initialized when the frame.arguments object is
> > +    // created, so if they are created during the first onEnterFrame or onPop
> > +    // event, the properties exist, and those events can also see the values.
> 
> So if we touch D.F.p.arguments in the onEnterFrame handler, then later
> handlers can see them, but otherwise they might be missing?? People are
> going to love coding to those rules. This definitely needs to go into the
> documentation.
> 
> If Debugger can fake up arguments for ordinary functions that never use
> `arguments`, why can't it do it for generators? Is it because they don't
> save that portion of their stack, the way they do for
> GeneratorObject::EXPRESSION_STACK_SLOT?

Yes, that is the reason. It's not cheap to save that portion of the stack, because of lifetime concerns: argv is borrowed from the caller. (For example, in the case of a simple, non-generator, JS-to-JS call, argv is a slice of the caller's operand stack.) You'd have to allocate some heap memory and copy it.

This lamentable behavior of `D.F.p.arguments` is pre-existing; it's bug 1445812.

> ::: js/src/vm/Debugger.cpp
> @@ +804,5 @@
> > +                gp = generatorFrames.lookupForAdd(genObj);
> > +                if (gp) {
> > +                    frame = &gp->value()->as<DebuggerFrame>();
> > +
> > +                    // This frame needs to be revived.
> 
> I think I would like this comment expanded a bit. It took me a bit to
> realize that, since !p, this is the first time this Debugger has ever
> encountered this AbstractFramePtr (as opposed to this 'frame'), and
> therefore the D.F's prior referent must have been popped, and therefore the
> D.F must be dead.

Now it says:

                    // We have found an existing Debugger.Frame object. But
                    // since it was previously popped (see comment above), it
                    // is not currently "live". We must revive it.

The previous comment hasn't changed; it says:

        // If this is a generator frame, there may be an existing
        // Debugger.Frame object that isn't in `frames` because the generator
        // was suspended, popping the stack frame, and later resumed.

What's going unsaid here is that

- popping a frame from the stack causes the AbstractFramePtr to become invalid
- that's why we have to remove the entry from the `frames` table on suspend
- but also, when it's re-pushed, it could very well have a different address on the stack
- and hence a different AbstractFramePtr
- so we couldn't just leave it in the `frames` table anyway
- but also, other stack frames could have been created at the same address in between
- so we couldn't just leave it in the `frames` table for that reason as well

However... I don't want to say *all* of that, and I'm not sure which pieces the reader is most likely to need.

> > +                        if (!ensureExecutionObservabilityOfScript(cx, referent.script()))
> 
> Suggestion only: Could this be wrapped up somewhere in a "Congratulations,
> now you've got an onStep handler!" function? It might be nice to share this
> code with DebuggerFrame::setOnStepHandler.

As you noticed, it ends up not being common code anymore, by part 6.

> @@ +819,5 @@
> > +                    if (!onStep.isUndefined()) {
> > +                        AutoRealm ar(cx, genObj);
> > +                        if (!ensureExecutionObservabilityOfScript(cx, referent.script()))
> > +                            return false;
> > +                        if (!referent.script()->incrementStepModeCount(cx))
> 
> Do we need to undo this if the insertion into Debugger::frames below fails?
> 
> I think I might be more comfortable with the entire 'reviving extant D.F'
> path in a separate function, sharing no common tail with the 'creating new
> D.F' path. Things like `ensureExecutionObservabilityOfFrame` need to be
> separated out anyway. And it might make it easier to manage reverting the
> fallible operations.

Wow, good catch. Fortunately (?), the entire thing gets rewritten in part 6, and the bug magically goes away.

Comment 33

[Jason Orendorff [:jorendorff]]

(In reply to Jim Blandy :jimb from comment #25)
> @@ +1110,5 @@
> > +/* static */ bool
> > +Debugger::slowPathOnNewGenerator(JSContext* cx, AbstractFramePtr frame,
> > +                                 Handle<GeneratorObject*> genObj)
> 
> Does it matter that this function doesn't put the
> CrossCompartmentKey::DebuggerGeneratorFrame entry in the compartment's
> wrapper table, the way getFrame does? If not, why not? Needs a comment, at
> least.

I've written a patch to add this. Still trying to write a test that illustrates the need for it. I should be able to make it assert during GC mark phase, I think.

Finishing this is on my to-do list for this bug.

Comment 34

[Jason Orendorff [:jorendorff]]

(In reply to Jason Orendorff [:jorendorff] from comment #33)
> I've written a patch to add this. Still trying to write a test that
> illustrates the need for it. I should be able to make it assert during GC
> mark phase, I think.

I am delighted to present the following correction: sfink and I are pretty sure the new kind of CrossCompartmentKey is not needed at all. In fact, maybe some of our existing kinds are not needed either (bug 1494121).

Comment 35

[Jason Orendorff [:jorendorff]]

(In reply to Jim Blandy :jimb from comment #25)
> @@ +1122,5 @@
> > +        // expressions are evaluated and well after onEnterFrame, so
> > +        // Debugger.Frame objects for `frame` may already have been exposed to
> > +        // debugger code. The GeneratorObject for this generator call, though,
> > +        // has just been created. It must be associated with any existing
> > +        // Debugger.Frames.
> 
> It feels like this comment belongs at the head of the function, above the
> forEachDebuggerFrame call.

Moved.

> ::: js/src/vm/Debugger.h
> @@ +195,5 @@
> >      }
> >  
> > +    // Remove entries whose keys satisfy the given predicate.
> > +    template <typename Predicate>
> > +    void removeIf(Predicate test) {
> 
> This made me think it removed a specific element. `removeAll` would be
> consistent with how Rust iterators and JavaScript array methods `all`...
> just a suggestion.

I left it for now. It's named after C++ STL std::remove_if.

I couldn't find any Rust or JavaScript methods that contain the word "all", except Rust's `iter.all()` which is "forall"--not what we're doing here. Rust has Vec::retain() which is the complement. (I don't really like that name anyway; not descriptive enough.) Maybe `removeKeysWhere`?

> @@ +561,5 @@
> > +     *
> > +     * An entry is present in this table when:
> > +     * -   both the debuggee generator object and the Debugger.Frame object exist
> > +     * -   the Debugger.Frame's owner is still an enabled debugger of
> > +     *     the debuggee compartment
> 
> Note that the comment on DebuggerWeakMap, in the last paragraph, says that
> entries cannot be deleted when their keys' compartments' are not debuggees,
> in the case of add/remove/add transitions. I know we've always made
> Debugger.Frame a special case, in that we do lose their identity when we
> remove their referent compartment as a debuggee, so I think the behavior
> described here is fine. But the comment on DebuggerWeakMap should probably
> be updated to note the exception.

Updated.

> -----------------------------------------------------------------
> 
> Okay, I've finished looking this over. Most of it looks great. Most
> important concerns:
> - the wrapper map not being updated in the onNewGenerator case

Very satisfactorily resolved (comment 34).

> - the potential missed cleanup in Debugger::getFrame

Resolved, none too satisfyingly, by doing nothing (last bit of comment 32).

Comment 36

[Jason Orendorff [:jorendorff]]

(In reply to Jason Orendorff [:jorendorff] from comment #29)
> Memo to myself: I want to try to write a test that fails because of the
> `!genObj->isBeforeInitialYield()` on this line of code:
> 
>                 if (!genObj->isBeforeInitialYield() && !genObj->isClosed()
> && genObj->isSuspended()) {
> 
> I think it's wrong, because (I think) the `.onPop` event's argument will
> expose the generator object to the debugger. It should be deleted (leaving
> the other two conditions).

(Reminder: This line of code, in AutoSetGeneratorRunning, is deciding whether or not to hack genObj.[[GeneratorState]] to "running", to keep Debugger hooks from resuming the generator when it's already on the stack.)

Added a test for this, Frame-onPop-generators-05.js.

The .onPop event does expose the generator object to the debugger. But that is after the JSOP_INITIALYIELD instruction puts the generator in the "suspended" state, so this code works as expected.

So it seems the generator object is never exposed, even to Debugger code, while genObj->isBeforeInitialYield() is true. Thus checking for that state was harmless. But it was still bizarre and extraneous. I've deleted it.

Comment 37

[Jason Orendorff [:jorendorff]]

(In reply to Jim Blandy :jimb from comment #13)
> How about testing onExceptionUnwind and getNewestFrame?

Added onExceptionUnwind-generators-01.js and Debugger-getNewestFrame-generators-01.js.
 
> We should document that initial onEnterFrame/onPop that both generators and
> async functions cause, for default argument evaluation (and destructuring, I
> guess?). We should also document that async functions then immediately
> re-enter the frame and begin executing the body, whereas generators do not.

Added new doc section "Stepping Into Generators: The 'Initial Yield'". Though how anybody will ever be able to stand reading it, I don't know.

Comment 38

[Jason Orendorff [:jorendorff]]

(In reply to Jim Blandy :jimb from comment #28)
> Part 6: Re-enable stepping when an async or generator frame with an .onStep
> hook is resumed
> -----------------------------------------------------------------
> Do we test anywhere that, when resuming a generator causes us to call one
> Debugger's onEnterFrame handler, another Debugger's Debugger.Frame gets
> marked as live, and becomes usable? That is, do we check for the existence
> of that loop in slowPathOnResumeFrame?

Added onEnterFrame-generator-05.js.

> ::: js/src/jit-test/tests/debug/Frame-onStep-async-01.js
> > +    dbg.onEnterFrame = undefined;
> 
> So the idea here is that once we've set the onStep handler, we can drop the
> onEnterFrame handler and the frames will still be recognized as debuggee
> frames?

Yes.

> ::: js/src/vm/Debugger.cpp
> > +    if (GlobalObject::DebuggerVector* debuggers = frame.callee()->global().getDebuggers()) {
> 
> I think `AbstractFramePtr` has a `global` method you can call directly,
> instead of `frame.callee()->global()`.

Thanks, fixed.

Comment 39

[Jason Orendorff [:jorendorff]]

Attachment: Bug 1448880 - Part 1: Always close a generator on early forced return. r?jimb

The previous code failed to close the generator in the case where
JSOP_GENERATOR had run but JSOP_INITIAL_YIELD had not, a bit of sloppiness that
created yet another special case. Things will get more complicated when we
start keeping frames live while suspended; it's better to not have this special
case.

Comment 40

[Jason Orendorff [:jorendorff]]

Attachment: Bug 1448880 - Part 2: Persist Debugger.Frame objects for generators across yield/await. r?jimb.

This is the minimal patch, but it leaves two bugs:

1.  When a generator or async function is resumed, stepping is reenabled in
    Debugger::getFrame, which isn't necessarily called. The onStep tests in
    this patch work because they all use an onEnterFrame hook, which causes
    getFrame to be called as soon as the generator is resumed.

2.  .onStep and .onPop hooks on suspended Frames do not survive GC if there are
    no other references to the Frame or the Debugger object. The behavior is safe,
    but the hooks can just mysteriously stop firing when GC happens.

The next three patches in this stack lay the groundwork for fixing these bugs,
without changing behavior; part 6 fixes the first bug; and part 7 fixes the second.

Depends on D6982

Comment 41

[Jason Orendorff [:jorendorff]]

Attachment: Bug 1448880 - Part 3: AbstractFramePtr::isGeneratorFrame(). r?jimb

This proves handy in several places, later in the stack.

Depends on D6983

Comment 42

[Jason Orendorff [:jorendorff]]

Attachment: Bug 1448880 - Part 4: Split Debugger::onResumeFrame from onEnterFrame. r?jimb

Pure refactoring, no change in behavior. This is in anticipation of doing
additional work in onResumeFrame.

Depends on D6984

Comment 43

[Jason Orendorff [:jorendorff]]

Attachment: Bug 1448880 - Part 5: Tell removeFromFrameMapsAndClearBreakpointsIn() if we are suspending. r?jimb

Since the argument is not used yet, this too is a pure refactoring, with no
change in behavior yet.

Depends on D6985

Comment 44

[Jason Orendorff [:jorendorff]]

Attachment: Bug 1448880 - Part 6: Re-enable stepping when an async or generator frame with an .onStep hook is resumed. r?jimb

That is, don't put it off until Debugger::getFrame() is called. The effect is
subtle, as indicated by the test changes: the onEnterFrame hooks in those tests
were causing getFrame to be called very early during generator resumption,
which made the tests pass.

With this patch, we no longer adjust the step mode count when suspending or
resuming. This change is necessary to make the frame->isDebuggee() call in
Debugger::onResumeFrame the right criterion for calling slowPathOnResumeFrame.
It's true if the step mode count on the script is nonzero. (This approach also
simplifies error handling, as resuming a Debugger.Frame is now idempotent: we
don't have to worry about adjusting the step mode count too much or not enough
on error.)

Depends on D6986

Comment 45

[Jason Orendorff [:jorendorff]]

Attachment: Bug 1448880 - Part 7: Fix hasAnyLiveHooks to include onStep handlers on suspended frames. r?jimb,sfink

Depends on D6988

Comment 46

[Jason Orendorff [:jorendorff]]

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

Comment 47

[Jason Orendorff [:jorendorff]]

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

Comment 48

[Steve Fink [:sfink] [:s:]]

Comment on attachment 9012352 [details]
Bug 1448880 - Part 7: Fix hasAnyLiveHooks to include onStep handlers on suspended frames. r?jimb,sfink

Steve Fink [:sfink] [:s:] has approved the revision.

Comment 49

[Jim Blandy :jimb]

(In reply to Jason Orendorff [:jorendorff] from comment #32)
> What's going unsaid here is that
> 
> - popping a frame from the stack causes the AbstractFramePtr to become
> invalid
> - that's why we have to remove the entry from the `frames` table on suspend
> - but also, when it's re-pushed, it could very well have a different address
> on the stack
> - and hence a different AbstractFramePtr
> - so we couldn't just leave it in the `frames` table anyway
> - but also, other stack frames could have been created at the same address
> in between
> - so we couldn't just leave it in the `frames` table for that reason as well
> 
> However... I don't want to say *all* of that, and I'm not sure which pieces
> the reader is most likely to need.

I think most of that follows from the nature of AbstractFramePtr, whose behavior here is not so different from that of any other dynamically managed pointer (using addresses as keys always suffers from the AB(A?) problem), so it's okay not to mention it. What I needed help with here was following the generator-specific implications, where Debugger.Frames refer to stack frames that come and go.

Comment 50

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2f3da01d8816
Part 1: Always close a generator on early forced return. r=jimb
https://hg.mozilla.org/integration/autoland/rev/64810935a751
Part 2: Persist Debugger.Frame objects for generators across yield/await. r=jimb.
https://hg.mozilla.org/integration/autoland/rev/9c03b503909a
Part 3: AbstractFramePtr::isGeneratorFrame(). r=jimb
https://hg.mozilla.org/integration/autoland/rev/61031045a58c
Part 4: Split Debugger::onResumeFrame from onEnterFrame. r=jimb
https://hg.mozilla.org/integration/autoland/rev/27b0121d9e31
Part 5: Tell removeFromFrameMapsAndClearBreakpointsIn() if we are suspending. r=jimb
https://hg.mozilla.org/integration/autoland/rev/6c8949f053e0
Part 6: Re-enable stepping when an async or generator frame with an .onStep hook is resumed. r=jimb
https://hg.mozilla.org/integration/autoland/rev/8ff4dac9916d
Part 7: Fix hasAnyLiveHooks to include onStep handlers on suspended frames. r=sfink

Comment 51

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/2f3da01d8816
https://hg.mozilla.org/mozilla-central/rev/64810935a751
https://hg.mozilla.org/mozilla-central/rev/9c03b503909a
https://hg.mozilla.org/mozilla-central/rev/61031045a58c
https://hg.mozilla.org/mozilla-central/rev/27b0121d9e31
https://hg.mozilla.org/mozilla-central/rev/6c8949f053e0
https://hg.mozilla.org/mozilla-central/rev/8ff4dac9916d