1250190 - Assertion failure: !found, at js/src/vm/Debugger.cpp:321

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

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

g = newGlobal();
Debugger(g).onNewPromise = function() g.makeFakePromise();
g.makeFakePromise();



Backtrace:

Program received signal SIGSEGV, Segmentation fault.
0x00000000009d4eb8 in isUniqueLockedInStack (dbg=..., cx=0x7ffff6907800) at js/src/vm/Debugger.cpp:321
#0  0x00000000009d4eb8 in isUniqueLockedInStack (dbg=..., cx=0x7ffff6907800) at js/src/vm/Debugger.cpp:321
#1  js::Debugger::handleUncaughtExceptionHelper (this=this@entry=0x7ffff694d800, ac=..., vp=vp@entry=0x7fffffdfee00, callHook=callHook@entry=true) at js/src/vm/Debugger.cpp:1159
#2  0x00000000009ef351 in handleUncaughtException (callHook=true, vp=..., ac=..., this=0x7ffff694d800) at js/src/vm/Debugger.cpp:1209
#3  js::Debugger::firePromiseHook (this=this@entry=0x7ffff694d800, cx=cx@entry=0x7ffff6907800, hook=hook@entry=js::Debugger::OnNewPromise, promise=..., promise@entry=..., vp=..., vp@entry=...) at js/src/vm/Debugger.cpp:2009
#4  0x00000000009ef72e in operator() (dbg=0x7ffff694d800, __closure=<synthetic pointer>) at js/src/vm/Debugger.cpp:2024
#5  dispatchHook<js::Debugger::slowPathPromiseHook(JSContext*, js::Debugger::Hook, JS::HandleObject)::__lambda11, js::Debugger::slowPathPromiseHook(JSContext*, js::Debugger::Hook, JS::HandleObject)::__lambda12> (fireHook=..., hookIsEnabled=..., cx=0x7ffff6907800) at js/src/vm/Debugger.cpp:1602
#6  js::Debugger::slowPathPromiseHook (cx=cx@entry=0x7ffff6907800, hook=hook@entry=js::Debugger::OnNewPromise, promise=..., promise@entry=...) at js/src/vm/Debugger.cpp:2026
#7  0x00000000009ef8e7 in JS::dbg::onNewPromise (cx=cx@entry=0x7ffff6907800, promise=promise@entry=...) at js/src/vm/Debugger.cpp:8697
#8  0x0000000000a30972 in MakeFakePromise (cx=0x7ffff6907800, argc=<optimized out>, vp=<optimized out>) at js/src/builtin/TestingFunctions.cpp:1345
#9  0x0000000000abf772 in js::CallJSNative (cx=0x7ffff6907800, native=0xa308f0 <MakeFakePromise(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/jscntxtinlines.h:235
[...]
#15 0x00000000009a21ea in js::proxy_Call (cx=<optimized out>, argc=<optimized out>, vp=<optimized out>) at js/src/proxy/Proxy.cpp:683
#16 0x0000000000abf772 in js::CallJSNative (cx=0x7ffff6907800, native=0x9a2140 <js::proxy_Call(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/jscntxtinlines.h:235
[...]
#21 0x0000000000000000 in ?? ()
rax	0x0	0
rbx	0x7ffff694d800	140737330337792
rcx	0x7ffff6ca588d	140737333844109
rdx	0x0	0
rsi	0x7ffff6f7a9d0	140737336814032
rdi	0x7ffff6f791c0	140737336807872
rbp	0x7fffffdfedb0	140737486253488
rsp	0x7fffffdfece0	140737486253280
r8	0x7ffff7fe07c0	140737354008512
r9	0x6372732f736a2f6c	7165916604736876396
r10	0x7fffffdfeaa0	140737486252704
r11	0x7ffff6c27ee0	140737333329632
r12	0x7ffff6907800	140737330051072
r13	0x7fffffdfee30	140737486253616
r14	0x7fffffdfee30	140737486253616
r15	0x7fffffdfee60	140737486253664
rip	0x9d4eb8 <js::Debugger::handleUncaughtExceptionHelper(mozilla::Maybe<js::AutoCompartment>&, JS::MutableHandle<JS::Value>*, bool)+936>
=> 0x9d4eb8 <js::Debugger::handleUncaughtExceptionHelper(mozilla::Maybe<js::AutoCompartment>&, JS::MutableHandle<JS::Value>*, bool)+936>:	movl   $0x141,0x0
   0x9d4ec3 <js::Debugger::handleUncaughtExceptionHelper(mozilla::Maybe<js::AutoCompartment>&, JS::MutableHandle<JS::Value>*, bool)+947>:	callq  0x4a60b0 <abort()>


This is why it's never a good idea to make fake promises.

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

cc'd jonco for promises, ni'd debugger people.

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

owait, till made promises, not jon. Sorry for the spam!

Comment 3

[Jim Blandy :jimb]

This looks like a problem in the new DebuggeeWouldRun code. Shu's on PTO until the end of the week. I can reproduce.

Comment 4

[Jim Blandy :jimb]

This is actually a stack overflow; we're 333 onNewPromise handlers deep by the time we crash.

Comment 5

[Till Schneidereit [:till]]

Right, I'm not sure what should happen in this case. Crashing probably isn't ideal.

With my Promise implementation and the following code, I get a DebuggeeWouldRun exception because parts of the Promise implementation are self-hosted:

g = newGlobal();
Debugger(g).onNewPromise = ()=>new g.Promise(_=>{});
new g.Promise(_=>{})

Comment 6

[Jan de Mooij [:jandem]]

FWIW, bz was just talking about self-hosted Promise + overrecursion errors today: http://logs.glob.uno/?c=mozilla%23jsapi&s=29+Feb+2016&e=29+Feb+2016#c704595 ...

Comment 7

[Jim Blandy :jimb]

The DebuggeeWouldRun logic assumes that if we find ourselves running a Debugger handler function, the Debuggee must not have been locked. All three test cases demonstrate ways to invoke a Debugger's handler --- either the onNewPromise or the onNewGlobalObject handler --- without actually running any debuggee JS.

So here's another way to provoke the same problem: 

var g = newGlobal();
Debugger(g).onNewGlobalObject = function() g.newGlobal();
g.newGlobal();

Like the test case in comment 0, this blows the stack; Debugger doesn't notice that it's been locking the same Debugger instance over and over again until we get the over-recursed termination, treat that as an uncaught exception, and run into the assertion in js::Debugger::uncaughtExceptionHandler.

Here's another way that doesn't entail blowing the stack:

var g = newGlobal();
var dbg = new Debugger(g);
dbg.onNewGlobalObject = function () {
  dbg.onNewGlobalObject = function () { throw "yadda"; };
  newGlobal();
}
newGlobal();

Comment 8

[Jim Blandy :jimb]

(In reply to Jan de Mooij [:jandem] from comment #6)
> FWIW, bz was just talking about self-hosted Promise + overrecursion errors
> today:
> http://logs.glob.uno/?c=mozilla%23jsapi&s=29+Feb+2016&e=29+Feb+2016#c704595
> ...

I don't think that's relevant, since this is entirely an issue with the new DebuggeeWouldRun detection.

Comment 9

[Jim Blandy :jimb]

You can do it with onNewScript too:

var g = newGlobal();
var dbg = new Debugger(g);
dbg.onNewScript = function () {
  dbg.onNewScript = function () { throw "yadda"; };
  g.Function("noodles;");
}
g.Function("poodles;");

Comment 10

[Christian Holler (:decoder)]

This is causing frequent crashes, marking as fuzzblocker.

Comment 11

[Fuzzing Team]

JSBugMon: Bisection requested, result:
=== Treeherder Build Bisection Results by autoBisect ===

The "good" changeset has the timestamp "20160219133522" and the hash "1205efecce10f87c04a9bf2bfb91c6b5cf5f2239".
The "bad" changeset has the timestamp "20160219134321" and the hash "2feba844e67bbf6dddec9578a171b95ee896dfea".

Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=1205efecce10f87c04a9bf2bfb91c6b5cf5f2239&tochange=2feba844e67bbf6dddec9578a171b95ee896dfea

Comment 12

[Christian Holler (:decoder)]

This looks like a regression from shu, but he's on PTO I think. Jim, since you seem to know what the problem is, can you take this? If not, please forward to shu or someone else :)

Comment 13

[Jim Blandy :jimb]

(In reply to Christian Holler (:decoder) from comment #12)
> This looks like a regression from shu, but he's on PTO I think. Jim, since
> you seem to know what the problem is, can you take this? If not, please
> forward to shu or someone else :)

Do check the fields; I assigned this bug to myself in comment 3. :)

Comment 14

[Jim Blandy :jimb]

Attachment: Make DebuggeeWouldRun checks not assume we always unlock before re-locking.x

Hi, Nick. Since Shu is out of town but this is a fuzzblocker, can I lean on you for a review here?

Comment 15

[Jim Blandy :jimb]

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

Comment 16

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8725811 [details] [diff] [review]
Make DebuggeeWouldRun checks not assume we always unlock before re-locking.x

Review of attachment 8725811 [details] [diff] [review]:
-----------------------------------------------------------------

I'm always happy to receive a review request!

::: js/src/jit-test/tests/debug/DebuggeeWouldRun-01.js
@@ +1,4 @@
> +// Bug 1250190: Shouldn't crash. |jit-test| exitstatus: 3
> +
> +g = newGlobal();
> +Debugger(g).onNewPromise = function() g.makeFakePromise();

Nit: don't use non-standard extensions, instead do:

  ... = function () { g.makeFakePromise(); };

or

  ... = () => g.makeFakePromise();

::: js/src/jit-test/tests/debug/DebuggeeWouldRun-02.js
@@ +1,4 @@
> +// Bug 1250190: Shouldn't crash. |jit-test| exitstatus: 3
> +
> +var g = newGlobal();
> +Debugger(g).onNewGlobalObject = function() g.newGlobal();

ditto

Comment 17

[Jim Blandy :jimb]

(In reply to Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] from comment #16)
> > +Debugger(g).onNewPromise = function() g.makeFakePromise();
> 
> Nit: don't use non-standard extensions, instead do:
> 
>   ... = function () { g.makeFakePromise(); };
> 
> or
> 
>   ... = () => g.makeFakePromise();

Oh, good point! Fixed.

Delightfully, both of these could be eta-reduced like this:

    Debugger(g).onNewPromise = g.makeFakePromise

BUT then the tests then take much longer to run than if we wrap it in an arrow function or an ordinary JS function expression. That's right, introducing an interpreted function makes the test fail faster.

Comment 18

[Jim Blandy :jimb]

Okay, will land this once the Try results are in and green.

Comment 19

[Jim Blandy :jimb]

Unfortunately, there's a CGC error on try. The program seems to exit with a success status when run with JS_GC_ZEAL=14, but exit with a failure status when run without JS_GC_ZEAL set. That seems bad.

Comment 20

[Jim Blandy :jimb]

It seems like having a live onNewGlobalObject hook doesn't protect a Debugger from being garbage collected, even though collection is a visible side effect. With JS_GC_ZEAL=14, the Debuggers in the tests were getting GC'd before they ever called their handlers.

Bug 1073753  discusses the problem of collecting Debuggers with live onNewGlobalObject hooks. In the mean time, the DebuggeeWouldRun tests can simply assign their Debuggers to a global variable, which will keep them live.

This also means the CGC tests run DebuggeeWouldRun-01 and -02 very slowly, so I've added them to js/src/devtools/automation/cgc-jittest-timeouts.txt.

New try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=75151a0f1faf

Comment 21

[Jim Blandy :jimb]

Attachment: Make DebuggeeWouldRun checks not assume we always unlock before re-locking. r=fitzgen

Revised patch. Carrying over r=fitzgen.

Comment 22

[Jim Blandy :jimb]

Try push looks good.

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6d6166d70ece

Comment 24

[Fuzzing Team]

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

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6d6166d70ece