Closed
Bug 909709
Opened 11 years ago
Closed 11 years ago
Likely asm.js/Ion crash on some android devices
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla27
People
(Reporter: glandium, Assigned: luke)
References
Details
Attachments
(2 files, 2 obsolete files)
11.91 KB,
patch
|
glandium
:
review+
|
Details | Diff | Splinter Review |
5.53 KB,
patch
|
glandium
:
review+
|
Details | Diff | Splinter Review |
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•11 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•11 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•11 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•11 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 | |
Comment 5•11 years ago
|
||
First tidy up #if indentation which seems to be off by one.
![]() |
Assignee | |
Comment 6•11 years ago
|
||
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•11 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•11 years ago
|
Attachment #799539 -
Flags: review?(mh+mozilla) → review+
Reporter | ||
Comment 8•11 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•11 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•11 years ago
|
Attachment #799542 -
Flags: review?(mh+mozilla)
![]() |
Assignee | |
Comment 10•11 years ago
|
||
Oops, sorry. qref'd this time.
Attachment #799542 -
Attachment is obsolete: true
Attachment #799835 -
Flags: review?(mh+mozilla)
Reporter | ||
Comment 11•11 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•11 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•11 years ago
|
||
yes
![]() |
Assignee | |
Comment 14•11 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•11 years ago
|
||
Trying again with thanks to Mike for build assistance:
https://tbpl.mozilla.org/?tree=Try&rev=d9ce2a701bdf
![]() |
Assignee | |
Comment 16•11 years ago
|
||
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•11 years ago
|
||
Green on try (the X orange is the fault of the parent cset and was backed out on inbound).
Reporter | ||
Updated•11 years ago
|
Attachment #804065 -
Flags: review?(mh+mozilla) → review+
![]() |
Assignee | |
Comment 18•11 years ago
|
||
Comment 19•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/5b52918bf6f6
https://hg.mozilla.org/mozilla-central/rev/4e3b6957add0
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in
before you can comment on or make changes to this bug.
Description
•