Likely asm.js/Ion crash on some android devices

RESOLVED FIXED in mozilla27

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: glandium, Assigned: luke)

Tracking

Trunk
mozilla27
ARM
Android
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

5 years ago
I found in bug 907957 that on some android devices, a SIGSEGV signal handler doesn't get correct values of siginfo_t.si_addr (it's always NULL). Currently, these devices would fail to load firefox at all, but once the workaround in bug 907957 is confirmed to work and lands, firefox will load, and that will undoubtedly trigger the same problem on the signal handler asm.js uses (well, provided asm.js is used).
(Assignee)

Comment 1

5 years ago
Thanks for thinking of us.  Even worse: with bug 864220, the issue would affect all Ion code.

If there are devices that lie to their signal handlers then it seems like we need to blacklist them in EnsureAsmJSSignalHandlersInstalled.  Hopefully this is a very narrow set of devices?

Note: we already have ComputeIsJITBroken() so hopefully the blacklisting logic could be in a function ComputeSignalHandlersAreBroken() next to ComputeIsJITBroken and share some of the querying logic.
Summary: Likely asm.js crash on some android devices → Likely asm.js/Ion crash on some android devices
(Reporter)

Comment 2

5 years ago
(In reply to Luke Wagner [:luke] from comment #1)
> Thanks for thinking of us.  Even worse: with bug 864220, the issue would
> affect all Ion code.
> 
> If there are devices that lie to their signal handlers then it seems like we
> need to blacklist them in EnsureAsmJSSignalHandlersInstalled.  Hopefully
> this is a very narrow set of devices?

No idea, unfortunately :(

> Note: we already have ComputeIsJITBroken() so hopefully the blacklisting
> logic could be in a function ComputeSignalHandlersAreBroken() next to
> ComputeIsJITBroken and share some of the querying logic.

Indeed. What I'm doing in bug 907957 is to do a quick test with a temporary signal handler, trigger a segfault manually, and check if the received si_addr matches what I'm expecting.
(Assignee)

Comment 3

5 years ago
(In reply to Mike Hommey [:glandium] from comment #2)
Hmm, perhaps you could put that into a reuable mfbt IsSignalHandlingBroken helper function that does the test once and caches the result in some static variable protected by a lock?  That'd be pretty nice.
(Reporter)

Comment 4

5 years ago
I added the following function in the patch for bug 907957:

extern "C" MFBT_API bool IsSignalHandlingBroken();

It's not in mfbt, but it's exposed from libmozglue, against which everything should be linked on android. I think it's reasonable to use it from there, until we find non android devices that have the same problem, at which point we can reassess.
You'll need a local declaration in js code, ifdefed on ANDROID.
(Assignee)

Updated

5 years ago
Depends on: 907957
(Assignee)

Comment 5

5 years ago
Created attachment 799539 [details] [diff] [review]
tidy up indentation

First tidy up #if indentation which seems to be off by one.
Assignee: general → luke
Status: NEW → ASSIGNED
Attachment #799539 - Flags: review?(mh+mozilla)
(Assignee)

Comment 6

5 years ago
Created attachment 799542 [details] [diff] [review]
check IsSignalHandlingBroken

I tested locally on a Nexus 4 (where signal handling isn't broken).  If you have any phones where it is broken, I'd appreciate if you could test this patch (you should see a "asm.js type error: Platform missing signal handler support" warning on the console if you evaluate "(function() { "use asm"; return {}})").
Attachment #799542 - Flags: review?(mh+mozilla)
(Assignee)

Comment 7

5 years ago
Mike, do you know what I should do about these b2g emulator failures:
  https://tbpl.mozilla.org/?tree=Try&rev=c72f59941ba4
? Should I only call the IsSignalHandlingBroken if defined(__arm__)?
(Reporter)

Updated

5 years ago
Attachment #799539 - Flags: review?(mh+mozilla) → review+
(Reporter)

Comment 8

5 years ago
(In reply to Luke Wagner [:luke] from comment #6)
> Created attachment 799542 [details] [diff] [review]
> check IsSignalHandlingBroken

The patch is empty.
(Reporter)

Comment 9

5 years ago
(In reply to Luke Wagner [:luke] from comment #7)
> Mike, do you know what I should do about these b2g emulator failures:
>   https://tbpl.mozilla.org/?tree=Try&rev=c72f59941ba4
> ? Should I only call the IsSignalHandlingBroken if defined(__arm__)?

Ah, b2g defines ANDROID. I guess the best thing to do is to inherit MOZ_LINKER from the toplevel configure (just export it before the js configure invocation), and use that (AC_SUBST(MOZ_LINKER) in js/src/configure.in ; ifdef MOZ_LINKER; DEFINES += -DMOZ_LINKER; endif in js/src/Makefile.in).
(Reporter)

Updated

5 years ago
Attachment #799542 - Flags: review?(mh+mozilla)
(Assignee)

Comment 10

5 years ago
Created attachment 799835 [details] [diff] [review]
check IsSignalHandlingBroken

Oops, sorry.  qref'd this time.
Attachment #799542 - Attachment is obsolete: true
Attachment #799835 - Flags: review?(mh+mozilla)
(Reporter)

Comment 11

5 years ago
Comment on attachment 799835 [details] [diff] [review]
check IsSignalHandlingBroken

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

I'll look at the fixed version (per comment 9)
Attachment #799835 - Flags: review?(mh+mozilla)
(Assignee)

Comment 12

5 years ago
(In reply to Mike Hommey [:glandium] from comment #11)
Oh, I missed comment 9, thanks!  I don't really understand what several of those things you said mean, do you suppose you could write just that in a patch that I could fold in?
(Reporter)

Comment 13

5 years ago
yes
(Assignee)

Comment 14

5 years ago
(In reply to Mike Hommey [:glandium] from comment #13)
Thanks!  Just a reminder: the first FFOS branch bearing this signal handling code is happening soon.
(Assignee)

Comment 15

5 years ago
Trying again with thanks to Mike for build assistance:
https://tbpl.mozilla.org/?tree=Try&rev=d9ce2a701bdf
(Assignee)

Comment 16

5 years ago
Created attachment 804065 [details] [diff] [review]
check IsSignalHandlingBroken

D'oh.  Cancelling previous try push as I didn't actually #ifdef MOZ_LINKER; try again:
https://tbpl.mozilla.org/?tree=Try&rev=68408f75bd81
Attachment #799835 - Attachment is obsolete: true
Attachment #804065 - Flags: review?(mh+mozilla)
(Assignee)

Comment 17

5 years ago
Green on try (the X orange is the fault of the parent cset and was backed out on inbound).
(Reporter)

Updated

5 years ago
Attachment #804065 - Flags: review?(mh+mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/5b52918bf6f6
https://hg.mozilla.org/mozilla-central/rev/4e3b6957add0
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.