Closed Bug 1546727 Opened 4 months ago Closed 18 days ago

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

Categories

(Core :: JavaScript Engine, task, P1)

task

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox68 --- fixed

People

(Reporter: jimb, Assigned: jimb)

References

(Blocks 3 open bugs)

Details

(Whiteboard: [debugger-mvp])

Attachments

(7 files, 2 obsolete files)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review

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.

See Also: → 1546817
Depends on: 1546817
See Also: 1546817

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

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

Depends on D28782

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

Depends on D28783

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

Depends on D28784

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

Depends on D28785

Blocks: 1547520

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

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.

Depends on: 1548075
Assignee: nobody → jimb
Status: NEW → ASSIGNED
Whiteboard: [debugger-mvp]
Pushed by jblandy@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/054f6ef6e447
Move JSSLOT_DEBUGFRAME_ enum into DebuggerFrame class. r=jorendorff
Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68

Reopening - not all patches are landed.

Status: RESOLVED → REOPENED
Keywords: leave-open
Resolution: FIXED → ---
Pushed by jblandy@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f4ab69d7aabf
Make DebuggerFrame_trace and finalize static member functions of DebuggerFrame. r=jorendorff
Status: REOPENED → ASSIGNED

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

Pushed by jblandy@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9224b5e39c2e
Make DebuggerFrame_maybeDecrementFrameScriptStepModeCount a method of DebuggerFrame. r=jorendorff
Pushed by jblandy@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b4e7d290701a
Make DebuggerFrame_requireLive and DebuggerFrame_checkThis member functions of DebuggerFrame. r=jorendorff
Pushed by jblandy@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f8b9b7dcd147
Make DebuggerFrame_getScript a static method of DebuggerFrame. r=jorendorff
Pushed by jblandy@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d57a8a8d3f8d
Remove extraneous 'inline' keywords, and add where missing. r=jorendorff
Depends on: 1552118
Priority: -- → P1

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 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.

Attachment #9066939 - Attachment is obsolete: true
Attachment #9061210 - Attachment is obsolete: true
Blocks: dbg-70
Status: ASSIGNED → RESOLVED
Closed: 4 months ago18 days ago
Keywords: leave-open
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.