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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Christian Holler (:decoder)]

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

Comment 1

[Fuzzing Team]

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.

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Jason, is bug 1448880 a likely regressor?

Comment 3

[Jason Orendorff [:jorendorff]]

Yes. I'll look at this today.

Comment 4

[Jason Orendorff [:jorendorff]]

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.

Comment 5

[Jason Orendorff [: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?

Comment 6

[Jim Blandy :jimb]

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.

Comment 7

[Jim Blandy :jimb]

Attachment: Bug 1551176: Add js::DebugScript::needed method. r?jorendorff

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.

Comment 8

[Jim Blandy :jimb]

Attachment: Bug 1551176: JSScript::getOrCreateDebugScript replaces ensureDebugScript. r?jorendorff

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

Comment 9

[Jim Blandy :jimb]

Attachment: Bug 1551176: Clean up js::DebugScript and step mode count. r?jorendorff

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

Comment 10

[Jim Blandy :jimb]

Attachment: Bug 1551176: Rename js::DebugScript::stepMode to 'stepperCount'. r?jorendorff

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

Comment 11

[Jim Blandy :jimb]

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

Depends on D32269

Comment 12

[Jim Blandy :jimb]

Attachment: Bug 1551176: Make DebuggerFrame::GeneratorInfo retain a pointer to the generator script. r?jorendorff

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

Comment 13

[Jim Blandy :jimb]

Attachment: Bug 1551176: Drop a generator script's stepper count when its Debugger.Frame is GC'd. r?jorendorff

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

Comment 14

[Jim Blandy :jimb]

Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=11c7f5777966b2b85471e7490658a9d28a5be883

Comment 15

[Jim Blandy :jimb]

Attachment: Bug 1551176: 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.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1bfe1a0b50fb
Add js::DebugScript::needed method. r=jorendorff

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4b3431481d55
JSScript::getOrCreateDebugScript replaces ensureDebugScript. r=jorendorff

Comment 18

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/1bfe1a0b50fb
https://hg.mozilla.org/mozilla-central/rev/4b3431481d55

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1b5f7804e104
Clean up js::DebugScript and step mode count. r=jorendorff

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/73c98da145a7
Rename js::DebugScript::stepMode to 'stepperCount'. r=jorendorff

Comment 21

[Jim Blandy :jimb]

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

Comment 22

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/1b5f7804e104
https://hg.mozilla.org/mozilla-central/rev/73c98da145a7

Comment 23

[Jim Blandy :jimb]

Attachment: Bug 1551176: Add missing `std::move` calls to CrossCompartmentKey constructors. r?jorendorff

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/71e4a3f2dc39
Add missing `std::move` calls to CrossCompartmentKey constructors. r=jorendorff

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b65e48539b71
Add GENERATOR_INFO_SLOT to js::DebuggerFrame. r=jorendorff

Comment 26

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/71e4a3f2dc39
https://hg.mozilla.org/mozilla-central/rev/b65e48539b71

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/eb86b4006925
Use DebuggerFrame::generator instead of GetGeneratorObjectForFrame where possible. r=jorendorff

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b7a24be78d82
Make DebuggerFrame::GeneratorInfo retain a pointer to the generator script. r=jorendorff

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/411716c0fa8e
Drop a generator script's stepper count when its Debugger.Frame is GC'd. r=jorendorff

Comment 30

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/eb86b4006925
https://hg.mozilla.org/mozilla-central/rev/b7a24be78d82
https://hg.mozilla.org/mozilla-central/rev/411716c0fa8e