Assertion failure: !cx->isExceptionPending(), at js/src/jscntxtinlines.h:238

RESOLVED FIXED in Firefox 46

Status

()

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

People

(Reporter: gkw, Assigned: jonco)

Tracking

(Blocks: 2 bugs, {assertion, regression, testcase})

Trunk
mozilla46
x86_64
Mac OS X
assertion, regression, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 affected, firefox46 fixed)

Details

(Whiteboard: [jsbugmon:update])

Attachments

(2 attachments)

(Reporter)

Description

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

// jsfunfuzz-generated
function f1() {}
function f2() {}
r = [function() {}, function() {}, [], function() {}, f1, function() {}, f2];
l = [0];
function f3() {
    return {
        a: 0
    };
}
var x = f3();
// Adapted from randomly chosen test: js/src/jit-test/tests/debug/Object-executeInGlobal-01.js
var h = newGlobal();
var dbg = new Debugger;
dbg.addDebuggee(h);
// Adapted from randomly chosen test: js/src/jit-test/tests/gc/oomInGetJumpLabelForBranch.js
oomTest(() => getBacktrace({
    thisprops: gc()
}));

Backtrace:

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   js-dbg-64-dm-darwin-d08afef8b42d	0x00000001007254b2 js::Invoke(JSContext*, JS::CallArgs const&, js::MaybeConstruct) + 1330 (jscntxtinlines.h:238)
1   js-dbg-64-dm-darwin-d08afef8b42d	0x000000010071a2e1 Interpret(JSContext*, js::RunState&) + 48417 (Interpreter.cpp:2763)
2   js-dbg-64-dm-darwin-d08afef8b42d	0x000000010070e53c js::RunScript(JSContext*, js::RunState&) + 412 (Interpreter.cpp:391)
3   js-dbg-64-dm-darwin-d08afef8b42d	0x00000001007252f9 js::Invoke(JSContext*, JS::CallArgs const&, js::MaybeConstruct) + 889 (Interpreter.cpp:462)
4   js-dbg-64-dm-darwin-d08afef8b42d	0x0000000100725acb js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value const*, JS::MutableHandle<JS::Value>) + 555 (Interpreter.cpp:496)
5   js-dbg-64-dm-darwin-d08afef8b42d	0x0000000100504a1c JS_CallFunction(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSFunction*>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) + 460 (jsapi.cpp:2803)
6   js-dbg-64-dm-darwin-d08afef8b42d	0x00000001006f8290 OOMTest(JSContext*, unsigned int, JS::Value*) + 752 (TestingFunctions.cpp:1167)
7   js-dbg-64-dm-darwin-d08afef8b42d	0x0000000100725292 js::Invoke(JSContext*, JS::CallArgs const&, js::MaybeConstruct) + 786 (jscntxtinlines.h:236)
8   js-dbg-64-dm-darwin-d08afef8b42d	0x000000010071a2e1 Interpret(JSContext*, js::RunState&) + 48417 (Interpreter.cpp:2763)
9   js-dbg-64-dm-darwin-d08afef8b42d	0x000000010070e53c js::RunScript(JSContext*, js::RunState&) + 412 (Interpreter.cpp:391)
10  js-dbg-64-dm-darwin-d08afef8b42d	0x0000000100726787 js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::ExecuteType, js::AbstractFramePtr, JS::Value*) + 583 (Interpreter.cpp:650)
11  js-dbg-64-dm-darwin-d08afef8b42d	0x0000000100726b6f js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) + 495 (RootingAPI.h:719)
12  js-dbg-64-dm-darwin-d08afef8b42d	0x000000010050a191 ExecuteScript(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSScript*>, JS::Value*) + 417 (jsapi.cpp:4410)
13  js-dbg-64-dm-darwin-d08afef8b42d	0x000000010050a402 JS_ExecuteScript(JSContext*, JS::Handle<JSScript*>) + 82 (RootingAPI.h:719)
14  js-dbg-64-dm-darwin-d08afef8b42d	0x000000010001e0f9 Process(JSContext*, char const*, bool, FileKind) + 3273 (js.cpp:515)
15  js-dbg-64-dm-darwin-d08afef8b42d	0x00000001000048e1 main + 11825 (js.cpp:6205)
16  js-dbg-64-dm-darwin-d08afef8b42d	0x0000000100000ea4 start + 52
(Reporter)

Comment 1

3 years ago
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/677cbf860ff2
user:        Eric Faust
date:        Fri Nov 13 18:26:00 2015 -0800
summary:     Bug 1223916 - Prohibit direct method calls at the parser level in self-hosted code. (r=till)

Eric, is bug 1223916 a likely regressor?
Blocks: 1223916
Has Regression Range: --- → yes
Has STR: --- → yes
Flags: needinfo?(efaustbmo)

Comment 2

3 years ago
This is unlikely to be me. Looks like bad OOM handling. What was the other possible bad changeset?
Flags: needinfo?(efaustbmo)
(Reporter)

Comment 3

3 years ago
Created attachment 8697366 [details]
more information

Thanks to :efaust and :jonco for f2f follow-ups and guidance.

Using the instructions in:

https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/Hacking_Tips#How_to_debug_oomTest%28%29_failures

I found that the following part of the resulting stack might be pointing to the real culprit, which might be something related to debuggers.


    frame #4: 0x0000000100929ce2 js-dbg-64-dm-darwin-412e4d7ce98c`JS::Zone::notifyObservingDebuggers() [inlined] bool js::HashSet<unsigned long long, js::DefaultHasher<unsigned long long>, js::TempAllocPolicy>::add<unsigned long long&>(p=0x0000000102e3fbe0, u=0x0000000000000008) + 274 at HashTable.h:385
    frame #5: 0x0000000100929cd4 js-dbg-64-dm-darwin-412e4d7ce98c`JS::Zone::notifyObservingDebuggers() [inlined] bool js::HashSet<unsigned long long, js::DefaultHasher<unsigned long long>, js::TempAllocPolicy>::put<unsigned long long&>(u=0x0000000000000008) + 26 at HashTable.h:458
    frame #6: 0x0000000100929cba js-dbg-64-dm-darwin-412e4d7ce98c`JS::Zone::notifyObservingDebuggers() [inlined] js::Debugger::debuggeeIsBeingCollected(majorGCNumber=8) at Debugger.h:270
    frame #7: 0x0000000100929cba js-dbg-64-dm-darwin-412e4d7ce98c`JS::Zone::notifyObservingDebuggers(this=0x0000000102ebd000) + 234 at Zone.cpp:324


Setting needinfo? from our Debugger gurus, :jimb and :fitzgen.
Flags: needinfo?(nfitzgerald)
Flags: needinfo?(jcoppeard)
(Reporter)

Updated

3 years ago
No longer blocks: 1223916
(Reporter)

Updated

3 years ago
Flags: needinfo?(jcoppeard) → needinfo?(jimb)
(Assignee)

Comment 4

3 years ago
The problem seems to be at the intersection of the debugger and the GC.

The specific issue is that if we hit OOM adding an entry to the degbuger's observedGCs set an error is reported to the context which is not cleared when we ignore this failure.

The more general issue is that the debugger uses a variety of AllocPolicy template arguments to for its hash tables - in this case it's the default TempAllocPolicy which reports allocation failure to the context.  This is actually only valid if the context it's created with is guaranteed to live as long as the table (tied to the lifetime of the debugger object), which is a bit dubious in this case.  But I don't really understand when contexts get created and destroyed anyway.

Anyway I think the answer to this is going to be to change the debugger to use RuntimeAllocPolicy for all its hashtables.
(Assignee)

Updated

3 years ago
Assignee: nobody → jcoppeard
Flags: needinfo?(nfitzgerald)
Flags: needinfo?(jimb)
(Assignee)

Comment 5

3 years ago
Created attachment 8697663 [details] [diff] [review]
bug1231386-debugger-alloc-policy

Not as complicated as I expected.
Attachment #8697663 - Flags: review?(terrence)

Comment 6

3 years ago
It's probably worth checking that the other insertion operations on WeakGlobalObjectSet also don't expect an exception to be set.
(Assignee)

Comment 7

3 years ago
(In reply to Jim Blandy :jimb from comment #6)
Good idea.  I checked and Debugger::debuggees is the only instance of WeakGlobalObjectSet.   Fallible operations on it are performed in Debugger::init() and Debugger::addDebuggeeGlobal() and these already call ReportOutOfMemory() on failure.
Comment on attachment 8697663 [details] [diff] [review]
bug1231386-debugger-alloc-policy

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

I've always wondered about that too! However, RuntimeAllocPolicy does not report failures; don't we need to report explicitly in all the places where we use these tables now? What about at https://dxr.mozilla.org/mozilla-central/source/js/src/vm/Debugger.cpp#3247, for example? That function gets exposed directly to script, so I'm dubious that it's safe to ignore the OOM there.
Attachment #8697663 - Flags: review?(terrence)
Comment on attachment 8697663 [details] [diff] [review]
bug1231386-debugger-alloc-policy

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

My bad. That instance is a local alias, so not relevant. I also verified with Jon that he visited all uses to ensure correct reporting.
Attachment #8697663 - Flags: review+

Comment 10

3 years ago
Thanks for the fix, Jon!

Comment 12

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/83810689970b
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.