944612 - Odinmonkey (ARM): Intermittent SIGSEGV crashes, ElfLoader sigaction wrapper drops install of asm.js handler when it gives up installing its own handler

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Douglas Crosher [:dougc]]

There are intermittent crashes running asm.js code on ARM/Android.  Bug 886736 added code to give up on installing the ElfLoader SIGSEGV handler if it detected slow handling.  The detection algorithm can be confused by system delays, is more likely to note slow handling on slower ARMv6 devices, but this is also reproducible on a Nexus 4, explaining the intermittent nature of the crashes.  The ElfLoader sigaction wrapper is always installed, and it catches the asm.js SIGSEGV handler installation, saving the handler, but when the ElfLoader handler is not also installed an asm.js related fault does not call the asm.js handler and leads to a crash.

A proof of concept workaround is to delay installation of the wrapper until the ElfLoader SIGSEGV handler has been installed.

Bug 874708 comment 12 notes that some system libraries redefine the handler within the scope of some of their code.  This might be mitigated in part by retaining the wrapper and ElfLoader handler, for the purpose of ensuring that the sigaction flags are compatible with the asm.js handler.  However in the multi-threaded code this would still be unreliable, so perhaps a list of problematic systems is needed for which asm.js compilation would be disabled.  Might the wrapper be retained and extended to report on unexpected attempts to redefine the SIGSEGV handler to help build this list?

This cause of crashes might account for some of the crashes reported in bug 867228, but only explains Android/ElfLoader specific crashes.

Comment 1

[Douglas Crosher [:dougc]]

Attachment: Avoid installation of the ElfLoader sigaction handler when it gives up installing its SIGSEGV handler

Comment 2

[Douglas Crosher [:dougc]]

Attachment: Avoid installation of the ElfLoader sigaction wrapper when it gives up installing its SIGSEGV handler

Sorry, just fixing the patch description.

Comment 3

[Douglas Crosher [:dougc]]

Comment on attachment 8340253 [details] [diff] [review]
Avoid installation of the ElfLoader sigaction wrapper when it gives up installing its SIGSEGV handler

This patch appears to also resolve bug 980498.  The problem is causing intermittent crashes.  Feedback welcomed?

Comment 4

[Mike Hommey [:glandium]]

Comment on attachment 8340253 [details] [diff] [review]
Avoid installation of the ElfLoader sigaction wrapper when it gives up installing its SIGSEGV handler

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

::: mozglue/linker/ElfLoader.cpp
@@ +847,5 @@
>    dbg->r_state = r_debug::RT_CONSISTENT;
>    dbg->r_brk();
>  }
>  
> +#if defined(ANDROID) && (defined(__i386__) || defined(__arm__))

This would break non android builds. Please leave this alone, especially the endif. It's also unrelated to the bug.

@@ +881,5 @@
>  
>  /* Replace the first instructions of the given function with a jump
>   * to the given new function. */
>  template <typename T>
> +static void

Whether diversion worked or not is still something that should be taken into account, even if it's delayed, so keep it a bool.

@@ +1026,5 @@
>    action.sa_flags = SA_SIGINFO | SA_NODEFER | SA_ONSTACK;
>    registeredHandler = !sys_sigaction(SIGSEGV, &action, nullptr);
> +
> +  if (registeredHandler)
> +    Divert(sigaction, __wrap_sigaction);

The diversion should be done before setting the handler, and the loader should be disabled when the diversion doesn't work.

All in all, a simpler fix is to change __wrap_sigaction to test registeredHandler.

Comment 5

[Douglas Crosher [:dougc]]

Attachment: Guard the ElfLoader sigaction wrapper against the SIGSEGV handler not being installed.

Thank you for the feedback.  The patch has been reworked as suggested.

There are a few other changes to the initialization code to better restore the prior handler in some of the bail out paths - happy to remove these?

The original patch was based on the result of Divert being determined at compile time.  I guess you must expect it might be a dynamic result in future?

It's a simple patch that corrects crashes so would it be appropriate to request it be uplifted after it has been proven in nightly?

Geoff: it would be interesting to confirm if this also fixes bug 980498?

Comment 6

[Geoff Brown [:gbrown]]

Comment on attachment 8393430 [details] [diff] [review]
Guard the ElfLoader sigaction wrapper against the SIGSEGV handler not being installed.

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

This patch also fixes bug 980498 (I verified .dmp files are created on SIGSEGV, in the Android 2.3 emulator).

Comment 7

[Mike Hommey [:glandium]]

Comment on attachment 8393430 [details] [diff] [review]
Guard the ElfLoader sigaction wrapper against the SIGSEGV handler not being installed.

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

::: mozglue/linker/ElfLoader.cpp
@@ +998,5 @@
>                                      PROT_READ | PROT_WRITE,
>                                      MAP_PRIVATE | MAP_ANONYMOUS, -1, 0));
> +  if (stackPtr.get() == MAP_FAILED) {
> +    /* Attempt to restore the original segfault signal handler. */
> +    sys_sigaction(SIGSEGV, &this->action, nullptr);

Should just move stackPtr.Assign and this test above sys_sigaction. This would avoid having to restore at all.

@@ +1008,5 @@
>    mprotect(stackPtr, stackPtr.GetLength(), PROT_NONE);
>    data->crash_int = 123;
>    stackPtr.Assign(MAP_FAILED, 0);
> +  /* Restore the original segfault signal handler. */
> +  sys_sigaction(SIGSEGV, &this->action, nullptr);

I guess you moved this for safety. Then you should move it above stackPtr.Assign(MAP_FAILED, 0);

Comment 8

[Douglas Crosher [:dougc]]

Attachment: Guard the ElfLoader sigaction wrapper against the SIGSEGV handler not being installed.

Thank you again for the feedback, these were good suggestions and have been applied.

gbrown: Thank you for testing.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5e6978476efa

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5e6978476efa