Closed
Bug 1270278
Opened 8 years ago
Closed 7 years ago
Assertion failure: !cx->isExceptionPending(), at js/src/builtin/TestingFunctions.cpp:1315
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
firefox49 | --- | affected |
People
(Reporter: gkw, Unassigned)
References
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.
Reporter | ||
Comment 1•8 years ago
|
||
Reporter | ||
Comment 2•8 years ago
|
||
=== 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)
Reporter | ||
Comment 3•8 years ago
|
||
Comment 4•8 years ago
|
||
No. It's not. I can take a look tomorrow though.
Comment 5•8 years ago
|
||
(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)
Reporter | ||
Comment 6•8 years ago
|
||
Terrence, just wondering if this slipped your radar? (It's been ~3 weeks with no assignment)
Flags: needinfo?(terrence)
Comment 7•8 years ago
|
||
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.
Comment 8•8 years ago
|
||
As per discussion in IRC.
Assignee: nobody → terrence
Status: NEW → ASSIGNED
Flags: needinfo?(terrence)
Attachment #8757073 -
Flags: review?(shu)
Comment 9•8 years ago
|
||
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+
Comment 10•8 years ago
|
||
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.
Comment 11•8 years ago
|
||
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.
Comment 12•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/619ef5aac05fa3dadb656fac5352dc712451c109 Bug 1270278; Handle OOM better in Debugger::onPopCall; r=shu
Comment 13•8 years ago
|
||
Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/1b661134e2ca
Comment 14•8 years ago
|
||
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)
Comment 15•8 years ago
|
||
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)
Reporter | ||
Comment 16•8 years ago
|
||
Terrence, just helping ensure this didn't slip through the cracks, so re-setting needinfo?.
Flags: needinfo?(terrence)
Comment 17•8 years ago
|
||
Thanks! I'm probably going to wait on these fiddly is-exception-pending bugs until after errormageddon lands.
Updated•8 years ago
|
Whiteboard: [jsbugmon:update] → [jsbugmon:update,ignore]
Comment 18•8 years ago
|
||
JSBugMon: The testcase found in this bug no longer reproduces (tried revision 82e1f1b9c055).
Comment 19•8 years ago
|
||
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
Updated•8 years ago
|
Flags: needinfo?(terrence)
Reporter | ||
Comment 20•7 years ago
|
||
As per bug 1277368 comment 52 - Jan, is this issue likely fixed by the patch already landed there?
Flags: needinfo?(jdemooij)
Comment 21•7 years ago
|
||
(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)
Comment 22•7 years ago
|
||
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: 7 years ago
Flags: needinfo?(shu)
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•