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

RESOLVED FIXED in Firefox -esr60

Status

()

P5
normal
RESOLVED FIXED
9 months ago
5 months ago

People

(Reporter: spectre, Assigned: spectre)

Tracking

unspecified
mozilla62
Other
All
Points:
---

Firefox Tracking Flags

(firefox-esr60 fixed, firefox62 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

9 months ago
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.
(Assignee)

Comment 1

9 months ago
Created attachment 8981059 [details] [diff] [review]
Don't install wasm signal handlers on platforms without a JIT

Pretty self-explanatory.

Lars, please advise if you are not an appropriate reviewer.
Assignee: nobody → spectre
Status: NEW → ASSIGNED
Attachment #8981059 - Flags: review?(lhansen)
I guess the patch is probably fine but what branch is this for?  It doesn't seem related to current mozilla-central.
Priority: -- → P5
(Assignee)

Comment 3

9 months ago
Sorry, that was against mozilla-release. Regenerating against mozilla-central. Apologies.
(Assignee)

Comment 4

9 months ago
Created attachment 8981217 [details] [diff] [review]
Don't install wasm signal handlers on platforms without a JIT

Rebased to tip.
Attachment #8981059 - Attachment is obsolete: true
Attachment #8981059 - Flags: review?(lhansen)
Attachment #8981217 - Flags: review?(lhansen)
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.
Attachment #8981217 - Flags: review?(lhansen) → review+
(Assignee)

Comment 6

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

Comment changed as requested, carrying forward r+, check-in requested. Thanks!
Attachment #8981217 - Attachment is obsolete: true
Attachment #8981240 - Flags: review+
Attachment #8981240 - Flags: checkin?
Keywords: checkin-needed

Comment 7

9 months ago
Pushed by apavel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9e9764dc517
Don't install wasm signal handlers on platforms without a JIT. r=lth
Keywords: checkin-needed

Comment 8

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f9e9764dc517
Status: ASSIGNED → RESOLVED
Last Resolved: 9 months ago
status-firefox62: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Attachment #8981240 - Flags: checkin? → checkin+
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
Attachment #8981240 - Flags: approval-mozilla-esr60?
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.
Attachment #8981240 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+

Comment 11

5 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-esr60/rev/d5d00faf0465
status-firefox-esr60: --- → fixed
You need to log in before you can comment on or make changes to this bug.