Closed Bug 1270278 Opened 4 years ago Closed 2 years ago

Assertion failure: !cx->isExceptionPending(), at js/src/builtin/TestingFunctions.cpp:1315

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
Tracking Status
firefox49 --- affected

People

(Reporter: gkw, Unassigned)

References

(Blocks 2 open bugs)

Details

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

Attachments

(3 files)

The following testcase crashes on mozilla-central revision 369a5ee3a288 (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/profiler/bug1242840.js
oomTest(function() {
    try {
        for (x of y);
    } catch (e) {
        x
    }
})

Backtrace:

0   js-dbg-64-dm-clang-darwin-369a5ee3a288	0x000000010e945360 OOMTest(JSContext*, unsigned int, JS::Value*) + 1760 (TestingFunctions.cpp:1315)
1   js-dbg-64-dm-clang-darwin-369a5ee3a288	0x000000010e77f27e js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) + 222 (jscntxtinlines.h:236)
2   js-dbg-64-dm-clang-darwin-369a5ee3a288	0x000000010e7717ce js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) + 702 (Interpreter.cpp:468)
3   js-dbg-64-dm-clang-darwin-369a5ee3a288	0x000000010e7666e2 Interpret(JSContext*, js::RunState&) + 48322 (Interpreter.cpp:2831)
4   js-dbg-64-dm-clang-darwin-369a5ee3a288	0x000000010e75a957 js::RunScript(JSContext*, js::RunState&) + 519 (Interpreter.cpp:426)
5   js-dbg-64-dm-clang-darwin-369a5ee3a288	0x000000010e772fd4 js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::AbstractFramePtr, JS::Value*) + 1124 (Interpreter.cpp:704)
/snip

For detailed crash information, see attachment.
=== Treeherder Build Bisection Results by autoBisect ===

The "good" changeset has the timestamp "20160429102348" and the hash "1d4795f0b4e28db3ddf6a5751653c179fef68726".
The "bad" changeset has the timestamp "20160429103935" and the hash "9e97e2282142b206ef97d13045eac502b58201ed".

Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=1d4795f0b4e28db3ddf6a5751653c179fef68726&tochange=9e97e2282142b206ef97d13045eac502b58201ed

Terrence, is bug 1267412 a likely regressor?
Blocks: 1267412
Flags: needinfo?(terrence)
No. It's not. I can take a look tomorrow though.
(In reply to Terrence Cole [:terrence] from comment #4)
> No. It's not. I can take a look tomorrow though.

Whoa, it actually totally is my fault. Good find, Gary! Patch coming soon.
Flags: needinfo?(terrence)
Terrence, just wondering if this slipped your radar? (It's been ~3 weeks with no assignment)
Flags: needinfo?(terrence)
I got distracted trying to solve the larger problem because I thought that my patch had changed the alloc policy on these vectors. I was wrong; it looks like AutoValueVector is already using TempAllocPolicy, so this is a pre-existing condition. The one difference I see is that AutoValueVector is overriding |resize| and calling resizeInfallible internally, which does some very slightly different things deep in Vector.h. In any case, the right fix is to either make DebugScopes::onPopCall properly fallible or to change the underlying type of these vectors.
As per discussion in IRC.
Assignee: nobody → terrence
Status: NEW → ASSIGNED
Flags: needinfo?(terrence)
Attachment #8757073 - Flags: review?(shu)
Comment on attachment 8757073 [details] [diff] [review]
bug-1270278-v0.diff

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

r=me with the clearPendingException in that method should also be changed to recoverFromOOM.
Attachment #8757073 - Flags: review?(shu) → review+
Oh man, this is *such* a great test case. It now passes with the args above, but fails with --tbpl. There's a second bug here somewhere in addition to the first.
Right, so the problem here is that we OOM inside some baseline code. This is fine. Since the debugger is active, we go to recompile the stack with debugging enabled. This also OOMs. This is less fine. We've decided (as policy) that any OOM inside the debug recompilation infrastructure should not crash, but should not be catchable either to avoid recursing in the error handler when we OOM small. So the error that eventually gets propagated up is uncatchable.

It turns out we have infrastructure for this in the form of --fuzzing-safe, which we can apparently just dump into the header.
Sorry, I had to back this out because it turned jit-test and xpcshell tests all sorts of interesting colors.

On debug builds, I see:

 Assertion failure: maybecx->isThrowingOutOfMemory(), at /home/worker/workspace/build/src/js/src/jscntxt.cpp:950 

on these jit-tests:

 tests/jit-test/jit-test/tests/baseline/bug842431-1.js
 tests/jit-test/jit-test/tests/debug/Frame-eval-29.js
 tests/jit-test/jit-test/tests/debug/Frame-onPop-error-scope-unwind-01.js
 tests/jit-test/jit-test/tests/debug/Frame-onPop-error-scope-unwind-02.js 
 
on these devtools tests:

 devtools/client/debugger/test/mochitest/browser_dbg_source-maps-04.js
 
on these xpcshell unit tests:

 devtools/server/tests/unit/test_stepping-06.js
 devtools/server/tests/unit/test_pause_exceptions-01.js
 devtools/server/tests/unit/test_pause_exceptions-02.js
 devtools/server/tests/unit/test_ignore_no_interface_exceptions.js
 devtools/server/tests/unit/test_ignore_caught_exceptions.js 

There are also opt jit-test failures on Linux64 in tests/jit-test/jit-test/tests/baseline/bug842431-1.js

Might be some other stuff too; I didn't dig through all the oranges.
Flags: needinfo?(terrence)
Wow, that's horrifying! Seems like this was *much* less isolated than I thought. Thanks for the backout, I'll be sure to Try before relanding.
Flags: needinfo?(terrence)
Terrence, just helping ensure this didn't slip through the cracks, so re-setting needinfo?.
Flags: needinfo?(terrence)
Thanks!

I'm probably going to wait on these fiddly is-exception-pending bugs until after errormageddon lands.
Whiteboard: [jsbugmon:update] → [jsbugmon:update,ignore]
JSBugMon: The testcase found in this bug no longer reproduces (tried revision 82e1f1b9c055).
I'm not going to be working on this in the immediate future. I still think errormageddon is the right solution.
Assignee: terrence → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(terrence)
As per bug 1277368 comment 52 - Jan, is this issue likely fixed by the patch already landed there?
Flags: needinfo?(jdemooij)
(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #20)
> As per bug 1277368 comment 52 - Jan, is this issue likely fixed by the patch
> already landed there?

It's unlikely. Forwarding to :shu since he reviewed the patch that landed here.
Flags: needinfo?(jdemooij) → needinfo?(shu)
The code that originally wasn't reporting OOM has been refactored to use DebugEnviornments::takeFrameSnapshot. I can't reproduce the crash, and a quick glance at all fallible calls inside takeFrameSnapshot call recoverFromOutOfMemory(). Marking FIXED.
Status: NEW → RESOLVED
Closed: 2 years ago
Flags: needinfo?(shu)
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.