1546727 - [jsdbg2] Debugger.Frames for suspended generators cannot find generator

Status: RESOLVED FIXED

(Core :: JavaScript Engine, task, P1)

Description

[Jim Blandy :jimb]

A Debugger.Frame for a suspended generator or async function call presently holds no reference to the call's underlying generator object. This makes certain useful operations impossible: for example, the Debugger.Frame.prototype.callee accessor is perfectly meaningful to apply to a suspended generator, but it presently returns null unless the call is presently on the stack somewhere.

More immediately, for Debugger to track resumptions of generators and async functions properly, the Baseline JIT must include instrumentation code for JSOP_AFTERYIELD instructions in the compiled script. The JIT's only indication that this instrumentation is needed, however, is the presence of a Debugger.Frame referring to some invocation of the script. It is difficult to keep the JIT informed when there is no path from a Debugger.Frame for a suspended call to the script in question.

The fix is simply to add another slot to Debugger.Frame that refers to the frame's generator object, if any. Follow-up bugs can make accessors like callee and environment work, consulting the FrameIter::Data for live frames, and the generator slot for suspended frames.

Comment 1

[Jim Blandy :jimb]

Attachment: Bug 1546727: Move JSSLOT_DEBUGFRAME_ enum into DebuggerFrame class. r?jorendorff

This is standard SpiderMonkey practice for NativeObject subclasses, and makes
slot access in DebuggerFrame methods a bit cleaner.

Comment 2

[Jim Blandy :jimb]

Attachment: Bug 1546727: Make DebuggerFrame_trace and finalize static member functions of DebuggerFrame. r?jorendorff

SpiderMonkey standard practice for classes derived from JSObject defines
ClassOps hooks as static member functions.

Depends on D28782

Comment 3

[Jim Blandy :jimb]

Attachment: Bug 1546727: Make DebuggerFrame_maybeDecrementFrameScriptStepModeCount a method of DebuggerFrame. r?jorendorff

This function probably predates the existence of the DebuggerFrame class, and
was never moved in.

Depends on D28783

Comment 4

[Jim Blandy :jimb]

Attachment: Bug 1546727: Make DebuggerFrame_requireLive and DebuggerFrame_checkThis member functions of DebuggerFrame. r?jorendorff

These function probably predate the existence of the DebuggerFrame class, and
were never moved in.

Depends on D28784

Comment 5

[Jim Blandy :jimb]

Attachment: Bug 1546727: Make DebuggerFrame_getScript a static method of DebuggerFrame. r?jorendorff

This function probably predates the existence of the DebuggerFrame class, and
was never moved in.

Depends on D28785

Comment 6

[Jim Blandy :jimb]

Attachment: Bug 1546727: Remove extraneous 'inline' keywords, and add where missing. r?jorendorff

Comment 7

[Jim Blandy :jimb]

Attachment: Bug 1546727: Add GENERATOR_INFO_SLOT to js::DebuggerFrame. r?jorendorff

Debugger.Frame objects referring to generator or async calls need to be able to
find the call's generator object, even when the call is suspended. This patch
adds a reserved slot to js::DebuggerFrame objects pointing to a new C++ type,
js::DebuggerFrame::GeneratorInfo, whose job is to hold a cross-compartment
pointer to the generator.

Future patches will also use GeneratorInfo to manage the debugging
instrumentation on the generator's script, so that Debugger can reliably resume
observation of resumed generator frames.

Depends on D29113

Comment 8

[Jim Blandy :jimb]

That prior patch was just posted for reference; it's not ready for review. Sorry about the confusion.

TIL that notifying Debugger from AbstractGeneratorObject::setClosed is too late to be useful: when a generator call makes its final return, by the time AGO::setClosed is called, the system has already called slowPathOnLeaveFrame and removed the generator's entry from generatorFrames, so slowPathOnGeneratorClosing can't find the Debugger.Frames that refer to it.

Another corner case to consider: it's possible to create a Debugger.Frame for a generator call, suspend the call, and then have the generator and the Debugger.Frame get GC'd. When this occurs, the underlying JSScript needs to be informed that the post-resume instrumentation is no longer needed. That's all fine; the wrinkle is, the JSScript may or may not also have been finalized. If it has, then trying to remove instrumentation will crash. I think we can use IsAboutToBeFinalized to detect this case, but I'm not sure.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/054f6ef6e447
Move JSSLOT_DEBUGFRAME_ enum into DebuggerFrame class. r=jorendorff

Comment 10

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/054f6ef6e447

Comment 11

[Jim Blandy :jimb]

Reopening - not all patches are landed.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f4ab69d7aabf
Make DebuggerFrame_trace and finalize static member functions of DebuggerFrame. r=jorendorff

Comment 13

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/f4ab69d7aabf

Comment 14

[Jim Blandy :jimb]

Attachment: Bug 1546727: Use DebuggerFrame::generator instead of GetGeneratorObjectForFrame where possible. r?jorendorff

Since GetGeneratorObjectForFrame is a bit involved (it looks up the identifier
'.generator' on the scope chain) and not entirely reliable (it returns nullptr
between the GENERATOR and SETALIASEDVAR .generator opcodes), it's better to
simply fetch the generator from the DebuggerFrame, when one is available.

Since a DebuggerFrame has a generator exactly when there is an entry in
generatorFrames going the other direction, from generator to DebuggerFrame, this
means that Debugger::removeFromFrameMapsAndClearBreakpointsIn can actually do
its job reliably, which lets us remove certain kinky conditions in The Famous
Step Count Assertion of 1874.

In other cases, GetGeneratorObjectForFrame is the only option, and its flakiness
doesn't matter; document those a bit better.

Depends on D29114

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9224b5e39c2e
Make DebuggerFrame_maybeDecrementFrameScriptStepModeCount a method of DebuggerFrame. r=jorendorff

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b4e7d290701a
Make DebuggerFrame_requireLive and DebuggerFrame_checkThis member functions of DebuggerFrame. r=jorendorff

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f8b9b7dcd147
Make DebuggerFrame_getScript a static method of DebuggerFrame. r=jorendorff

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d57a8a8d3f8d
Remove extraneous 'inline' keywords, and add where missing. r=jorendorff

Comment 19

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/9224b5e39c2e
https://hg.mozilla.org/mozilla-central/rev/b4e7d290701a
https://hg.mozilla.org/mozilla-central/rev/f8b9b7dcd147
https://hg.mozilla.org/mozilla-central/rev/d57a8a8d3f8d

Comment 20

[Jim Blandy :jimb]

Attachment: Bug 1546727: Use DebuggerFrame::generator instead of GetGeneratorObjectForFrame where possible. r?jorendorff

Since GetGeneratorObjectForFrame is a bit involved (it looks up the identifier
'.generator' on the scope chain) and not entirely reliable (it returns nullptr
between the GENERATOR and SETALIASEDVAR .generator opcodes), it's better to
simply fetch the generator from the DebuggerFrame, when one is available.

Since a DebuggerFrame has a generator exactly when there is an entry in
generatorFrames going the other direction, from generator to DebuggerFrame, this
means that Debugger::removeFromFrameMapsAndClearBreakpointsIn can actually do
its job reliably, which lets us remove certain kinky conditions in The Famous
Step Count Assertion of 1874.

In other cases, GetGeneratorObjectForFrame is the only option, and its flakiness
doesn't matter; document those a bit better.

Depends on D32270

Comment 21

[Phabricator Automation]

Comment on attachment 9066939 [details]
Bug 1546727: Use DebuggerFrame::generator instead of GetGeneratorObjectForFrame where possible. r?jorendorff

Revision D32271 was moved to bug 1551176. Setting attachment 9066939 [details] to obsolete.