Likely asm.js/Ion crash on some android devices

RESOLVED FIXED in mozilla27

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: glandium, Assigned: luke)

Tracking

Trunk
mozilla27
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

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).
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
(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.
(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.
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.
Depends on: 907957
First tidy up #if indentation which seems to be off by one.
Assignee: general → luke
Status: NEW → ASSIGNED
Attachment #799539 - Flags: review?(mh+mozilla)
Posted patch check IsSignalHandlingBroken (obsolete) — Splinter Review
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)
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__)?
Attachment #799539 - Flags: review?(mh+mozilla) → review+
(In reply to Luke Wagner [:luke] from comment #6)
> Created attachment 799542 [details] [diff] [review]
> check IsSignalHandlingBroken

The patch is empty.
(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).
Attachment #799542 - Flags: review?(mh+mozilla)
Posted patch check IsSignalHandlingBroken (obsolete) — Splinter Review
Oops, sorry.  qref'd this time.
Attachment #799542 - Attachment is obsolete: true
Attachment #799835 - Flags: review?(mh+mozilla)
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)
(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?
yes
(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.
Trying again with thanks to Mike for build assistance:
https://tbpl.mozilla.org/?tree=Try&rev=d9ce2a701bdf
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)
Green on try (the X orange is the fault of the parent cset and was backed out on inbound).
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
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.