909709 - Likely asm.js/Ion crash on some android devices

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Mike Hommey [:glandium]]

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).

Comment 1

[Luke Wagner [:luke]]

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.

Comment 2

[Mike Hommey [:glandium]]

(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.

Comment 3

[Luke Wagner [:luke]]

(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.

Comment 4

[Mike Hommey [:glandium]]

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.

Comment 5

[Luke Wagner [:luke]]

Attachment: tidy up indentation

First tidy up #if indentation which seems to be off by one.

Comment 6

[Luke Wagner [:luke]]

Attachment: 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 {}})").

Comment 7

[Luke Wagner [:luke]]

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__)?

Comment 8

[Mike Hommey [:glandium]]

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

The patch is empty.

Comment 9

[Mike Hommey [:glandium]]

(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).

Comment 10

[Luke Wagner [:luke]]

Attachment: check IsSignalHandlingBroken

Oops, sorry.  qref'd this time.

Comment 11

[Mike Hommey [:glandium]]

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)

Comment 12

[Luke Wagner [:luke]]

(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?

Comment 13

[Mike Hommey [:glandium]]

yes

Comment 14

[Luke Wagner [:luke]]

(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.

Comment 15

[Luke Wagner [:luke]]

Trying again with thanks to Mike for build assistance:
https://tbpl.mozilla.org/?tree=Try&rev=d9ce2a701bdf

Comment 16

[Luke Wagner [:luke]]

Attachment: 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

Comment 17

[Luke Wagner [:luke]]

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

Comment 18

[Luke Wagner [:luke]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5b52918bf6f6
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e3b6957add0

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5b52918bf6f6
https://hg.mozilla.org/mozilla-central/rev/4e3b6957add0