The only way to step into generator.next() or step over `await` in an async function is to "just know" exactly what code you're going to be stepping into, and set a breakpoint. That is silly; a debugger implementing "step in" should be able to set a single hook and expect it to fire if we step into something. onEnterFrame is that hook.

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

Jim, the tests are "passing" in the sense that they'll pass once I land the patches in bug 1469350, bug 1440481, bug 1474385, and bug 1471954.

Comment on attachment 8991788 [details] [diff] [review] Part 2: Fire onEnterFrame when resuming a generator or async function r?jandem for js/src/jit changes; r?jimb for everything else. JSOP_DEBUGAFTERYIELD is perplexing. I can see why it's there, but it makes Baseline's behavior uncomfortably different from what the interpreter does. Maybe it would be better if JSOP_DEBUGAFTERYIELD simply meant "the DebugTrap on this instruction can't be disabled when running debug-mode baseline jitcode". As far as BaselineCompiler is concerned, it would just be a no-op, like in the interpreter. But I'm not sure that is the right approach either, and I really don't have time to go after it. What I've got here passes tests and seems reasonable enough (?).

Comment on attachment 8991787 [details] [diff] [review] Part 1: Minor code tweaks, in preparation. There should be no observable change in behavior yet Review of attachment 8991787 [details] [diff] [review]: ----------------------------------------------------------------- Nice.

Comment on attachment 8991788 [details] [diff] [review] Part 2: Fire onEnterFrame when resuming a generator or async function Review of attachment 8991788 [details] [diff] [review]: ----------------------------------------------------------------- r=me on the jit/ changes, with comment below addressed. ::: js/src/jit/BaselineDebugModeOSR.cpp @@ +536,5 @@ > recompInfo->resumeAddr = bl->nativeCodeForPC(script, pc, &recompInfo->slotInfo); > popFrameReg = false; > break; > > case ICEntry::Kind_DebugPrologue: If we reuse this Kind, I think we should rename it to DebugPrologueOrAfterYield, and also update this comment: https://searchfox.org/mozilla-central/rev/b0275bc977ad7fda615ef34b822bba938f2b16fd/js/src/jit/BaselineDebugModeOSR.cpp#369 (You could argue that JSOP_DEBUGAFTERYIELD is in a sense the "debug prologue" here but this code is complicated and I think it makes sense to be explicit about the two cases. Alternative is to add a new Kind_DebugAfterYield.)

Comment on attachment 8991782 [details] [diff] [review] Part 0: Add some passing tests Review of attachment 8991782 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit-test/tests/debug/Frame-live-06.js @@ +1,4 @@ > +// frame.live is false for generator frames after they return. > + > +let g = newGlobal(); > +g.eval("function* f(explicitly) { debugger;}"); This function is never called, so we never actually produce a generator frame. Add some assertions to catch this mistake. ::: js/src/jit-test/tests/debug/Frame-onPop-generators-04.js @@ +22,5 @@ > +g.eval("debugger;"); > + > +assertEq(genFrame instanceof Debugger.Frame, true); > +assertEq(genFrame.live, false); > +assertThrowsInstanceOf(() => genFrame.callee, Error); Why does it behave this way?? ::: js/src/jit-test/tests/debug/onEnterFrame-async-resumption-01.js @@ +17,5 @@ > + }; > + > + // Don't tell anyone, but if we force-return a generator object here, > + // the robots accept it as one of their own and plug it right into the > + // async function machinery. This may be handy against Skynet someday. Noted. Thanks.

Comment on attachment 8991782 [details] [diff] [review] Part 0: Add some passing tests sorry, meant to reject because of the first comment.

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

Comment on attachment 8991788 [details] [diff] [review] Part 2: Fire onEnterFrame when resuming a generator or async function Review of attachment 8991788 [details] [diff] [review]: ----------------------------------------------------------------- I have some questions, so I'm just needinfo'ing you. ::: js/src/jit-test/tests/debug/Frame-live-07.js @@ +37,5 @@ > + assertDeepEq(result, what); > + }; > + g.eval("debugger;"); > + > + assertEq(poppedFrame.live, false); Could we also assert that t == when? @@ +40,5 @@ > + > + assertEq(poppedFrame.live, false); > + try { > + poppedFrame.older; > + throw new Error("expected exception, none thrown"); Isn't this something that asserts.js has a function for? ::: js/src/jit-test/tests/debug/Frame-onStep-generators-01.js @@ +27,5 @@ > + }; > +}; > + > +g.f(); > +assertEq(log.join(" "), "5 6 1 6 7 1 2 7 8 2 3 8 9 3 9 10"); This is pretty nice, to see the 'coroutine' behavior spelled out so clearly. ::: js/src/jit-test/tests/debug/Frame-onStep-generators-02.js @@ +40,5 @@ > +assertThrowsValue(() => g.f(), "fit"); > +// z{1} is the initial generator setup. > +// z{12} is the first .next() call, running to `yield 1` on line 2 > +// The final `z{!}` is for the .throw() call. > +assertEq(log, "f{56z{1}67z{12}78z{!}!}"); lovely ::: js/src/jit-test/tests/debug/onEnterFrame-async-resumption-02.js @@ +1,2 @@ > +// A Debugger can {throw:} from onEnterFrame in an async function. > +// The resulting promise (if any) is resolved with the thrown error value. Isn't it more correct to say it is 'rejected with the thrown error value'? Also, this test seems to say that the initial onEnterFrame does something different. That should be commented somewhere. Perhaps the same 'resume point' comment you have elsewhere? @@ +16,5 @@ > +// Repeat the test for each onEnterFrame event. > +// It fires up to three times: > +// - when the generator g.f is called; > +// - when we enter it to run to `yield 1`; > +// - when we resume after the yield to run to the end. This comment talks about 'generator' and 'yield', but I think it means 'async function' and 'await'. ::: js/src/jit-test/tests/debug/onEnterFrame-generator-01.js @@ +23,5 @@ > + assertEq(frame.callee, gw.makeDebuggeeValue(g.gen)); > + > + // `arguments` elements don't work in resumed generator frames, > + // because generators don't keep the arguments around. > + // The first onEnterFrame and onPop events can see them. If this is true, it seems like we should file a follow-up bug for it. I tried writing a generator that used `arguments` after yielding, and it worked fine. I think this means that we ought to be able to get the arguments under certain circumstances: https://searchfox.org/mozilla-central/rev/2466b82b729765fb0a3ab62f812c1a96a7362478/js/src/vm/GeneratorObject.cpp#164-165 ::: js/src/jit-test/tests/debug/onEnterFrame-generator-02.js @@ +1,1 @@ > +// onEnterFrame fires after the [[GeneratorState]] is set to "executing". But, doesn't this test just flat-out ignore the first call to onEnterFrame? Does the comment only apply to onEnterFrame calls after the first? ::: js/src/jit-test/tests/debug/onEnterFrame-generator-resumption-01.js @@ +1,1 @@ > +// A debugger can {throw:} from onEnterFrame at any resume point in a generator. We should make sure to mention this term 'resume point' in the docs. ::: js/src/jit-test/tests/debug/onEnterFrame-generator-resumption-02.js @@ +14,5 @@ > +// - four times at resume points: > +// - initial resume at the top of the generator body > +// - resume after yielding 1 > +// - resume after yielding 2 > +// - resume after yielding 3 (and running to the end). I read this as 'resume after (yielding 3 and running to the end)'. There's no onEnterFrame after we run to the end, right? Doesn't this actually mean to say: - resume after yielding 3 (this resumption will run to the end) @@ +19,5 @@ > +// This test ignores the initial call and focuses on resume points. > +for (let i = 1; i < 5; i++) { > + let hits = 0; > + dbg.onEnterFrame = frame => { > + return hits++ < i ? undefined : {return: "ignore this string"}; I guess because you're using the [... X] (I want to call it 'unspread') syntax, there's no easy way to check that this value made it through... ::: js/src/vm/GeneratorObject.cpp @@ +8,5 @@ > > #include "vm/JSObject.h" > > #include "vm/ArrayObject-inl.h" > +#include "vm/Debugger-inl.h" What is this needed for? ::: js/src/vm/Interpreter.cpp @@ +4278,5 @@ > + case ResumeMode::Throw: > + case ResumeMode::Terminate: > + goto error; > + case ResumeMode::Return: > + MOZ_ASSERT_IF(REGS.fp()->callee().isGenerator(), gen->isClosed()); I don't understand what's going on here. 1) Don't we know for sure that the callee is a generator? We just called GeneratorObject::resume above. 2) Why would the generator be closed? It could have been open when we called Debugger::onEnterFrame, and I don't see why that would close it.

(In reply to Jim Blandy :jimb from comment #11) > ::: js/src/jit-test/tests/debug/Frame-live-07.js > Could we also assert that t == when? Yes. Added. > > + try { > > + poppedFrame.older; > > + throw new Error("expected exception, none thrown"); > > Isn't this something that asserts.js has a function for? Oh! So it does, `assertErrorMessage`. Updated. > ::: js/src/jit-test/tests/debug/onEnterFrame-async-resumption-02.js > > +// The resulting promise (if any) is resolved with the thrown error value. > > Isn't it more correct to say it is 'rejected with the thrown error value'? Yes. Fixed. > Also, this test seems to say that the initial onEnterFrame does something > different. That should be commented somewhere. Perhaps the same 'resume > point' comment you have elsewhere? Wow, you're right. The behavior is exactly the same on the initial onEnterFrame, so the `if` guarding that assertion is deceptive. I've deleted it. > @@ +16,5 @@ > > +// Repeat the test for each onEnterFrame event. > > +// It fires up to three times: > > +// - when the generator g.f is called; > > +// - when we enter it to run to `yield 1`; > > +// - when we resume after the yield to run to the end. > > This comment talks about 'generator' and 'yield', but I think it means > 'async function' and 'await'. Yes. Thanks, fixed. > ::: js/src/jit-test/tests/debug/onEnterFrame-generator-01.js > > + // `arguments` elements don't work in resumed generator frames, > > + // because generators don't keep the arguments around. > > + // The first onEnterFrame and onPop events can see them. > > If this is true, it seems like we should file a follow-up bug for it. I > tried writing a generator that used `arguments` after yielding, and it > worked fine. > > I think this means that we ought to be able to get the arguments under > certain circumstances: > https://searchfox.org/mozilla-central/rev/ > 2466b82b729765fb0a3ab62f812c1a96a7362478/js/src/vm/GeneratorObject.cpp#164- > 165 Yes, we can arrange so that if the generator uses `arguments`, then `frame.arguments` works properly. I'll look into it! > ::: js/src/jit-test/tests/debug/onEnterFrame-generator-02.js > > +// onEnterFrame fires after the [[GeneratorState]] is set to "executing". > > But, doesn't this test just flat-out ignore the first call to onEnterFrame? > Does the comment only apply to onEnterFrame calls after the first? I added this comment: dbg.onEnterFrame = frame => { // The first time onEnterFrame fires, there is no generator object, so // there's nothing to test. The generator object doesn't exist until // JSOP_GENERATOR is reached, right before the initial yield. if (genObj !== null) { ... Let me know if there's something else I should be saying here. More coming in a separate comment.

Filed bug 1483625 for frame.arguments. > ::: js/src/jit-test/tests/debug/onEnterFrame-generator-resumption-02.js > > +// - resume after yielding 3 (and running to the end). > > - resume after yielding 3 (this resumption will run to the end) Yes, that's better. Changed. > > + return hits++ < i ? undefined : {return: "ignore this string"}; > > I guess because you're using the [... X] (I want to call it 'unspread') > syntax, there's no easy way to check that this value made it through... Changed to use a loop and check that this value comes through. > ::: js/src/vm/Interpreter.cpp > @@ +4278,5 @@ > > + case ResumeMode::Throw: > > + case ResumeMode::Terminate: > > + goto error; > > + case ResumeMode::Return: > > + MOZ_ASSERT_IF(REGS.fp()->callee().isGenerator(), gen->isClosed()); > > I don't understand what's going on here. > > 1) Don't we know for sure that the callee is a generator? We just called > GeneratorObject::resume above. Ah - yes, sorry about that. This assertion was probably moved around several times before it ended up here, and I never noticed it could be strengthened. > 2) Why would the generator be closed? It could have been open when we called > Debugger::onEnterFrame, and I don't see why that would close it. AdjustGeneratorResumptionValue does it; see this comment: https://searchfox.org/mozilla-central/rev/2466b82b729765fb0a3ab62f812c1a96a7362478/js/src/vm/Debugger.cpp#1452-1457 In particular, the bytecode sequence for returning from a generator includes JSOP_FINALYIELDRVAL, which closes the generator. There is another possible approach which that comment does not mention: close the generator in the interpreter, at the point where we currently have the gen->isClosed() assertion instead -- and at the corresponding place or places in jit/VMFunctions.cpp. It's more code, though. No strong preference. More to come.

(In reply to Jason Orendorff [:jorendorff] from comment #13) > > 1) Don't we know for sure that the callee is a generator? We just called > > GeneratorObject::resume above. > > Ah - yes, sorry about that. This assertion was probably moved around several > times before it ended up here, and I never noticed it could be strengthened. Okay, great. > > 2) Why would the generator be closed? It could have been open when we called > > Debugger::onEnterFrame, and I don't see why that would close it. > > AdjustGeneratorResumptionValue does it; see this comment: > https://searchfox.org/mozilla-central/rev/ > 2466b82b729765fb0a3ab62f812c1a96a7362478/js/src/vm/Debugger.cpp#1452-1457 Oh, wow, that function. I had forgotten about the the call to `finalSuspend` there. Could we have a comment on the assertion that the debugger has adjusted the generator's state before returning from the onEnterFrame call?

Thanks for all your fixes and explanations. If you update the patch, I'll r+.

> 1) Don't we know for sure that the callee is a generator? We just called > GeneratorObject::resume above. Correction on this item: the callee could also be an async non-generator function, in which case it is not closed. Possibly it should be closed in that case, too. Filed bug 1483669 for that. But note that it doesn't matter as far as observable behavior is concerned, because the generator object for an async function call is never exposed to the user and thus there's no way for the user to attempt to reenter it--the specific thing form of madness I was trying to rule out here. In fact I think the generator object immediately becomes garbage (educated guess). > ::: js/src/jit-test/tests/debug/onEnterFrame-generator-resumption-01.js > @@ +1,1 @@ > > +// A debugger can {throw:} from onEnterFrame at any resume point in a generator. > > We should make sure to mention this term 'resume point' in the docs. Can you be more specific? Do you mean it should be mentioned in the docs for onEnterFrame? Or somewhere else? > ::: js/src/vm/GeneratorObject.cpp > > +#include "vm/Debugger-inl.h" > > What is this needed for? It's not needed until a later patch. Removed.

Comment on attachment 9001377 [details] [diff] [review] Part 2b: Address jimb code review comments for part 2. To fold Review of attachment 9001377 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/Interpreter.cpp @@ +4291,5 @@ > case ResumeMode::Throw: > case ResumeMode::Terminate: > goto error; > case ResumeMode::Return: > + MOZ_ASSERT_IF(REGS.fp()->callee().isGenerator(), // as opposed to an async function perfect

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

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/50ed38c98cc0 Part 0: Add some passing tests. r=jimb https://hg.mozilla.org/integration/mozilla-inbound/rev/13020b58f0fa Part 1: Minor code tweaks, in preparation. There should be no observable change in behavior yet. r=jandem https://hg.mozilla.org/integration/mozilla-inbound/rev/972ad5dc9a84 Part 2: Fire onEnterFrame when resuming a generator or async function. r=jandem, r=jimb

Backed out for jit failures Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=972ad5dc9a842911479edf3737c0e2aee7fad35d Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=195167328&repo=mozilla-inbound&lineNumber=3216 Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/84eb90b730f0fa8b4073d4fb9ba29d28ff4eabed ***commit message in the backout is wrong

https://hg.mozilla.org/integration/mozilla-inbound/rev/a981419137d9ba16d0b1ce7423e39e4a4ec113de Bug 1475417 - Part 0: Add some passing tests. r=jimb https://hg.mozilla.org/integration/mozilla-inbound/rev/da2c87b3210c160afc98ee238f3f61a2a26b3a55 Bug 1475417 - Part 1: Minor code tweaks, in preparation. There should be no observable change in behavior yet. r=jandem https://hg.mozilla.org/integration/mozilla-inbound/rev/87509a363c9ee2a38998a2e4dacc16e577a877ec Bug 1475417 - Part 2: Fire onEnterFrame when resuming a generator or async function. r=jandem, r=jimb

https://hg.mozilla.org/mozilla-central/rev/a981419137d9 https://hg.mozilla.org/mozilla-central/rev/da2c87b3210c https://hg.mozilla.org/mozilla-central/rev/87509a363c9e