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

RESOLVED FIXED in Firefox 47

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: decoder, Assigned: jimb)

Tracking

(Blocks: 1 bug, {assertion, regression, testcase})

Trunk
mozilla47
assertion, regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox47 fixed)

Details

(Whiteboard: [fuzzblocker] [jsbugmon:update])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

2 years ago
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.
cc'd jonco for promises, ni'd debugger people.
Flags: needinfo?(nfitzgerald)
Flags: needinfo?(jimb)
owait, till made promises, not jon. Sorry for the spam!
(Assignee)

Comment 3

2 years ago
This looks like a problem in the new DebuggeeWouldRun code. Shu's on PTO until the end of the week. I can reproduce.
Assignee: nobody → jimb
Flags: needinfo?(nfitzgerald)
Flags: needinfo?(jimb)
(Assignee)

Comment 4

2 years ago
This is actually a stack overflow; we're 333 onNewPromise handlers deep by the time we crash.
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(_=>{})
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 ...
(Assignee)

Comment 7

2 years ago
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();
(Assignee)

Comment 8

2 years ago
(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.
(Assignee)

Comment 9

2 years ago
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;");
(Reporter)

Comment 10

2 years ago
This is causing frequent crashes, marking as fuzzblocker.
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update,bisect][fuzzblocker]

Updated

2 years ago
Whiteboard: [jsbugmon:update,bisect][fuzzblocker] → [fuzzblocker] [jsbugmon:update]

Comment 11

2 years ago
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
(Reporter)

Comment 12

2 years ago
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 :)
Flags: needinfo?(jimb)
(Assignee)

Comment 13

2 years ago
(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. :)
Flags: needinfo?(jimb)
(Assignee)

Comment 14

2 years ago
Created attachment 8725811 [details] [diff] [review]
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?
Attachment #8725811 - Flags: review?(nfitzgerald)
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
Attachment #8725811 - Flags: review?(nfitzgerald) → review+
(Assignee)

Comment 17

2 years ago
(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.
(Assignee)

Comment 18

2 years ago
Okay, will land this once the Try results are in and green.
Blocks: 912337
(Assignee)

Comment 19

2 years ago
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.
(Assignee)

Comment 20

2 years ago
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
(Assignee)

Updated

2 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 21

2 years ago
Created attachment 8726088 [details] [diff] [review]
Make DebuggeeWouldRun checks not assume we always unlock before re-locking. r=fitzgen

Revised patch. Carrying over r=fitzgen.
Attachment #8725811 - Attachment is obsolete: true
(Assignee)

Comment 22

2 years ago
Try push looks good.
(Assignee)

Updated

2 years ago
Flags: in-testsuite+
OS: Linux → All
Hardware: x86_64 → All
Target Milestone: --- → mozilla47

Updated

2 years ago
Whiteboard: [fuzzblocker] [jsbugmon:update] → [fuzzblocker] [jsbugmon:update,ignore]

Comment 24

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

Comment 25

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6d6166d70ece
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox47: affected → fixed
Resolution: --- → FIXED
Whiteboard: [fuzzblocker] [jsbugmon:update,ignore] → [fuzzblocker] [jsbugmon:update]
You need to log in before you can comment on or make changes to this bug.