1326452 - Crash in AstDecodeExpr

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P1)

Description

[chase]

Attachment: test.c: A simple C program to compile to wasm

This bug was filed from the Socorro interface and is 
report bp-c0bb0466-b22a-4854-8cf3-d2c8d2161230.
=============================================================

Reloading a web page containing wasm with Developer Tools open causes Firefox to crash. Also switching to the Debugger tab of Developer Tools causes a crash.

Steps to reproduce:
1) Ensure config option `javascript.options.wasm` is set to true.
2) Compile the attached C source to a HTML file using the emcc command `emcc -s WASM=1 -s "BINARYEN_METHOD='native-wasm,asmjs,interpret-binary'" -o test.html test.c`.
3) Open the resulting HTML file in Firefox over HTTP.
4) Open Developer Tools.
5) Reload page, or switch to Debugger tab.

The past few updates of Firefox Developer Edition (and possibly before, cannot confirm) have been affected by this. Using emcc version 1.37.0. The wasm demo on webassembly.org does not experience this problem. As per the crash report, the offending module is libxul.so.

Comment 1

[chase]

Attachment: test.wasm

The corresponding wasm file generated with the above test case.

Comment 2

[Dan Gohman [:sunfish]]

In a debug build, this appears to manifest as:

Assertion failure: kind_ == DefinitionKind::Function, at .../src/js/src/wasm/WasmAST.h:668

I see this in Aurora but not in Nightly. Binary search points to this:

https://hg.mozilla.org/integration/mozilla-inbound/rev/bcaa68d34a94

as the patch where the assertion failure appears to be fixed.

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Thanks for opening the report, and thanks for letting me know about this. I think this has been fixed by/into a massive refactoring of the sections decoding in general. We've decided *not* to uplift the patches because they were big and risky, but now I do realize that this means that opening the devtools may indeed crash the entire browser. Only 52 is affected, on which we probably need to mitigate the issue by just not displaying anything; or, we could just uplift it all. I'll investigate mitigating on Aurora.

Comment 4

[Luke Wagner [:luke]]

Since debugging doesn't work at all in 52, I think it makes sense to just remove the BinaryToText() call from Code::createText(), keeping just the experimental message, updated appropriately.

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: no-source.patch

Approval Request Comment
[Feature/Bug causing the regression]: risky patches not uplifted to aurora
[User impact if declined]: crashes when running WebAssembly programs with devtools open
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: not a fix for nightly; just a feature to disable in aurora; manual shell testing passed; manual browser testing will be done once the try build finishes.
[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 is temporarily disabling a feature (namely, wasm text format generation in the debugger devtools), so it is very conservative.
[String changes made/needed]: n/a

Try build to test the results in a browser: https://treeherder.mozilla.org/#/jobs?repo=try&revision=57d6de37204868438bc2f5d76b3558f39c5c9f8a

Comment 6

[Julien Cristau [:jcristau]]

Comment on attachment 8823633 [details] [diff] [review]
no-source.patch

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

typo in test comment

::: js/src/jit-test/tests/debug/wasm-05.js
@@ +2,5 @@
>  // when source text is generated.
>  
>  load(libdir + "asserts.js");
>  
> +// Disabled in aurora (see also bug 1326462).

bug number is 1326452

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

Thanks, fixed locally.

Comment 8

[Luke Wagner [:luke]]

Comment on attachment 8823633 [details] [diff] [review]
no-source.patch

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

Thanks!

Comment 9

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: no-source.patch

Approval Request Comment: see previous comments.

Updated test comment; carrying forward r+.

Comment 10

[Benjamin Bouvier [:bbouvier] (inactive)]

Fwiw, I've manually checked that the expected message showed up in the devtools console in aurora, and it does. This will be confusing for users of aurora52 because the message tells to download aurora, but it will be correct for when 52 is beta.

Comment 11

[Julien Cristau [:jcristau]]

Comment on attachment 8823745 [details] [diff] [review]
no-source.patch

avoid crash in devtools with wasm in aurora52

Comment 12

[Justin Wood (:Callek)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/68d35cdae645