Closed Bug 1272908 Opened 6 years ago Closed 6 years ago

Assertion failure: !cx->isExceptionPending(), at js/src/vm/Debugger.cpp:1866

Categories

(Core :: JavaScript Engine, defect)

x86_64
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox49 --- affected
firefox50 --- fixed

People

(Reporter: gkw, Unassigned)

References

Details

(Keywords: assertion, testcase, Whiteboard: [jsbugmon:update])

Attachments

(3 files)

The following testcase crashes on mozilla-central revision c3f5e6079284 (build with --enable-debug --enable-more-deterministic, run with --fuzzing-safe --no-threads --no-baseline --no-ion):

// Adapted from randomly chosen test: js/src/jit-test/tests/modules/bug-1233915.js
g = newGlobal();
g.parent = this;
g.eval("(" + function() {
    Debugger(parent).onExceptionUnwind = function(frame)
    frame.eval("")
} + ")()");
// Adapted from randomly chosen test: js/src/jit-test/tests/debug/bug1254123.js
function ERROR(msg) {
    throw new Error("boom");
}
var dbg = new Debugger;
dbg.onNewGlobalObject = ERROR;
oomTest(function() {
    newGlobal();
})

Backtrace:

0   js-dbg-64-dm-clang-darwin-c3f5e6079284	0x000000010d4d441b js::Debugger::fireNewGlobalObject(JSContext*, JS::Handle<js::GlobalObject*>, JS::MutableHandle<JS::Value>) + 923 (Debugger.cpp:1866)
1   js-dbg-64-dm-clang-darwin-c3f5e6079284	0x000000010d4d46d1 js::Debugger::slowPathOnNewGlobalObject(JSContext*, JS::Handle<js::GlobalObject*>) + 609 (Debugger.cpp:1915)
2   js-dbg-64-dm-clang-darwin-c3f5e6079284	0x000000010d2ce561 JS_FireOnNewGlobalObject(JSContext*, JS::Handle<JSObject*>) + 97 (RootingAPI.h:667)
3   js-dbg-64-dm-clang-darwin-c3f5e6079284	0x000000010cd8eb39 NewGlobalObject(JSContext*, JS::CompartmentOptions&, JSPrincipals*) + 1065 (js.cpp:6623)
4   js-dbg-64-dm-clang-darwin-c3f5e6079284	0x000000010cd9a131 NewGlobal(JSContext*, unsigned int, JS::Value*) + 993 (RootingAPI.h:661)

For detailed crash information, see attachment.
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/9e97e2282142
parent:      295477:1d4795f0b4e2
user:        Terrence Cole
date:        Mon Apr 25 15:42:19 2016 -0700
summary:     Bug 1267412 - Use MutableHandleValue instead of pointer-to-AutoValueVector; r=sfink

Terrence, is bug 1267412 a likely regressor?
Blocks: 1267412
Flags: needinfo?(terrence)
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/038ba23ecf52
user:        Eric Faust
date:        Tue May 03 03:28:31 2016 -0700
summary:     Bug 1254190 - Propagate failure of matchAllDebuggeeGlobals() in Debugger. (r=shu)

A separate bisection for an earlier unreduced version of the testcase points to bug 1254190 instead. Not sure which one is correct.
Flags: needinfo?(efaustbmo)
Attached patch FixSplinter Review
More OOM fixes. Wheeeeeee.
Flags: needinfo?(terrence)
Flags: needinfo?(efaustbmo)
Attachment #8754161 - Flags: review?(shu)
Comment on attachment 8754161 [details] [diff] [review]
Fix

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

::: js/src/vm/ScopeObject.cpp
@@ +2736,5 @@
>           * but it simplifies later indexing logic.
>           */
>          Rooted<GCVector<Value>> vec(cx, GCVector<Value>(cx));
> +        if (!frame.copyRawFrameSlots(&vec)) {
> +            cx->recoverFromOutOfMemory();

Policy question. Is recoverFromOutOfMemory for things that we shouldn't care about OOMing? I can't really get in my head when this would not adversely interact with other parts of the system.
Attachment #8754161 - Flags: review?(shu) → review+
Flags: needinfo?(jorendorff)
Pushed by efaustbmo@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/30406bf293ec
Fix OOM in DebugScopes::onPopCall(). (r=shu)
https://hg.mozilla.org/mozilla-central/rev/30406bf293ec
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
(In reply to Shu-yu Guo [:shu] from comment #6)
> > +            cx->recoverFromOutOfMemory();
> 
> Policy question. Is recoverFromOutOfMemory for things that we shouldn't care
> about OOMing? I can't really get in my head when this would not adversely
> interact with other parts of the system.

That is not so far-fetched: all OOM returns are supposed to leave the universe in a good state. If you can't, you must oom-crash.

So whenever you are trying something that doesn't really need to succeed, it's OK to call this method and forget that OOM happened. It so happens that the JS engine is full of "fast paths" that don't really need to succeed, because there's a corresponding slow path handy.

People tend to use recoverFromOutOfMemory when hacking on a function that's currently infallible. They hack in some optimization that needs to allocate memory. If the allocation fails, then what? Maybe you should make the function fallible and change all its call sites. But you could also just recover from OOM, give up on doing that optimization, and move on. It's not so bad.
Flags: needinfo?(jorendorff)
You need to log in before you can comment on or make changes to this bug.