Closed Bug 1305197 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox51 --- fixed
firefox52 --- fixed

People

(Reporter: gkw, Assigned: bbouvier)

References

(Blocks 2 open bugs)

Details

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

Attachments

(3 files, 1 obsolete file)

The following testcase crashes on mozilla-central revision f0e6cc636021 (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/basic/undefined-warning-bug1274499.js
options("werror");
// Adapted from randomly chosen test: js/src/jit-test/tests/wasm/basic.js
function wasmFailValidateText(str, errorType, pattern) {
    let binary = wasmTextToBinary(str, 'new-format');
    assertEq(WebAssembly.validate(binary), false);
}
wasmFailValidateText('(module (func) (func) (export "a" 2))', TypeError, /exported function index out of bounds/);


Backtrace:

0   js-dbg-64-dm-clang-darwin-f0e6cc636021	0x000000010a7a929f js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) + 271 (jscntxtinlines.h:242)
1   js-dbg-64-dm-clang-darwin-f0e6cc636021	0x000000010a7a8ec3 js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) + 643 (Interpreter.cpp:446)
2   js-dbg-64-dm-clang-darwin-f0e6cc636021	0x000000010a7a0a54 Interpret(JSContext*, js::RunState&) + 34452 (Interpreter.cpp:2922)
3   js-dbg-64-dm-clang-darwin-f0e6cc636021	0x000000010a7981c4 js::RunScript(JSContext*, js::RunState&) + 452 (Interpreter.cpp:404)
4   js-dbg-64-dm-clang-darwin-f0e6cc636021	0x000000010a7aa48f js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::AbstractFramePtr, JS::Value*) + 511 (Interpreter.cpp:685)
/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/5ad42aa79b3d
user:        Benjamin Bouvier
date:        Wed Sep 07 17:52:31 2016 +0200
summary:     Bug 1292723: Implement a basic WebAssembly.validate; r=luke

Benjamin, is bug 1292723 a likely regressor?
Blocks: 1292723
Flags: needinfo?(bbouvier)
Yes, but I think it's only catchable through the report-warning-as-errors shell flag. We probably need something like bool NoExceptionPending() at the end of Webassembly.validate.
Attached patch error.patch (obsolete) — Splinter Review
(not sure the test has much worth, since it's shell only, so not adding it)
Assignee: nobody → bbouvier
Status: NEW → ASSIGNED
Flags: needinfo?(bbouvier)
Attachment #8794737 - Flags: review?(luke)
Comment on attachment 8794737 [details] [diff] [review]
error.patch

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

::: js/src/asmjs/WasmJS.cpp
@@ +1585,5 @@
>  
>      callArgs.rval().setBoolean(validated);
> +
> +    // Return isExceptionPending in case werror is active.
> +    return !cx->isExceptionPending();

Could you move the error check up to right after the report by writing "if (!JS_ReportErrorFlagsAndNumber...) return false"?  Then no comment is necessary.
Attachment #8794737 - Flags: review?(luke) → review+
Pushed by bbouvier@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb1127ffac27
Return value of WebAssembly_validate must be consistent with werror; r=luke
https://hg.mozilla.org/mozilla-central/rev/cb1127ffac27
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Benjamin, do you mind backporting this to mozilla-aurora? While wasm is still Nightly-only, the assertion is general enough that ignoring it would block finding other issues with the same assertion name, e.g. bug 1296661. Thanks!

(even the stacks are highly similar, there isn't anything wasm-specific on the stack in this bug)
Flags: needinfo?(bbouvier)
Sure, this one is easy to uplift.
Flags: needinfo?(bbouvier)
Attached patch uplift.patchSplinter Review
Approval Request Comment
[Feature/regressing bug #]: 1292723
[User impact if declined]: taking this will make the life of fuzzers easier (comment 8).
[Describe test coverage new/current, TreeHerder]: test added, green for a few days on treeherder.
[Risks and why]: no risk
[String/UUID change made/needed]: n/a
Attachment #8794737 - Attachment is obsolete: true
Attachment #8797410 - Flags: review+
Attachment #8797410 - Flags: approval-mozilla-aurora?
FWIW, the shipping target is now 52, not 51, so we probably don't need to uplift anything until Nov :)
(In reply to Luke Wagner [:luke] from comment #11)
> FWIW, the shipping target is now 52, not 51, so we probably don't need to
> uplift anything until Nov :)

Small patch easy to uplift + can make aurora fuzzing easier, so this one seems valuable to be uplifted :)
Comment on attachment 8797410 [details] [diff] [review]
uplift.patch

Take it in 51 aurora to make fuzzing easier.
Attachment #8797410 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
has problems to apply:

unable to find 'js/src/jit-test/tests/wasm/validate.js' for patching
1 out of 1 hunks FAILED -- saving rejects to file js/src/jit-test/tests/wasm/validate.js.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh bug1305197-uplift.patch
Flags: needinfo?(bbouvier)
For kicks, I tried uplifting this to Aurora on top of bug 1292723, but I ended up having to backout for failures in validate.js. Guess we'll need an updated branch patch instead :(
https://hg.mozilla.org/releases/mozilla-aurora/rev/0ac9735113c1a211afd2660a9e7066dcf33a28f3

https://treeherder.mozilla.org/logviewer.html#?job_id=3759004&repo=mozilla-aurora
(In reply to Ryan VanderMeulen [:RyanVM] from comment #16)
> For kicks, I tried uplifting this to Aurora on top of bug 1292723, but I
> ended up having to backout for failures in validate.js. Guess we'll need an
> updated branch patch instead :(
> https://hg.mozilla.org/releases/mozilla-aurora/rev/
> 0ac9735113c1a211afd2660a9e7066dcf33a28f3
> 
> https://treeherder.mozilla.org/logviewer.html#?job_id=3759004&repo=mozilla-
> aurora

Sorry for the trouble; I think one of these patches is also relying on bug 1287220, which patches we don't want to uplift. I'll make a simpler patch to uplift.
Attached patch branch.patchSplinter Review
This one applies cleanly and passes on an aurora build. When merging from inbound to aurora, the validate.js aurora file can be removed and replaced by the inbound version (added a comment to that effect in the test itself).
Flags: needinfo?(bbouvier)
You need to log in before you can comment on or make changes to this bug.