1523263 - Crash in js::jit::LIRGenerator::visitWasmStore

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, defect)

Description

[Julien Cristau [:jcristau]]

This bug is for crash report bp-53c18697-62c7-4634-b2a6-110940190128.

Top 10 frames of crashing thread:

0 libxul.so js::jit::LIRGenerator::visitWasmStore js/src/jit/arm64/Lowering-arm64.cpp:403
1 libxul.so js::jit::LIRGenerator::visitInstructionDispatch js/src/jit/Lowering.cpp:4724
2 libxul.so js::jit::LIRGenerator::visitInstruction js/src/jit/Lowering.cpp:4747
3 libxul.so js::jit::LIRGenerator::visitBlock js/src/jit/Lowering.cpp:4834
4 libxul.so js::jit::LIRGenerator::generate js/src/jit/Lowering.cpp:4908
5 libxul.so js::jit::GenerateLIR js/src/jit/Ion.cpp:1661
6 libxul.so js::wasm::IonCompileFunctions js/src/wasm/WasmIonCompile.cpp:4080
7 libxul.so ExecuteCompileTask js/src/wasm/WasmGenerator.cpp:713
8 libxul.so js::wasm::ExecuteCompileTaskFromHelperThread js/src/wasm/WasmGenerator.cpp:737
9 libxul.so js::HelperThread::handleWasmWorkload js/src/vm/HelperThreads.cpp:1956

Crashes at null on android arm64, starting with the 20190125095457 build.

Comment 1

[Lars T Hansen [:lth]]

This function should never be invoked because we are not supposed to be using Ion for Wasm on ARM64. Some predicate somewhere needs to be updated so that we continue to block Ion on ARM64.

Comment 2

[Lars T Hansen [:lth]]

I have a hunch this may be asm.js; TryInstantiate() in AsmJS.cpp guards on HasCompilerSupport() but not on IonCanCompile(). In that case we'll propagate a compilerargs object that has ion==true into the pipeline, I believe. But I've no evidence as yet.

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

This is weird indeed. Probably time to check all the options().wasmIon() calls...

(In reply to Lars T Hansen [:lth] from comment #2)

I have a hunch this may be asm.js; TryInstantiate() in AsmJS.cpp guards on HasCompilerSupport() but not on IonCanCompile(). In that case we'll propagate a compilerargs object that has ion==true into the pipeline, I believe. But I've no evidence as yet.

I don't think this is asm.js: LWasmStore is only lowered for MWasmStore which is only generated for wasm code: https://searchfox.org/mozilla-central/source/js/src/wasm/WasmIonCompile.cpp#768

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: compiler-support.patch

After the work in bug 1509441, that's the only place where we'd set compilerFlags->ion to true without checking IonCanCompile(). I don't think this fixes the issue we're observing here, because I'd expect serialization not to happen at all on ARM64 (since there's no serializing tier support in the first place, and wasm serialization has been disabled some time ago). But we can observe if it reduces the number of crashes in the wild...

Comment 5

[Lars T Hansen [:lth]]

Comment on attachment 9039548 [details] [diff] [review]
compiler-support.patch

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

OK.  Agree these are good to add regardless.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2670f29c6984
Limit wasm deserialization to available compiler backends; r=lth

Comment 7

[Luke Wagner [:luke]]

Note that bug 1521058 just added this, so if it's a recent crash, that's probably the cause.
https://bugzilla.mozilla.org/attachment.cgi?id=9037574&action=diff

My assumption I had from reading the code was that these flags simply reflect the about:config flags and are further gated by IonCanCompile()/BaselineCanCompile() so this patch wouldn't have been necessary, but maybe I missed something...

On a side note, in bug 1521058, I was also mystified as to how we're deserializing an IDB module given that IDB serialization is disabled entirely. It's quite possible (given the low volume) that someone's just running mochitests manually, one of which contains a profile containing a wasm module stored in IDB (to test this path). Otherwise, it means someone is using a profile which managed to store wasm in IDB on another architecture before bug 1469395 removed the ability to do so, which seems unlikely. Note that bug 1487479 will remove IDB databases containing wasm entirely which will remove this whole case.

Comment 8

[Benjamin Bouvier [:bbouvier] (inactive)]

My assumption I had from reading the code was that these flags simply reflect the about:config flags and are further gated by IonCanCompile()/BaselineCanCompile() so this patch wouldn't have been necessary, but maybe I missed something...

The ionEnabled/baselineEnabled flags are telling both whether there's support and the about:config flags are enabled. If ionEnabled is set to true, it's preferred as a compiler backend by default for small modules, or used in background tier2 compilation. So in the case where we have !IonCanCompile() but ionEnabled set to true, we may trigger an Ion compilation and just ignore the fact it's not supported on the current platform.

I've audited all uses of wasmIon() to make sure they were all accompanied by an IonCanCompile() call. I've looked at all the MutableCompileArgs uses, which are the only mutable ways to touch the compile args fields manually (i.e. without integrity checks added in bug 1509441).

Comment 9

[Luke Wagner [:luke]]

Ah, thanks. I see this just changed. When I landed bug 1521058 we had:

bool ionEnabled = IonCanCompile() && ((args_->ionEnabled && !gcEnabled) || !baselineEnabled);

Comment 10

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/2670f29c6984

Comment 11

[Benjamin Bouvier [:bbouvier] (inactive)]

For completeness: this patch might actually be the right fix, because Nightly Fennec users have been migrated from ARM32 to ARM64 with the same profile, so their older profile might contain arm32 ion-compiled serialized code, that the arm64 backend can't understand and will try to re-compile on deserialization, here with Ion.

From irc:

18:39:21 <jcristau> previously we only pushed the x86 and arm32 apks to google play, a couple of weeks ago we started uploading arm64 > builds for nightly, and aiui eligible devices magically get that one instead of the arm32 one now
18:39:42 <luke> jcristau: huh, interesting. i vaguely recall (from having multiple fennec installs) that each install had a separate profile dir.
18:39:50 <luke> oh, but you're saying the arm64 was adopted as an upgrade?
18:39:53 <jcristau> yes
18:40:00 <luke> gotcha, yeah, i see how this arises now

Comment 12

[Benjamin Bouvier [:bbouvier] (inactive)]

Three days without a crash in the versions of Firefox with this patch, so I guess it was actually correct.