Crash in AstDecodeExpr

RESOLVED FIXED in Firefox 52

Status

()

P1
critical
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: sharmalay, Assigned: bbouvier)

Tracking

(Blocks: 1 bug)

52 Branch
mozilla53
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 unaffected, firefox52+ fixed, firefox53 fixed)

Details

(crash signature)

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

2 years ago
Created attachment 8822726 [details]
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.

Updated

2 years ago
Blocks: 1188259
(Reporter)

Comment 1

2 years ago
Created attachment 8822732 [details]
test.wasm

The corresponding wasm file generated with the above test case.
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.
Component: Developer Tools → JavaScript Engine: JIT
Product: Firefox → Core
(Assignee)

Comment 3

2 years ago
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.
Flags: needinfo?(bbouvier)
(Assignee)

Updated

2 years ago
Flags: needinfo?(bbouvier)

Comment 4

2 years ago
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.
(Assignee)

Comment 5

2 years ago
Created attachment 8823633 [details] [diff] [review]
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
Assignee: nobody → bbouvier
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #8823633 - Flags: review?(luke)
Attachment #8823633 - Flags: approval-mozilla-aurora?
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
(Assignee)

Comment 7

2 years ago
Thanks, fixed locally.

Comment 8

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

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

Thanks!
Attachment #8823633 - Flags: review?(luke) → review+
(Assignee)

Comment 9

2 years ago
Created attachment 8823745 [details] [diff] [review]
no-source.patch

Approval Request Comment: see previous comments.

Updated test comment; carrying forward r+.
Attachment #8823633 - Attachment is obsolete: true
Attachment #8823633 - Flags: approval-mozilla-aurora?
Attachment #8823745 - Flags: review+
Attachment #8823745 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 10

2 years ago
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.
(Assignee)

Updated

2 years ago
status-firefox52: --- → affected
tracking-firefox52: --- → ?
Priority: -- → P1
Comment on attachment 8823745 [details] [diff] [review]
no-source.patch

avoid crash in devtools with wasm in aurora52
Attachment #8823745 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
tracking-firefox52: ? → +
Blocks: 1317319
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → unaffected
status-firefox52: affected → fixed
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.