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

RESOLVED FIXED in Firefox 65

Status

()

enhancement
P1
normal
RESOLVED FIXED
Last year
16 days ago

People

(Reporter: jorendorff, Assigned: jorendorff)

Tracking

(Blocks 2 bugs, Regressed 1 bug)

unspecified
mozilla65
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 wontfix, firefox62 wontfix, firefox63 wontfix, firefox64 wontfix, firefox65 fixed)

Details

Attachments

(7 attachments, 13 obsolete attachments)

46 bytes, text/x-phabricator-request
Details | Review
46 bytes, text/x-phabricator-request
Details | Review
46 bytes, text/x-phabricator-request
Details | Review
46 bytes, text/x-phabricator-request
Details | Review
46 bytes, text/x-phabricator-request
Details | Review
46 bytes, text/x-phabricator-request
Details | Review
46 bytes, text/x-phabricator-request
sfink
: review+
Details | Review
No description provided.
Priority: -- → P1
Assignee: nobody → jorendorff
Depends on: 1469350, 1469330, 1469323
Depends on: 1475417
Also deletes two tests that are completely redundant ever since we removed
legacy generators.
Attachment #8991779 - Flags: review?(jimb)
Comment on attachment 8991779 [details] [diff] [review]
Part 0: Add some passing tests

This patch was meant for another bug.
Attachment #8991779 - Attachment is obsolete: true
Attachment #8991779 - Flags: review?(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.
Attachment #8997578 - Flags: review?(jimb)
Attachment #8992109 - Attachment is obsolete: true
Attachment #8992109 - Flags: review?(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.
Attachment #8997580 - Flags: review?(jimb)
Attachment #8997578 - Attachment is obsolete: true
Attachment #8997578 - Flags: review?(jimb)
This proves handy in several places, later in the stack.
Attachment #8997583 - Flags: review?(jimb)
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.
Attachment #8997850 - Flags: review?(jimb)
Attachment #8997583 - Attachment is obsolete: true
Attachment #8997583 - Flags: review?(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.
Attachment #8997886 - Flags: review?(jimb)
Attachment #8997850 - Attachment is obsolete: true
Attachment #8997850 - Flags: review?(jimb)
This proves handy in several places, later in the stack.
Attachment #8997887 - Flags: review?(jimb)
Pure refactoring, no change in behavior. This is in anticipation of doing
additional work in onResumeFrame.
Attachment #8997889 - Flags: review?(jimb)
Since the argument is not used yet, this too is a pure refactoring, with no
change in behavior yet.
Attachment #8997890 - Flags: review?(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.
Attachment #8992109 - Attachment is obsolete: false
Attachment #8992109 - Attachment is obsolete: true
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.)
Attachment #8997962 - Flags: review?(jimb)
Attachment #8998019 - Flags: review?(jimb)
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'...
Attachment #8998019 - Flags: review?(sphink) → review+
Posted patch Part 2a: Documentation (obsolete) — Splinter Review
This will be folded into part 2.
Attachment #8998291 - Flags: review?(jimb)
(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)
Flags: needinfo?(sphink)
Blocks: 1477063
Right, sorry, I guess that'd be IsMarkedUnbarriered().
Flags: needinfo?(sphink)
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
Attachment #8997580 - Flags: review?(jimb) → review+
(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.
> ::: 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 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.
Attachment #8997886 - Flags: review?(jimb) → review-
Attachment #8998291 - Flags: review?(jimb) → review+
Attachment #8997887 - Flags: review?(jimb) → review+
Attachment #8997889 - Flags: review?(jimb) → review+
Attachment #8997890 - Flags: review?(jimb) → review+
(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.
(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 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()`.
Attachment #8997962 - Flags: review?(jimb) → review+
Attachment #8998019 - Flags: review?(jimb) → review+
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.
You're the best!
(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 += ")";
        }
    };
(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.
(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.
(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).
(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).
(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.
(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.
(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.
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.
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
This proves handy in several places, later in the stack.

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

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

Depends on D6985
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
Attachment #8997580 - Attachment is obsolete: true
Attachment #8997886 - Attachment is obsolete: true
Attachment #8997887 - Attachment is obsolete: true
Attachment #8997889 - Attachment is obsolete: true
Attachment #8997890 - Attachment is obsolete: true
Attachment #8998291 - Attachment is obsolete: true
Attachment #8997962 - Attachment is obsolete: true
Attachment #8998019 - Attachment is obsolete: true
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.
Attachment #9012352 - Flags: review+
(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.
Pushed by jorendorff@mozilla.com:
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
Depends on: 1501666
Depends on: 1501731
Depends on: 1501722
No longer depends on: 1501731
Regressions: 1551176
Regressions: 1565895
You need to log in before you can comment on or make changes to this bug.