Closed Bug 1339999 Opened 6 years ago Closed 6 years ago

Assertion failure: throwing, at js/src/jscntxt.cpp:1092

Categories

(Core :: JavaScript Engine, defect)

Product:
Core
Component:
JavaScript Engine
Platform:
x86_64
macOS
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox52 --- wontfix
firefox-esr52 --- fixed
firefox53 --- fixed
firefox54 --- fixed
firefox55 --- fixed

People

(Reporter: gkw, Assigned: till)

References

Details

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

Attachments

(2 files)

Detailed Crash Information
30.89 KB, text/plain
Details
Properly handle OOM during exception throwing in all Promise code
5.10 KB, patch
arai
: review+
jcristau
: approval-mozilla-aurora+
lizzard
: approval-mozilla-beta+
jcristau
: approval-mozilla-esr52+
Details | Diff | Splinter Review 
The following testcase crashes on mozilla-central revision 0a7831d838f7 (build with --enable-debug --enable-more-deterministic, run with --fuzzing-safe --no-threads --baseline-eager --no-ion):

try {
    // Adapted from js/src/tests/test262/built-ins/Promise/race/resolve-poisoned-then.js
    var x = Object.defineProperty({}, 'then', {
        get: function() {
            throw 0;
        }
    });
    var y = {
        then: function(m) {
            m(x);
        }
    };
    Promise.race([y]);
    // Adapted from js/src/jit-test/tests/debug/bug1106719.js
    g = newGlobal();
    g.parent = this;
    g.eval("Debugger(parent).onExceptionUnwind = function() {};");
    gcparam("maxBytes", gcparam("gcBytes"));
    function f() {
        f()(arguments);
    }
    f();
} catch (e) {}



Backtrace:

0   js-dbg-64-dm-clang-darwin-0a7831d838f7	0x00000001021f6b41 JSContext::getPendingException(JS::MutableHandle<JS::Value>) + 337 (jscntxt.cpp:1092)
1   js-dbg-64-dm-clang-darwin-0a7831d838f7	0x0000000101d767ff js::GetAndClearException(JSContext*, JS::MutableHandle<JS::Value>) + 15 (Interpreter.cpp:4483)
2   js-dbg-64-dm-clang-darwin-0a7831d838f7	0x0000000101dd8917 ResolvePromiseInternal(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>) + 1527 (Promise.cpp:367)
3   js-dbg-64-dm-clang-darwin-0a7831d838f7	0x0000000101e25d1a ResolvePromiseFunction(JSContext*, unsigned int, JS::Value*) + 602 (Promise.cpp:424)
4   js-dbg-64-dm-clang-darwin-0a7831d838f7	0x0000000101d718c9 js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) + 201 (jscntxtinlines.h:282)
/snip

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/0ab4d5b055e4
user:        Till Schneidereit
date:        Fri Aug 05 18:40:46 2016 +0200
summary:     Bug 1289318 - Part 5: Port most Promise functions directly involved in Promise resolution from JS to C++. r=efaust

Till, is bug 1289318 a likely regressor?
Blocks: 1289318
Flags: needinfo?(till)
This came to light now only because of the recent test262 testcase landings.
Not all Promise code properly handled OOM during exception throwing. This patch introduces a wrapper for GetAndClearException that does properly handle this situation and changes all Promise code to use it instead of GetAndClearException.
Assignee: nobody → till
Status: NEW → ASSIGNED
Flags: needinfo?(till)
Attachment #8841975 - Flags: review?(arai.unmht)
Comment on attachment 8841975 [details] [diff] [review]
Properly handle OOM during exception throwing in all Promise code

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

r+ with "print(e)" removed :)
Attachment #8841975 - Flags: review?(arai.unmht) → review+
Pushed by tschneidereit@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/76bf73578801
Properly handle OOM during exception throwing in all Promise code. r=arai
wow, the testcase needs "// |jit-test| error: out of memory" line
Pushed by tschneidereit@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cfd2fd77ff04
Properly handle OOM during exception throwing in all Promise code. r=arai
Backed out for failing handling-oom-during-exception-throwing.js:

https://hg.mozilla.org/integration/mozilla-inbound/rev/4dbbd735c27d7a4a2968c819c5022d31e71b2dc8

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=cfd2fd77ff046cfbe2d1693858f093df9af41d74&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=81450972&repo=mozilla-inbound

[task 2017-03-03T14:19:56.281386Z] TEST-PASS | js/src/jit-test/tests/promise/bug-1298776.js | Success (code 0, args "--ion-eager --ion-offthread-compile=off")
[task 2017-03-03T14:19:56.336087Z] TEST-PASS | js/src/jit-test/tests/profiler/test-baseline-eval-frame-profiling.js | Success (code 0, args "--ion-eager --ion-offthread-compile=off --non-writable-jitcode --ion-check-range-analysis --ion-extra-checks --no-sse3 --no-threads")
[task 2017-03-03T14:19:56.338866Z] TEST-PASS | js/src/jit-test/tests/promise/getwaitforallpromise-error-handling.js | Success (code 0, args "--baseline-eager")
[task 2017-03-03T14:19:56.363757Z] TEST-PASS | js/src/jit-test/tests/profiler/test-bug1026485.js | Success (code 0, args "--no-baseline --no-ion")
[task 2017-03-03T14:19:56.373028Z] TEST-PASS | js/src/jit-test/tests/promise/getwaitforallpromise-error-handling.js | Success (code 0, args "--no-baseline --no-ion")
[task 2017-03-03T14:19:56.395467Z] Exit code: 0
[task 2017-03-03T14:19:56.395567Z] FAIL - promise/handling-oom-during-exception-throwing.js
[task 2017-03-03T14:19:56.395723Z] TEST-UNEXPECTED-FAIL | js/src/jit-test/tests/promise/handling-oom-during-exception-throwing.js | Unknown (code 0, args "")
[task 2017-03-03T14:19:56.395778Z] INFO exit-status     : 0
[task 2017-03-03T14:19:56.395831Z] INFO timed-out       : False
[task 2017-03-03T14:19:56.400600Z] TEST-PASS | js/src/jit-test/tests/promise/getwaitforallpromise-error-handling.js | Success (code 0, args "--ion-eager --ion-offthread-compile=off")
[task 2017-03-03T14:19:56.403690Z] TEST-PASS | js/src/jit-test/tests/promise/bug-1298776.js | Success (code 0, args "--baseline-eager")
[task 2017-03-03T14:19:56.409403Z] Exit code: 0
[task 2017-03-03T14:19:56.409497Z] FAIL - promise/handling-oom-during-exception-throwing.js
[task 2017-03-03T14:19:56.409622Z] TEST-UNEXPECTED-FAIL | js/src/jit-test/tests/promise/handling-oom-during-exception-throwing.js | Unknown (code 0, args "--baseline-eager")
[task 2017-03-03T14:19:56.409675Z] INFO exit-status     : 0
[task 2017-03-03T14:19:56.409728Z] INFO timed-out       : False
[task 2017-03-03T14:19:56.418459Z] TEST-PASS | js/src/jit-test/tests/profiler/test-baseline-eval-frame-profiling.js | Success (code 0, args "--baseline-eager")
[task 2017-03-03T14:19:56.442556Z] Exit code: 0
[task 2017-03-03T14:19:56.442666Z] FAIL - promise/handling-oom-during-exception-throwing.js
[task 2017-03-03T14:19:56.442765Z] TEST-UNEXPECTED-FAIL | js/src/jit-test/tests/promise/handling-oom-during-exception-throwing.js | Unknown (code 0, args "--ion-eager --ion-offthread-compile=off --non-writable-jitcode --ion-check-range-analysis --ion-extra-checks --no-sse3 --no-threads")
[task 2017-03-03T14:19:56.442794Z] INFO exit-status     : 0
[task 2017-03-03T14:19:56.442841Z] INFO timed-out       : False
[task 2017-03-03T14:19:56.445693Z] TEST-PASS | js/src/jit-test/tests/promise/no-reentrant-drainjobqueue.js | Success (code 0, args "")
[task 2017-03-03T14:19:56.459786Z] TEST-PASS | js/src/jit-test/tests/promise/getwaitforallpromise-error-handling.js | Success (code 0, args "--ion-eager --ion-offthread-compile=off --non-writable-jitcode --ion-check-range-analysis --ion-extra-checks --no-sse3 --no-threads")
[task 2017-03-03T14:19:56.459866Z] Exit code: 0
[task 2017-03-03T14:19:56.459906Z] FAIL - promise/handling-oom-during-exception-throwing.js
[task 2017-03-03T14:19:56.459970Z] TEST-UNEXPECTED-FAIL | js/src/jit-test/tests/promise/handling-oom-during-exception-throwing.js | Unknown (code 0, args "--no-baseline --no-ion")
[task 2017-03-03T14:19:56.460001Z] INFO exit-status     : 0
[task 2017-03-03T14:19:56.460026Z] INFO timed-out       : False
[task 2017-03-03T14:19:56.465054Z] Exit code: 0
[task 2017-03-03T14:19:56.465116Z] FAIL - promise/handling-oom-during-exception-throwing.js
[task 2017-03-03T14:19:56.465184Z] TEST-UNEXPECTED-FAIL | js/src/jit-test/tests/promise/handling-oom-during-exception-throwing.js | Unknown (code 0, args "--ion-eager --ion-offthread-compile=off")
[task 2017-03-03T14:19:56.465217Z] INFO exit-status     : 0
[task 2017-03-03T14:19:56.465247Z] INFO timed-out       : False
Pushed by tschneidereit@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/caa3df341ea8
Properly handle OOM during exception throwing in all Promise code. r=arai
I ended up pushing this without the test after trying way too long to make it pass (or, in this case, fail) reliably.

I did find ways to trigger an oom in the right place in most cases, but not in all. A significant problem is that the oom is supposed to occur in the thenable's then method, but that's called directly from C++ and then promptly swallowed. I *think* the test that I originally checked in, and the code from comment 0, sort of accidentally worked in that the recursive calls in function f triggered an oom, not the promise code, but we handled that correctly. Removing the `function f` stuff makes the entire test case behave very differently.

In any case, I'm out of ideas here, but think that the missing test shouldn't keep us from landing this bug fix. arai, if you have an idea how to reliably test this, that'd be great. If not, I propose we just move on.
Flags: needinfo?(till) → needinfo?(arai.unmht)
can we just enclose the entire test code (including drainJobQueue if needed) with try-catch, and check if it doesn't crash?
(maybe need to reset gcParam before leaving the try-catch)
Flags: needinfo?(arai.unmht) → needinfo?(till)
https://hg.mozilla.org/mozilla-central/rev/caa3df341ea8
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Seems like this might be worth backporting to Aurora/Beta/ESR52?
status-firefox52: --- → wontfix
status-firefox53: --- → affected
status-firefox-esr52: --- → affected
Till, WDYT about uplifting this to the appropriate branches from comment 15?
(In reply to Tooru Fujisawa [:arai] (almost away until April) from comment #13)
> can we just enclose the entire test code (including drainJobQueue if needed)
> with try-catch, and check if it doesn't crash?
> (maybe need to reset gcParam before leaving the try-catch)

I tried that, but it didn't reliably trigger the OOM at the right place. It's quite possible that I overlooked something, but I tried all variations of this and similar setups that I could come up with, to no avail. I don't want to spend more time on building a reliable test for this, so I think we'll just let it be.
Flags: needinfo?(till)
Comment on attachment 8841975 [details] [diff] [review]
Properly handle OOM during exception throwing in all Promise code

[Approval Request Comment]
[Feature/Bug causing the regression]: bug 1313049
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: more crashes
Fix Landed on Version: 55
Risk to taking this patch (and alternatives if risky): very low, adds OOM handling in some places in Promise code, nothing else
String or UUID changes made by this patch: n/a

[Is this code covered by automated tests?]: no, because tests proved too flaky across platforms.
[Has the fix been verified in Nightly?]: yes, the test case from comment 0 and similar ones done manually don't reproduce anymore
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: it adds OOM handling without other changes
Attachment #8841975 - Flags: approval-mozilla-esr52?
Attachment #8841975 - Flags: approval-mozilla-beta?
Attachment #8841975 - Flags: approval-mozilla-aurora?
Comment on attachment 8841975 [details] [diff] [review]
Properly handle OOM during exception throwing in all Promise code

add OOM handling, aurora54+
Attachment #8841975 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8841975 [details] [diff] [review]
Properly handle OOM during exception throwing in all Promise code

Crash fix, let's take this for beta.
Attachment #8841975 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8841975 [details] [diff] [review]
Properly handle OOM during exception throwing in all Promise code

crash fix for esr52
Attachment #8841975 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
You need to log in before you can comment on or make changes to this bug.