[jsdbg2] Debugger.Frames for suspended generators cannot find generator
Categories
(Core :: JavaScript Engine, task, P1)
Tracking
()
Tracking | Status | |
---|---|---|
firefox68 | --- | fixed |
People
(Reporter: jimb, Assigned: jimb)
References
(Blocks 1 open bug)
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.
Assignee | ||
Updated•6 years ago
|
Assignee | ||
Comment 1•6 years ago
|
||
This is standard SpiderMonkey practice for NativeObject subclasses, and makes
slot access in DebuggerFrame methods a bit cleaner.
Assignee | ||
Comment 2•6 years ago
|
||
SpiderMonkey standard practice for classes derived from JSObject defines
ClassOps hooks as static member functions.
Depends on D28782
Assignee | ||
Comment 3•6 years ago
|
||
This function probably predates the existence of the DebuggerFrame class, and
was never moved in.
Depends on D28783
Assignee | ||
Comment 4•6 years ago
|
||
These function probably predate the existence of the DebuggerFrame class, and
were never moved in.
Depends on D28784
Assignee | ||
Comment 5•6 years ago
|
||
This function probably predates the existence of the DebuggerFrame class, and
was never moved in.
Depends on D28785
Assignee | ||
Comment 6•6 years ago
|
||
Assignee | ||
Comment 7•6 years ago
|
||
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
Assignee | ||
Comment 8•6 years ago
|
||
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.
Updated•6 years ago
|
Updated•6 years ago
|
Comment 10•6 years ago
|
||
bugherder |
Assignee | ||
Comment 11•6 years ago
|
||
Reopening - not all patches are landed.
Comment 12•6 years ago
|
||
Updated•6 years ago
|
Comment 13•6 years ago
|
||
bugherder |
Assignee | ||
Comment 14•6 years ago
|
||
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•6 years ago
|
||
Comment 16•6 years ago
|
||
Comment 17•6 years ago
|
||
Comment 18•6 years ago
|
||
Comment 19•6 years ago
|
||
bugherder |
Updated•6 years ago
|
Assignee | ||
Comment 20•6 years ago
|
||
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•6 years ago
|
||
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.
Updated•6 years ago
|
Assignee | ||
Updated•6 years ago
|
Description
•