Workaround Android sigaction bug

Workaround Android sigaction bug

On Android L+, signal and sigaction symbols are provided by libsigchain
that override the system's versions. There is a bug in these functions
where they essentially ignore requests to install SIG_DFL.

Workaround this issue by explicitly performing a syscall to
__NR_rt_sigaction to install SIG_DFL on Android.

BUG=473973

[cjhopman]

mark@ ptal

[primiano]

It's very unfortunate that we have to do this but, in all honesty, considering
that this is a platform bug, this seems to me the cleanest and safest way to
work around this bug.

Not an owner, but FWIW this LGTM.
Thanks for preparing this Chris.

https://breakpad.appspot.com/1804002/diff/20001/client/linux/handler/exceptio...
File client/linux/handler/exception_handler.cc (right):

https://breakpad.appspot.com/1804002/diff/20001/client/linux/handler/exceptio...
client/linux/handler/exception_handler.cc:202: syscall(__NR_sigaction, sig, &sa,
NULL);
Looking at android sources: if we want to 100% mimic what they do upon signal()
[1], we'd they set also sa_flags = SA_RESTART.
On the other side, looking at the man page for SA_RESTART, It doesn't really
feel we should have that after a crash handler.

[1]
https://android.googlesource.com/platform/bionic/+/android-5.1.0_r3/libc/bion...

[Mark Mentovai]

LGTM

https://breakpad.appspot.com/1804002/diff/20001/client/linux/handler/exceptio...
File client/linux/handler/exception_handler.cc (right):

https://breakpad.appspot.com/1804002/diff/20001/client/linux/handler/exceptio...
client/linux/handler/exception_handler.cc:202: syscall(__NR_sigaction, sig, &sa,
NULL);
primiano wrote:
> Looking at android sources: if we want to 100% mimic what they do upon
signal()
> [1], we'd they set also sa_flags = SA_RESTART.
> On the other side, looking at the man page for SA_RESTART, It doesn't really
> feel we should have that after a crash handler.
> 
> [1]
>
https://android.googlesource.com/platform/bionic/+/android-5.1.0_r3/libc/bion...

We should do this to mimic signal(). Crash signals occur synchronously and won’t
interrupt a system call anyway.

[cjhopman]

https://breakpad.appspot.com/1804002/diff/20001/client/linux/handler/exceptio...
File client/linux/handler/exception_handler.cc (right):

https://breakpad.appspot.com/1804002/diff/20001/client/linux/handler/exceptio...
client/linux/handler/exception_handler.cc:202: syscall(__NR_sigaction, sig, &sa,
NULL);
On 2015/04/10 13:07:55, Mark Mentovai wrote:
> primiano wrote:
> > Looking at android sources: if we want to 100% mimic what they do upon
> signal()
> > [1], we'd they set also sa_flags = SA_RESTART.
> > On the other side, looking at the man page for SA_RESTART, It doesn't really
> > feel we should have that after a crash handler.
> > 
> > [1]
> >
>
https://android.googlesource.com/platform/bionic/+/android-5.1.0_r3/libc/bion...
> 
> We should do this to mimic signal(). Crash signals occur synchronously and
won’t
> interrupt a system call anyway.

Done.

[Mark Mentovai]

Committed r1438

[tpurtell]

On 2015/04/10 17:57:39, Mark Mentovai wrote:
> Committed r1438

I noticed that Android System WebView just released in the past two days seems
to have started eating signals in funny ways.  I don't know if they have
included this changeset or not, but I found myself here exploring why it may be
occurring.

Signal chaining via reissuing a signal with SA_RESTART seems to be fairly
traditional.  Normally to chain, you reset the handler to the previous handler
rather than SIGDFL, so that it has a chance to do its work.  In the Android
case, the runtime has registered a signal handler which sends an IPC to
debuggerd and triggers Play Store native crash reporting.

I believe that setting the handler to default would have the result of hiding
all native crash traces for developers.  There may be some at various levels:
app, library, runtime, manufacturer.

Regardless, I would expect the chromium developers to have their own signal
handlers for various reasons, so I'm not so sure that this is the cause of the
problem I am seeing.  I noticed they incorporated this change several days ago.

[cjhopman]

On 2015/04/25 08:11:31, tpurtell wrote:
> On 2015/04/10 17:57:39, Mark Mentovai wrote:
> > Committed r1438
> 
> I noticed that Android System WebView just released in the past two days seems
> to have started eating signals in funny ways.  I don't know if they have
> included this changeset or not, but I found myself here exploring why it may
be
> occurring.
> 
> Signal chaining via reissuing a signal with SA_RESTART seems to be fairly
> traditional.  Normally to chain, you reset the handler to the previous handler
> rather than SIGDFL, so that it has a chance to do its work.  In the Android
> case, the runtime has registered a signal handler which sends an IPC to
> debuggerd and triggers Play Store native crash reporting.
> 
> I believe that setting the handler to default would have the result of hiding
> all native crash traces for developers.  There may be some at various levels:
> app, library, runtime, manufacturer.
> 
> Regardless, I would expect the chromium developers to have their own signal
> handlers for various reasons, so I'm not so sure that this is the cause of the
> problem I am seeing.  I noticed they incorporated this change several days
ago.

If you look at how we handle crashes in Chrome, you'll see that we only
reinstall SIG_DFL for renderer processes on JB-MR2+. We do this explicitly to
avoid the Play Store's native crash reporting (because it shows a disruptive
dialog to the user). All browser crashes still reinstall the previous handler.

Afaik, webview doesn't install the breakpad handler at all, though.

[cjhopman]

On 2015/04/26 00:26:08, cjhopman wrote:
> On 2015/04/25 08:11:31, tpurtell wrote:
> > On 2015/04/10 17:57:39, Mark Mentovai wrote:
> > > Committed r1438
> > 
> > I noticed that Android System WebView just released in the past two days
seems
> > to have started eating signals in funny ways.  I don't know if they have
> > included this changeset or not, but I found myself here exploring why it may
> be
> > occurring.
> > 
> > Signal chaining via reissuing a signal with SA_RESTART seems to be fairly
> > traditional.  Normally to chain, you reset the handler to the previous
handler
> > rather than SIGDFL, so that it has a chance to do its work.  In the Android
> > case, the runtime has registered a signal handler which sends an IPC to
> > debuggerd and triggers Play Store native crash reporting.
> > 
> > I believe that setting the handler to default would have the result of
hiding
> > all native crash traces for developers.  There may be some at various
levels:
> > app, library, runtime, manufacturer.
> > 
> > Regardless, I would expect the chromium developers to have their own signal
> > handlers for various reasons, so I'm not so sure that this is the cause of
the
> > problem I am seeing.  I noticed they incorporated this change several days
> ago.
> 
> If you look at how we handle crashes in Chrome, you'll see that we only
> reinstall SIG_DFL for renderer processes on JB-MR2+. We do this explicitly to
> avoid the Play Store's native crash reporting (because it shows a disruptive
> dialog to the user). All browser crashes still reinstall the previous handler.
> 
> Afaik, webview doesn't install the breakpad handler at all, though.

I'd suggest filing a bug at http://crbug.com/new and specify the behavior that
you're seeing.

[tpurtell]

On 2015/04/26 00:26:50, cjhopman wrote:
> On 2015/04/26 00:26:08, cjhopman wrote:
> > On 2015/04/25 08:11:31, tpurtell wrote:
> > > On 2015/04/10 17:57:39, Mark Mentovai wrote:
> > > > Committed r1438
> > > 
> > > I noticed that Android System WebView just released in the past two days
> seems
> > > to have started eating signals in funny ways.  I don't know if they have
> > > included this changeset or not, but I found myself here exploring why it
may
> > be
> > > occurring.
> > > 
> > > Signal chaining via reissuing a signal with SA_RESTART seems to be fairly
> > > traditional.  Normally to chain, you reset the handler to the previous
> handler
> > > rather than SIGDFL, so that it has a chance to do its work.  In the
Android
> > > case, the runtime has registered a signal handler which sends an IPC to
> > > debuggerd and triggers Play Store native crash reporting.
> > > 
> > > I believe that setting the handler to default would have the result of
> hiding
> > > all native crash traces for developers.  There may be some at various
> levels:
> > > app, library, runtime, manufacturer.
> > > 
> > > Regardless, I would expect the chromium developers to have their own
signal
> > > handlers for various reasons, so I'm not so sure that this is the cause of
> the
> > > problem I am seeing.  I noticed they incorporated this change several days
> > ago.
> > 
> > If you look at how we handle crashes in Chrome, you'll see that we only
> > reinstall SIG_DFL for renderer processes on JB-MR2+. We do this explicitly
to
> > avoid the Play Store's native crash reporting (because it shows a disruptive
> > dialog to the user). All browser crashes still reinstall the previous
handler.
> > 
> > Afaik, webview doesn't install the breakpad handler at all, though.
> 
> I'd suggest filing a bug at http://crbug.com/new and specify the behavior that
> you're seeing.

Thanks for the reply Chris.

Starting in Lollipop, the WebView in Android uses Chromium (Android System
WebView).  I suppose that in the case where Chromium is embedded in another
process, that process may being serving as the renderer. 

FYI this is the only log I get on 5.0+ with M42 WebView when a signal would
normally be handled in process.

* W/google-breakpad(32239): -----BEGIN BREAKPAD MICRODUMP-----
* W/google-breakpad(32239): O A arm 08 aarch64 3.10.61-4351282 #1 SMP PREEMPT
Fri Mar 27 02:44:10 KST 2015
* W/google-breakpad(32239): S 0 DEAFF740 DEAFF000 00001000
* W/google-breakpad(32239): S DEAFF000 58F3F2F.... lots of stuff

I suppose a change to use breakpad within the Chromium powered WebView combined
with this could swallow signals on 5.0+

My situation is a bit complicated because my app is using the Xamarin runtime
(mono VM).  This catches SIGSEGV in the case of a programming error and
translates it into a C# exception.  I would like to be able to catch those
errors and report them, so that I can correct them.  Unfortunately, I have lost
that visibility.

I suppose the ultimate test case would be to use native JNI to cause a SIGSEGV
and then showing that in a process with WebView, breakpad swallows this
exception (with no debuggerd report or UI popup).  I will try to put this
together.

If you happen to know the guy or gal who deals with crash reporting on the new
Chromium WebView, perhaps you could give them a heads up.

[primiano]

It would be much better to move this discussion to a bug.
Yes, as you are pointing out WebView form M42 uses breakpad for microdump
reporting.
It would be easier if you could define the "eating signals in funny ways",
possibly with a repro case.
If you want to do funky user-space handling of SIGSEGV my suggestion would be to
make sure that your signal handler is more prioritary than WebView's one.
Even if you solve the "eating" problem, that would still generate a microdump on
console every time you hit a managed exception, which is probably not what you
want.

The only way to achieve this with POSIX signals is to register your signal
handler later (latest handlers take precedence).

In practice this means that you have to initialise your Xamarin runtime AFTER
WebView has been loaded.
I will try to look into the signal "eating" problem. The crash handler should
not eat any signal (But, even though, that is not going to really solve your
problem)

[primiano]

Just a small correction to what I said:

> that would still generate a microdump on console every time you hit a managed
exception

This is not really true. Actually the breakpad crash handler will handle only
the first one and  self-unregister after that.
So, by design, if your runtime registered a SIGSEGV before WebView has been
loaded, you should get a microdump the first time you hit a SEGV and then your
runtime should keep handling the remaining signals.

[tpurtell]

I filed an issue report:
http://code.google.com/p/chromium/issues/detail?id=481420

Maybe breakpad could fully avoid libsignchain issue by reading and registering
handlers with a direct syscall to __NR_rt_sigaction, and directly invoke the
parent handler rather then trying to unregister.

[torne]

Fix for WebView eating signals on user builds is here:
https://codereview.chromium.org/1105293003/ - the problem is in the chromium
crash component, not breakpad itself.