1385844 - Assertion failure: frame.isDebuggee(), at js/src/vm/Debugger-inl.h:18

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 8b577b152383 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-stdcxx-compat --disable-profiling --enable-debug --enable-optimize, run with --fuzzing-safe):

var g = newGlobal([]);
var dbg = Debugger(g);
dbg.onExceptionUnwind = function(f, x) {
    var o = {}
    var global = this;
    var p = new Proxy(o, {
        "deleteProperty": function(await, key) {
            var g = newGlobal();
            g.parent = global;
            g.eval("var dbg = new Debugger(parent); dbg.onEnterFrame = function(frame) {};");
        }
    })
    assertEq(delete p.foo, true);
};
dbg.onDebuggerStatement = function(f) {
    assertEq(f.eval('throw 42'), null);
};
g.eval('debugger');


Backtrace:

 received signal SIGSEGV, Segmentation fault.
0x0000000000545636 in js::Debugger::onLeaveFrame (cx=0x7ffff6924000, frame=..., pc=0x7ffff44dd789 "\231\230\220\004ِ\t\224\377\377\377", <incomplete sequence \367\234>, ok=true) at js/src/vm/Debugger-inl.h:22
#0  0x0000000000545636 in js::Debugger::onLeaveFrame (cx=0x7ffff6924000, frame=..., pc=0x7ffff44dd789 "\231\230\220\004ِ\t\224\377\377\377", <incomplete sequence \367\234>, ok=true) at js/src/vm/Debugger-inl.h:22
#1  0x000000000053793e in Interpret (cx=0x7ffff6924000, state=...) at js/src/vm/Interpreter.cpp:4325
#2  0x000000000053d373 in js::RunScript (cx=0x7ffff6924000, state=...) at js/src/vm/Interpreter.cpp:409
#3  0x000000000053d8c6 in js::InternalCallOrConstruct (cx=cx@entry=0x7ffff6924000, args=..., construct=construct@entry=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:487
#4  0x000000000053db78 in InternalCall (cx=cx@entry=0x7ffff6924000, args=...) at js/src/vm/Interpreter.cpp:514
#5  0x000000000053dcad in js::Call (cx=cx@entry=0x7ffff6924000, fval=..., fval@entry=..., thisv=..., thisv@entry=..., args=..., rval=..., rval@entry=...) at js/src/vm/Interpreter.cpp:533
#6  0x0000000000aa4878 in js::Call (cx=0x7ffff6924000, fval=..., thisObj=<optimized out>, arg0=..., rval=...) at js/src/vm/Interpreter.h:112
#7  0x0000000000b47031 in js::Debugger::fireDebuggerStatement (this=this@entry=0x7ffff693a000, cx=cx@entry=0x7ffff6924000, vp=..., vp@entry=...) at js/src/vm/Debugger.cpp:1784
#8  0x0000000000b47514 in js::Debugger::<lambda(js::Debugger*)>::operator() (dbg=0x7ffff693a000, __closure=<synthetic pointer>) at js/src/vm/Debugger.cpp:1045
#9  js::Debugger::dispatchHook<js::Debugger::slowPathOnDebuggerStatement(JSContext*, js::AbstractFramePtr)::<lambda(js::Debugger*)>, js::Debugger::slowPathOnDebuggerStatement(JSContext*, js::AbstractFramePtr)::<lambda(js::Debugger*)> > (fireHook=..., cx=0x7ffff6924000, hookIsEnabled=...) at js/src/vm/Debugger.cpp:1925
#10 js::Debugger::slowPathOnDebuggerStatement (cx=0x7ffff6924000, frame=...) at js/src/vm/Debugger.cpp:1046
#11 0x000000000053a358 in js::Debugger::onDebuggerStatement (frame=..., frame@entry=..., cx=<optimized out>) at js/src/vm/Debugger-inl.h:58
#12 Interpret (cx=0x7ffff6924000, state=...) at js/src/vm/Interpreter.cpp:3942
#13 0x000000000053d373 in js::RunScript (cx=0x7ffff6924000, state=...) at js/src/vm/Interpreter.cpp:409
#14 0x0000000000540209 in js::ExecuteKernel (cx=cx@entry=0x7ffff6924000, script=..., script@entry=..., envChainArg=..., newTargetValue=..., evalInFrame=..., evalInFrame@entry=..., result=result@entry=0x7fffffffc920) at js/src/vm/Interpreter.cpp:698
#15 0x0000000000576dd6 in EvalKernel (cx=cx@entry=0x7ffff6924000, v=..., evalType=evalType@entry=INDIRECT_EVAL, caller=..., env=..., pc=pc@entry=0x0, vp=...) at js/src/builtin/Eval.cpp:328
#16 0x0000000000577168 in js::IndirectEval (cx=0x7ffff6924000, argc=<optimized out>, vp=<optimized out>) at js/src/builtin/Eval.cpp:421
#17 0x00000000005486db in js::CallJSNative (cx=cx@entry=0x7ffff6924000, native=0x5770a0 <js::IndirectEval(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/jscntxtinlines.h:293
#18 0x000000000053d7ab in js::InternalCallOrConstruct (cx=cx@entry=0x7ffff6924000, args=..., construct=construct@entry=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:469
#19 0x000000000053db78 in InternalCall (cx=cx@entry=0x7ffff6924000, args=...) at js/src/vm/Interpreter.cpp:514
#20 0x000000000053dcad in js::Call (cx=cx@entry=0x7ffff6924000, fval=..., fval@entry=..., thisv=..., args=..., rval=...) at js/src/vm/Interpreter.cpp:533
#21 0x0000000000aa397d in js::Wrapper::call (this=this@entry=0x1f1b070 <js::CrossCompartmentWrapper::singleton>, cx=cx@entry=0x7ffff6924000, proxy=..., proxy@entry=..., args=...) at js/src/proxy/Wrapper.cpp:169
#22 0x0000000000a8ac82 in js::CrossCompartmentWrapper::call (this=0x1f1b070 <js::CrossCompartmentWrapper::singleton>, cx=0x7ffff6924000, wrapper=..., args=...) at js/src/proxy/CrossCompartmentWrapper.cpp:359
#23 0x0000000000a9b56a in js::Proxy::call (cx=cx@entry=0x7ffff6924000, proxy=proxy@entry=..., args=...) at js/src/proxy/Proxy.cpp:481
#24 0x0000000000a9b65c in js::proxy_Call (cx=0x7ffff6924000, argc=<optimized out>, vp=<optimized out>) at js/src/proxy/Proxy.cpp:741
#25 0x00000000005486db in js::CallJSNative (cx=cx@entry=0x7ffff6924000, native=0xa9b5e0 <js::proxy_Call(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/jscntxtinlines.h:293
#26 0x000000000053dab9 in js::InternalCallOrConstruct (cx=0x7ffff6924000, args=..., construct=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:451
[...]
#38 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at js/src/shell/js.cpp:8559
rax	0x0	0
rbx	0x7fffffffaa60	140737488333408
rcx	0x7ffff6c28a2d	140737333332525
rdx	0x0	0
rsi	0x7ffff6ef7770	140737336276848
rdi	0x7ffff6ef6540	140737336272192
rbp	0x7fffffffaaa0	140737488333472
rsp	0x7fffffffaa60	140737488333408
r8	0x7ffff6ef7770	140737336276848
r9	0x7ffff7fe4740	140737354024768
r10	0x0	0
r11	0x0	0
r12	0x1	1
r13	0x1	1
r14	0x7ffff6924000	140737330167808
r15	0x7ffff44dd789	140737292130185
rip	0x545636 <js::Debugger::onLeaveFrame(JSContext*, js::AbstractFramePtr, unsigned char*, bool)+454>
=> 0x545636 <js::Debugger::onLeaveFrame(JSContext*, js::AbstractFramePtr, unsigned char*, bool)+454>:	movl   $0x0,0x0
   0x545641 <js::Debugger::onLeaveFrame(JSContext*, js::AbstractFramePtr, unsigned char*, bool)+465>:	ud2

Comment 1

[Naveed Ihsanullah [:naveed]]

Jason please take a look for post 57. Thank you.

Comment 2

[Jason Orendorff [:jorendorff]]

Reproduces cleanly. Debugging.

Comment 3

[Jason Orendorff [:jorendorff]]

I reduced it slightly to:

var g1 = this;
var g2 = newGlobal();
var dbg = Debugger(g2);
dbg.onExceptionUnwind = function(f, x) {
    var h = newGlobal();
    h.parent = g1;
    h.eval("var dbg = new Debugger(parent); dbg.onEnterFrame = function(frame) {};");
};
dbg.onDebuggerStatement = function(f) {
    assertEq(f.eval('throw 42').throw, 42);
};
g2.eval('debugger');

Currently building something else, but my guess is that Jan's patch in bug 1385843 fixes this too.

Comment 4

[Jason Orendorff [:jorendorff]]

Nope. Still asserts.

I have a few other bugs to fix before getting back to this.

Comment 5

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

var g1 = this;
var g2 = newGlobal();
var dbg = Debugger(g2);
dbg.onExceptionUnwind = function(f, x) {
    var h = newGlobal();
    h.parent = g1;
    h.eval("var dbg = new Debugger(parent); dbg.onEnterFrame = function(frame) {};");
};
dbg.onDebuggerStatement = function(f) {
    assertEq(f.eval('throw 42').throw, 42);
};
g2.eval('debugger');

jorendorff's testcase in comment 3 still reproduces on m-c rev 31724aea10ca with --fuzzing-safe --no-threads --no-baseline --no-ion at:

Assertion failure: frame.isDebuggee(), at vm/Debugger-inl.h:19

Jason, what's next?

Comment 6

[Liz Henry (:lizzard)]

Wontfix for 64. We could still take a patch for 66 and uplift for 65.

Comment 7

[Fuzzing Team]

JSBugMon: The testcase found in this bug no longer reproduces (tried revision 3dc7d345da52).

Comment 8

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

var g1 = this;
var g2 = newGlobal();
var dbg = Debugger(g2);
dbg.onExceptionUnwind = function(f, x) {
    var h = newGlobal();
    h.parent = g1;
    h.eval("var dbg = new Debugger(parent); dbg.onEnterFrame = function(frame) {};");
};
dbg.onDebuggerStatement = function(f) {
    assertEq(f.eval('throw 42').throw, 42);
};
g2.eval('debugger');

asserts js shell compiled with --enable-debug on m-c rev 8ec327de0ba7 using --fuzzing-safe --no-threads --no-baseline --no-ion --more-compartments at Assertion failure: frame.isDebuggee(), at js/src/vm/Debugger-inl.h:21

Comment 9

[Fuzzing Team]

JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.

Comment 10

[Julien Cristau [:jcristau]]

I think we can drop this from regression triage.

Comment 11

[Fuzzing Team]

JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.

Comment 12

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

Jim mentioned over Slack that he's still the only one taking Debugger fuzzbugs.

Comment 13

[Jim Blandy :jimb]

I can reproduce this. This should be quick.

Comment 14

[Jim Blandy :jimb]

Here's a further simplified test case:

var g1 = this;

var h = newGlobal();
h.parent = g1;
h.eval(`
  var hdbg = new Debugger(parent);
  function j() {
    hdbg.onEnterFrame = function(frame) {};
  }
`);

var g2 = newGlobal();
g2.j = h.j;

var dbg = new Debugger(g2);
var g2DO = dbg.addDebuggee(g2);

dbg.onDebuggerStatement = function(f) {
  f.eval('j()');
};
g2.eval('debugger');

Since an onEnterFrame hook detects calls anywhere in any debuggee realm, setting such a hook entails setting the isDebuggee flag on all stack frames in the debuggee realms. This is the job of Debugger::updateExecutionObservabilityOfFrames.

Unfortunately, that function uses FrameIter to walk the stack. FrameIter respects 'debugger eval prev' links, which make the parent of a frame for a call to Debugger.Frame.prototype.eval to appear to be the Debugger.Frame's referent, not the actual youngest debuggee frame. (Debugger eval prev links are somewhat nonsensical, and perhaps should be removed, but they predate the Debugger API.)

In the test case, the function h.f sets an onEnterFrame hook on a Debugger whose debuggee is the main global running the test script. At that point, the JavaScript stack looks like this (youngest to oldest):

1. In global h, a call to h.j , setting the onEnterFrame hook
2. In global g2, a debugger eval frame evaluating the expression j()
3. In the main global, the onDebuggerStatement handler
4. In global g2, an eval frame evaluating the statement debugger;
5. In the main global, a frame running the test script top level code.

As a debugger eval frame, frame 2 has a debugger eval prev link pointing to frame 4. The FrameIter in Debugger::updateExecutionObservabilityOfFrames follows that link, skipping over frame 3. When we return from frame 3, the assertion notices that the frame's script is marked as a debuggee (setting the onEnterFrame hook set its realm's DebuggerObservesAllExecution flag), but that the frame itself is not. This violates the Debugger's invariant that all frames running debuggee scripts must be themselves debuggee frames.

Comment 15

[Jim Blandy :jimb]

Attachment: Bug 1385844: Ignore 'debugger eval prev' links when marking frames as debuggees. r?jorendorff

Setting a hook on a Debugger may expand the set of behaviors it observes, so
that new scripts and stack frames must have their isDebuggee flags set. The
Debugger::updateExecutionObservabilityOfFrames function is supposed to walk
the stack and sets the flag where necessary.

However, the old code performed that stack walk using FrameIter, which follows
'debugger eval prevlinks, potentially skipping over stack frames that need to be flagged. This patch changes the code to useAllFramesIter, which differs from FrameIter` only in that it ignores 'debugger eval prev' links.

Comment 16

[Jim Blandy :jimb]

JS-centric try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2470668c2c9c37620c0a4686419457814f7a49ba

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1f35e218386e
Ignore 'debugger eval prev' links when marking frames as debuggees. r=jorendorff

Comment 18

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/1f35e218386e