1464751 - If there is no JIT, there should be no JIT signal handlers

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement, P5)

Description

[Cameron Kaiser [:spectre]]

On platforms where JS_CODEGEN_NONE is in effect, such as the ppc64le Talos II I'm typing this on, the browser immediately asserts if any signal occurs (usually shortly after startup) in js/src/wasm/WasmSignalHandlers.cpp. So, we shouldn't be trying to install signal handlers on those platforms.

Comment 1

[Cameron Kaiser [:spectre]]

Attachment: Don't install wasm signal handlers on platforms without a JIT

Pretty self-explanatory.

Lars, please advise if you are not an appropriate reviewer.

Comment 2

[Lars T Hansen [:lth]]

I guess the patch is probably fine but what branch is this for?  It doesn't seem related to current mozilla-central.

Comment 3

[Cameron Kaiser [:spectre]]

Sorry, that was against mozilla-release. Regenerating against mozilla-central. Apologies.

Comment 4

[Cameron Kaiser [:spectre]]

Attachment: Don't install wasm signal handlers on platforms without a JIT

Rebased to tip.

Comment 5

[Lars T Hansen [:lth]]

Comment on attachment 8981217 [details] [diff] [review]
Don't install wasm signal handlers on platforms without a JIT

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

::: js/src/wasm/WasmSignalHandlers.cpp
@@ +1393,5 @@
>          return sHaveSignalHandlers;
>      sTriedInstallSignalHandlers = true;
>  
> +#if defined (JS_CODEGEN_NONE)
> +    // If there is no JIT, then there should be no signal handlers.

If it were me I would write "no Wasm signal handlers", since that's what we're trying to install here.

Comment 6

[Cameron Kaiser [:spectre]]

Attachment: Don't install wasm signal handlers on platforms without a JIT v2

Comment changed as requested, carrying forward r+, check-in requested. Thanks!

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9e9764dc517
Don't install wasm signal handlers on platforms without a JIT. r=lth

Comment 8

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/f9e9764dc517

Comment 9

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8981240 [details] [diff] [review]
Don't install wasm signal handlers on platforms without a JIT v2

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: crash at start on platforms with no JITs. So it'd be nice for tier 3 platforms. Somebody asked for this on irc, and I figured I'd ask, if this can ride along another ESR build.
Fix Landed on Version: 62
Risk to taking this patch (and alternatives if risky): very very very very low
String or UUID changes made by this patch: n/a

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8981240 [details] [diff] [review]
Don't install wasm signal handlers on platforms without a JIT v2

Fixes a startup crash for Tier 3 platforms without a JIT. Approved for ESR 60.3.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/d5d00faf0465