Closed Bug 1551176 Opened 6 months ago Closed 5 months ago

Assertion failure: liveStepperCount + suspendedStepperCount == trappingScript->stepModeCount(), at js/src/vm/Debugger.cpp:2448

Categories

(Core :: JavaScript Engine, defect, P1, critical)

x86_64
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla69
Tracking Status
firefox-esr60 --- unaffected
firefox67 --- wontfix
firefox68 --- wontfix
firefox69 --- fixed

People

(Reporter: decoder, Assigned: jimb)

References

(Blocks 2 open bugs, Regression)

Details

(4 keywords, Whiteboard: [jsbugmon:update][debugger-mvp])

Attachments

(9 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
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review

The following testcase crashes on mozilla-central revision d51f3432e142 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --disable-profiling --enable-debug --enable-optimize, run with --fuzzing-safe --ion-offthread-compile=off):

gczeal(2);
let g = newGlobal({newCompartment: true});
g.eval(`
  function* f() {}
`);
function test() {
  let dbg = new Debugger(g);
  dbg.onEnterFrame = frame => {
    frame.onStep = () => {};
  };
  result = g.f();
}
for (let ttl = 0; ttl < 10, !test(); ttl++) {}

Backtrace:

received signal SIGSEGV, Segmentation fault.
#0  js::Debugger::onSingleStep (cx=<optimized out>, vp=...) at js/src/vm/Debugger.cpp:2447
#1  0x00005555558edcd7 in Interpret (cx=0x7ffff5f19000, state=...) at js/src/vm/Interpreter.cpp:1887
#2  0x00005555558f1446 in js::RunScript (cx=0x7ffff5f19000, state=...) at js/src/vm/Interpreter.cpp:423
#3  0x00005555558f1c8f in js::InternalCallOrConstruct (cx=<optimized out>, cx@entry=0x7ffff5f19000, args=..., construct=construct@entry=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:563
#4  0x00005555558f210d in InternalCall (cx=cx@entry=0x7ffff5f19000, args=...) at js/src/vm/Interpreter.cpp:590
#5  0x00005555558f2280 in js::Call (cx=cx@entry=0x7ffff5f19000, fval=..., fval@entry=..., thisv=..., args=..., rval=...) at js/src/vm/Interpreter.cpp:606
#6  0x0000555555e7fc62 in js::ForwardingProxyHandler::call (this=<optimized out>, cx=0x7ffff5f19000, proxy=..., args=...) at js/src/proxy/Wrapper.cpp:162
#7  0x0000555555e69713 in js::CrossCompartmentWrapper::call (this=0x555557c52200 <js::CrossCompartmentWrapper::singleton>, cx=<optimized out>, wrapper=..., args=...) at js/src/proxy/CrossCompartmentWrapper.cpp:237
#8  0x0000555555e75f75 in js::Proxy::call (cx=0x7ffff5f19000, proxy=proxy@entry=..., args=...) at js/src/proxy/Proxy.cpp:504
#9  0x00005555558f1ed6 in js::InternalCallOrConstruct (cx=<optimized out>, cx@entry=0x7ffff5f19000, args=..., construct=construct@entry=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:509
#10 0x00005555558f210d in InternalCall (cx=0x7ffff5f19000, args=...) at js/src/vm/Interpreter.cpp:590
#11 0x00005555558e33e0 in js::CallFromStack (args=..., cx=<optimized out>) at js/src/vm/Interpreter.cpp:594
#12 Interpret (cx=0x7ffff5f19000, state=...) at js/src/vm/Interpreter.cpp:3082
#13 0x00005555558f1446 in js::RunScript (cx=0x7ffff5f19000, state=...) at js/src/vm/Interpreter.cpp:423
[...]
#22 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at js/src/shell/js.cpp:11370
rax	0x555557c9c980	93825033423232
rbx	0x7fffffffb710	140737488336656
rcx	0x555556b80ea8	93825015484072
rdx	0x0	0
rsi	0x7ffff6eeb770	140737336227696
rdi	0x7ffff6eea540	140737336223040
rbp	0x7fffffffbc40	140737488337984
rsp	0x7fffffffb640	140737488336448
r8	0x7ffff6eeb770	140737336227696
r9	0x7ffff7fe6cc0	140737354034368
r10	0x58	88
r11	0x7ffff6b927a0	140737332717472
r12	0x7fffffffb750	140737488336720
r13	0x11	17
r14	0x27ef858ce100	43909691269376
r15	0x7fffffffb7c0	140737488336832
rip	0x555555a479c0 <js::Debugger::onSingleStep(JSContext*, JS::MutableHandle<JS::Value>)+1920>
=> 0x555555a479c0 <js::Debugger::onSingleStep(JSContext*, JS::MutableHandle<JS::Value>)+1920>:	movl   $0x0,0x0
   0x555555a479cb <js::Debugger::onSingleStep(JSContext*, JS::MutableHandle<JS::Value>)+1931>:	ud2
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/6c8949f053e0
user:        Jason Orendorff
date:        Tue Oct 23 23:24:11 2018 +0000
summary:     Bug 1448880 - Part 6: Re-enable stepping when an async or generator frame with an .onStep hook is resumed. r=jimb

This iteration took 385.713 seconds to run.

Jason, is bug 1448880 a likely regressor?

Flags: needinfo?(jorendorff)
Regressed by: 1448880

Yes. I'll look at this today.

Not a JIT bug. The command-line options are irrelevant. Simpler:

let g = newGlobal({newCompartment: true});
g.eval(`function* f() {}`);
for (let i = 0; i < 2; i++) {
  let dbg = new Debugger(g);
  dbg.onEnterFrame = frame => {
    frame.onStep = () => {};
  };
  g.f();
  dbg.onEnterFrame = undefined;
  gc();
}

Here's the assertion that's failing.

    MOZ_ASSERT(liveStepperCount + suspendedStepperCount ==
               trappingScript->stepModeCount());

The assertion failure happens on the second iteration: the first time we pause for single-stepping after gc().

After GC, suspendedStepperCount is 0, which makes sense: all of the generator objects and corresponding Debugger.Frame objects should have been collected. But the trappingScript->stepModeCount() is not being decremented, because we just don't do that. Oops.

Flags: needinfo?(jorendorff)

Jim, GC can collect a generator and the corresponding Debugger.Frame, which logically ought to decrement the step count on the script. I guess from DebuggerFrame::finalize. Are we having fun yet?

The same GC can also collect the script. We can detect that, though. What do you think? Any chance it's safe to call decrementStepModeCount() from a finalizer? Is there a better way?

Flags: needinfo?(jimb)
Blocks: 1539654

This is, delightfully, what I was just talking with tcampbell about how to fix! The same problem arises even without stepping, in bug 1539654, because any generator frame being observed by a Debugger.Frame needs 'resume' instrumentation (i.e., JSOP_AFTERYIELD actually needs to have code generated for it), but if the generator and the Debugger.Frame are both unreachable, then the script need no longer be marked as a debuggee.

The challenge is that DebuggerFrame::finalize can't actually go and tweak the JSScript directly, since if they're all garbage it's not safe to touch the JSScript, and we have no reliable way to tell if the JSScript is garbage or not: I just learned that IsAboutToBeFinalized isn't safe to apply to a DebuggerFrame's pointer to a JSScript when both are being swept.

The fix, I think, is for DebugScript, the little struct that holds the reasons a JSScript might be a debuggee (breakpoints, stepper count, and now generator observer count), to actually be shared by the JSScript and a generator call Debugger.Frame, with a reference count. Then it can serve as a safe place for communication between JSScript and Debugger.Frame, since it will always outlive all them.

DebugScript will contain a backpointer to its JSScript, which JSScript::finalize will clear. Debugger.Frame's finalizer can check the DebugScript, decrement the right counts, and if the backpointer is present, toggle debug traps or what-have-you.

Flags: needinfo?(jimb)
Assignee: nobody → jimb
Priority: -- → P1
Whiteboard: [jsbugmon:update] → [jsbugmon:update][debugger-mvp]

Giving DebugScript a needed method makes the tests for cleaning it up a little
neater, and will help us add more doodads to it in subsequent patches.

All extant calls to JSScript::ensureDebugScript are immediately followed by a
call to JSScript::debugScript. A fallible getOrCreate interface is cleaner for
the callers, and not much more work in the callee.

Depends on D32266

The present JSScript::setNewStepMode method deals with both increments and
decrements. This provides a single site from which to call
BaselineScript::toggleDebugTraps. But it also checks whether it should free the
DebugScript, which is only needed when we're decrementing, and requires
incrementStepModeCount to furnish a FreeOp which is never needed.

On the balance, removing setNewStepMode altogether and letting
JSScript::incrementStepModeCount and decrementStepModeCount each specialize in
building things up or tearing things down seems cleaner, even if both need to
call toggleDebugTraps.

Depends on D32267

This field of js::DebugScript is a count of the number of Debugger.Frames with
onPop handlers that apply to the given script, and its name should reflect that
more directly. All accessors and mutators renamed accordingly.

Depends on D32268

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 that points to a new
GeneratorInfo class that holds a cross-compartment wrapper to the generator
object.

Depends on D32269

In later patches in the series, DebuggerFrame needs to be able to access a
generator's script even when the generator object itself is being finalized, so
it's simpler to just hold a reference to it directly.

Depends on D32271

A Debugger.Frame for a generator or async call continues to refer to the same
call across suspensions (awaits and yields). This means that, even as the
underlying concrete frames (InterpreterFrame, BaselineFrame) come and go, the
Debugger.Frame retains its relationship with a particular
AbstractGeneratorObject. When that generator is resumed, the Debugger.Frame
acquires the new concrete frame as its new referent.

Normally, when a stack frame is popped, if it had a Debugger.Frame with an
onStep handler, we decrement the frame's script's stepper count, since that
Debugger.Frame's onStep handler is obviously not going to fire any more; the
frame is dead. But in the case of a generator or async frame, the generator call
may be resumed at some point, so for such frames, we leave the script's stepper
count incremented until the generator call returns, throws, or otherwise exits
permanently.

This means that if a Debugger.Frame and its AbstractGeneratorObject are GC'd, we
must decrement the generator's script's stepper count. Of course, the script
itself may also be being GC'd, in which case we need not do anything.

This patch makes DebuggerFrame::clearGenerator solely responsible for dropping
the stepper count on generator frames. Since DebuggerFrame::finalize already
clears the frame's generator, this takes care of the stepper count automatically.

Depends on D32272

Status: NEW → 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.

Pushed by jblandy@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1bfe1a0b50fb
Add js::DebugScript::needed method. r=jorendorff
Pushed by jblandy@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4b3431481d55
JSScript::getOrCreateDebugScript replaces ensureDebugScript. r=jorendorff
Status: ASSIGNED → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla69
Pushed by jblandy@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1b5f7804e104
Clean up js::DebugScript and step mode count. r=jorendorff
Pushed by jblandy@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/73c98da145a7
Rename js::DebugScript::stepMode to 'stepperCount'. r=jorendorff
Status: RESOLVED → REOPENED
Keywords: leave-open
Resolution: FIXED → ---

All patches on this bug must land before it is fixed.

Status: REOPENED → ASSIGNED
Pushed by jblandy@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/71e4a3f2dc39
Add missing `std::move` calls to CrossCompartmentKey constructors. r=jorendorff
Pushed by jblandy@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b65e48539b71
Add GENERATOR_INFO_SLOT to js::DebuggerFrame. r=jorendorff
Regressions: 1557343
Pushed by jblandy@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/eb86b4006925
Use DebuggerFrame::generator instead of GetGeneratorObjectForFrame where possible. r=jorendorff
Pushed by jblandy@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b7a24be78d82
Make DebuggerFrame::GeneratorInfo retain a pointer to the generator script. r=jorendorff
Pushed by jblandy@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/411716c0fa8e
Drop a generator script's stepper count when its Debugger.Frame is GC'd. r=jorendorff
Keywords: leave-open
Regressions: 1559062
Regressions: 1560754
You need to log in before you can comment on or make changes to this bug.