1757733 - Crash in [@ MozCrashWarningReporter] from wasm

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, defect, P2)

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

11 crashes from 2 installations (1x macOS 12, 1x KDE), Firefox 99.0a1 20220301215224 + 20220301094029. This also has 70 crash reports for Firefox 97.0.1 compared to <= 5 crashes for other versions, and these crashes have a different wasm stack.

Crash report: https://crash-stats.mozilla.org/report/index/2774e1b3-f29a-47ae-851b-a7b5a0220302

MOZ_CRASH Reason: MOZ_CRASH(Why is someone touching JSAPI without an AutoJSAPI?)

Top 10 frames of crashing thread:

0 XUL MozCrashWarningReporter xpcom/base/CycleCollectedJSRuntime.cpp:472
1 XUL js::ReportErrorNumberVA js/src/vm/ErrorReporting.cpp:478
2 XUL js::WarnNumberASCII js/src/vm/Warnings.cpp:74
3 XUL js::wasm::Log js/src/wasm/WasmLog.cpp:43
4 XUL js::wasm::CompileArgs::build js/src/wasm/WasmCompile.cpp:159
5 XUL ModuleValidator<mozilla::Utf8Unit>::finish js/src/wasm/AsmJS.cpp:2157
6 XUL js::CompileAsmJS js/src/wasm/AsmJS.cpp:7179
7 XUL js::frontend::Parser<js::frontend::FullParseHandler, mozilla::Utf8Unit>::asmJS js/src/frontend/Parser.cpp:3890
8 XUL js::frontend::GeneralParser<js::frontend::FullParseHandler, mozilla::Utf8Unit>::statementList js/src/frontend/Parser.cpp:4097
9 XUL js::frontend::GeneralParser<js::frontend::FullParseHandler, mozilla::Utf8Unit>::innerFunctionForFunctionBox js/src/frontend/Parser.cpp:3300

Comment 1

[Ryan Hunt [:rhunt]]

This crash is being hit because we're reporting a warning on a JSContext that is being used off the main thread (which implies it's potentially outside of an AutoJSAPI). This warning should only happen if wasmVerbose is enabled (which it is not by default), but there also appears to be code for reporting an error in the same function (CompileArgs::build) that I wonder if it would experience a similar issue.

This is in AsmJS code, and it looks like it's the only place we use a JSContext off of the main thread in wasm code. I think the proper thing to do would be to rewrite AsmJS to not report errors/warnings here.

Comment 2

[Lars T Hansen [:lth]]

P2 since I suspect this triggers only with non-default options. It would be nice to get this fixed for FF99, so uplift if landing after the train leaves?

Comment 4

[Ryan Hunt [:rhunt]]

Attachment: Bug 1757733 - wasm: Don't report warnings from AsmJS compilation, which may be off-main-thread. r?yury

AsmJS compilation may be off the main thread, so we cannot report warnings
to JSContext. wasm::Log may do this if the right pref is on. CompileArgs::
build() uses wasm::Log. AsmJS uses CompileArgs::build(). This commit adds
a separate version of CompileArgs::build() which will not log or report
errors. AsmJS then asserts that only an OOM may be possible here, as we
should ensure a wasm compiler is available before compiling.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/472418229a51
wasm: Don't report warnings from AsmJS compilation, which may be off-main-thread. r=yury

Comment 6

[Marian-Vasile Laza]

Backed out changeset 472418229a51 (Bug 1757733) for causing bustages on testWasm.cpp.
Backout link
Push with failures
Failure Log

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/51d54dda3ba1
wasm: Don't report warnings from AsmJS compilation, which may be off-main-thread. r=yury

Comment 8

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/51d54dda3ba1

Comment 9

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:rhunt, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 10

[Ryan Hunt [:rhunt]]

Comment on attachment 9267707 [details]
Bug 1757733 - wasm: Don't report warnings from AsmJS compilation, which may be off-main-thread. r?yury

Beta/Release Uplift Approval Request

• User impact if declined: A user with javascript.options.wasm_verbose pref enabled (this is not enabled by default) may experience a crash when asm.js code is compiled off of the main thread.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This patch changes the asm.js compiler to not log the active compilers.
• String changes made/needed:

Comment 11

[Donal Meehan [:dmeehan]]

Comment on attachment 9267707 [details]
Bug 1757733 - wasm: Don't report warnings from AsmJS compilation, which may be off-main-thread. r?yury

Approved for 99.0b7. Thanks.

Comment 12

[Donal Meehan [:dmeehan]]

https://hg.mozilla.org/releases/mozilla-beta/rev/19f3321b12c1