android: Crash child immediately on uncaught exception

android: Crash child immediately on uncaught exception

By installing our own handler that kills the process immediately.
The default handler will pop up a dialog, and the process will stay
alive until that dialog is dismissed by user. This is generally bad
because browser will not notice this crash until user dismisses the
dialog as well.

Note that suppressing this dialog matches the behavior for native
crashes. See breakpad::FinalizeCrashDoneAndroid for the equivalent
logic.

Refactor base a bit so the logic to check for release android build
can be shared.

BUG=680775
Review-Url: https://codereview.chromium.org/2675013002
Cr-Commit-Position: refs/heads/master@{#448308}
Committed: https://chromium.googlesource.com/chromium/src/+/29b572b3ffeb491584ce5e6cd24a5eca9ac4d738

[boliu, 2017-02-03 21:22:31]

The CQ bit was checked by boliu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-03 21:22:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-03 22:38:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-03 22:38:23]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[boliu, 2017-02-03 23:21:31]

Description was changed from

==========
android: Crash child immediately on uncaught exception

By installing our own handler that kills the process immediately.
The default handler will pop up a dialog, and the process will stay
alive until that dialog is dismissed by user. This is generally bad
because browser will not notice this crash until user dismisses the
dialog as well.

Note that suppressing this dialog matches the behavior for native
crashes. See breakpad::FinalizeCrashDoneAndroid for the equivalent
logic.

BUG=680775
==========

to

==========
android: Crash child immediately on uncaught exception

By installing our own handler that kills the process immediately.
The default handler will pop up a dialog, and the process will stay
alive until that dialog is dismissed by user. This is generally bad
because browser will not notice this crash until user dismisses the
dialog as well.

Note that suppressing this dialog matches the behavior for native
crashes. See breakpad::FinalizeCrashDoneAndroid for the equivalent
logic.

Refactor base a bit so the logic to check for release android OS
can be shared

BUG=680775
==========

[boliu, 2017-02-03 23:27:32]

boliu@chromium.org changed reviewers:
+ agrieve@chromium.org, torne@chromium.org

[boliu, 2017-02-03 23:27:32]

ptal

torne for base
agrieve for content stuff, since this was his idea :)

I tested on chrome. Webview should be fine too (and probably desired as well).
I'll probably loop in chromecast folks after everyone else agrees on this.

[boliu, 2017-02-03 23:32:10]

Description was changed from

==========
android: Crash child immediately on uncaught exception

By installing our own handler that kills the process immediately.
The default handler will pop up a dialog, and the process will stay
alive until that dialog is dismissed by user. This is generally bad
because browser will not notice this crash until user dismisses the
dialog as well.

Note that suppressing this dialog matches the behavior for native
crashes. See breakpad::FinalizeCrashDoneAndroid for the equivalent
logic.

Refactor base a bit so the logic to check for release android OS
can be shared

BUG=680775
==========

to

==========
android: Crash child immediately on uncaught exception

By installing our own handler that kills the process immediately.
The default handler will pop up a dialog, and the process will stay
alive until that dialog is dismissed by user. This is generally bad
because browser will not notice this crash until user dismisses the
dialog as well.

Note that suppressing this dialog matches the behavior for native
crashes. See breakpad::FinalizeCrashDoneAndroid for the equivalent
logic.

Refactor base a bit so the logic to check for release android build
can be shared.

BUG=680775
==========

[agrieve, 2017-02-04 01:27:53]

Implementation looks great! lgtm /w comments

https://codereview.chromium.org/2675013002/diff/20001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/app/KillChildUncaughtExceptionHandler.java
(right):

https://codereview.chromium.org/2675013002/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/app/KillChildUncaughtExceptionHandler.java:29:
// Only suppress default dialog on release builds.
I see that FinalizeCrashDoneAndroid() also checks for user builds, but it's not
clear to me why we want this behaviour? We test all the time with userdebug, so
I'd think we'd want to exercise this logic there as well. The browser process
shows a sad tab when a renderer / gpu crashes anyways, so that should be a good
indication to a developer that they should look at their logs.

https://codereview.chromium.org/2675013002/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/app/KillChildUncaughtExceptionHandler.java:41:
Process.killProcess(Process.myPid());
nit: might be worth commenting that this is also how Android framework kills
itself (otherwise it's mighty odd to call killProcess as well as System.exit().

[boliu, 2017-02-04 05:59:12]

https://codereview.chromium.org/2675013002/diff/20001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/app/KillChildUncaughtExceptionHandler.java
(right):

https://codereview.chromium.org/2675013002/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/app/KillChildUncaughtExceptionHandler.java:29:
// Only suppress default dialog on release builds.
On 2017/02/04 01:27:53, agrieve wrote:
> I see that FinalizeCrashDoneAndroid() also checks for user builds, but it's
not
> clear to me why we want this behaviour? We test all the time with userdebug,
so
> I'd think we'd want to exercise this logic there as well. The browser process
> shows a sad tab when a renderer / gpu crashes anyways, so that should be a
good
> indication to a developer that they should look at their logs.

Yeah I think it's an orthogonal debate, but probably worth discussing.

I think the argument for not suppressing it in developer build is that for
native crashes, microdump is harder to deal with than debuggerd output. Roughly
similar here, if this was enabled on developer builds, then a java exception
wouldn't print anything at all.

That begs a related question though.. should this handler dump the java stack to
logcat? Maybe it's better to dump the stack here (so release builds will have
stack dumped twice), and always enable it?

https://codereview.chromium.org/2675013002/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/app/KillChildUncaughtExceptionHandler.java:41:
Process.killProcess(Process.myPid());
On 2017/02/04 01:27:53, agrieve wrote:
> nit: might be worth commenting that this is also how Android framework kills
> itself (otherwise it's mighty odd to call killProcess as well as
System.exit().

ack

[agrieve, 2017-02-05 23:34:18]

https://codereview.chromium.org/2675013002/diff/20001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/app/KillChildUncaughtExceptionHandler.java
(right):

https://codereview.chromium.org/2675013002/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/app/KillChildUncaughtExceptionHandler.java:29:
// Only suppress default dialog on release builds.
On 2017/02/04 05:59:12, boliu wrote:
> On 2017/02/04 01:27:53, agrieve wrote:
> > I see that FinalizeCrashDoneAndroid() also checks for user builds, but it's
> not
> > clear to me why we want this behaviour? We test all the time with userdebug,
> so
> > I'd think we'd want to exercise this logic there as well. The browser
process
> > shows a sad tab when a renderer / gpu crashes anyways, so that should be a
> good
> > indication to a developer that they should look at their logs.
> 
> Yeah I think it's an orthogonal debate, but probably worth discussing.
> 
> I think the argument for not suppressing it in developer build is that for
> native crashes, microdump is harder to deal with than debuggerd output.
Roughly
> similar here, if this was enabled on developer builds, then a java exception
> wouldn't print anything at all.
> 
> That begs a related question though.. should this handler dump the java stack
to
> logcat? Maybe it's better to dump the stack here (so release builds will have
> stack dumped twice), and always enable it?

Hearing that, I think I'd be happy either way. Certainly adding what you've said
here as a comment would make sense though (pointing out the tradeoff).

[Torne, 2017-02-06 12:29:21]

lgtm

[boliu, 2017-02-06 12:59:18]

https://codereview.chromium.org/2675013002/diff/20001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/app/KillChildUncaughtExceptionHandler.java
(right):

https://codereview.chromium.org/2675013002/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/app/KillChildUncaughtExceptionHandler.java:29:
// Only suppress default dialog on release builds.
On 2017/02/05 23:34:18, agrieve wrote:
> On 2017/02/04 05:59:12, boliu wrote:
> > On 2017/02/04 01:27:53, agrieve wrote:
> > > I see that FinalizeCrashDoneAndroid() also checks for user builds, but
it's
> > not
> > > clear to me why we want this behaviour? We test all the time with
userdebug,
> > so
> > > I'd think we'd want to exercise this logic there as well. The browser
> process
> > > shows a sad tab when a renderer / gpu crashes anyways, so that should be a
> > good
> > > indication to a developer that they should look at their logs.
> > 
> > Yeah I think it's an orthogonal debate, but probably worth discussing.
> > 
> > I think the argument for not suppressing it in developer build is that for
> > native crashes, microdump is harder to deal with than debuggerd output.
> Roughly
> > similar here, if this was enabled on developer builds, then a java exception
> > wouldn't print anything at all.
> > 
> > That begs a related question though.. should this handler dump the java
stack
> to
> > logcat? Maybe it's better to dump the stack here (so release builds will
have
> > stack dumped twice), and always enable it?
> 
> Hearing that, I think I'd be happy either way. Certainly adding what you've
said
> here as a comment would make sense though (pointing out the tradeoff).

Decided to just add a comment, and left behavior as is.

https://codereview.chromium.org/2675013002/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/app/KillChildUncaughtExceptionHandler.java:41:
Process.killProcess(Process.myPid());
On 2017/02/04 05:59:12, boliu wrote:
> On 2017/02/04 01:27:53, agrieve wrote:
> > nit: might be worth commenting that this is also how Android framework kills
> > itself (otherwise it's mighty odd to call killProcess as well as
> System.exit().
> 
> ack

Done.

[boliu, 2017-02-06 13:00:00]

The CQ bit was checked by boliu@chromium.org

[boliu, 2017-02-06 13:00:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
agrieve@chromium.org, torne@chromium.org
Link to the patchset: https://codereview.chromium.org/2675013002/#ps40001
(title: "comments")

[commit-bot: I haz the power, 2017-02-06 13:00:10]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[boliu, 2017-02-06 13:11:33]

The CQ bit was unchecked by boliu@chromium.org

[boliu, 2017-02-06 13:13:15]

boliu@chromium.org changed reviewers:
+ halliwell@chromium.org

[boliu, 2017-02-06 13:13:16]

Oh wait, I said I was gonna loop in chromecast.

+halliwell since this impacts chromecast as well. I assume it should be fine for
the same reasons as chrome on android?

[halliwell, 2017-02-06 15:36:59]

halliwell@chromium.org changed reviewers:
+ sanfin@chromium.org, tsunghung@chromium.org

[halliwell, 2017-02-06 15:41:34]

On 2017/02/06 13:13:16, boliu wrote:
> Oh wait, I said I was gonna loop in chromecast.
> 
> +halliwell since this impacts chromecast as well. I assume it should be fine
for
> the same reasons as chrome on android?

+sanfin/tsunghung - PTAL

[Simeon, 2017-02-06 18:04:08]

On 2017/02/06 15:41:34, halliwell wrote:
> On 2017/02/06 13:13:16, boliu wrote:
> > Oh wait, I said I was gonna loop in chromecast.
> > 
> > +halliwell since this impacts chromecast as well. I assume it should be fine
> for
> > the same reasons as chrome on android?
> 
> +sanfin/tsunghung - PTAL

lgtm

[boliu, 2017-02-06 18:09:53]

The CQ bit was checked by boliu@chromium.org

[commit-bot: I haz the power, 2017-02-06 18:10:20]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[AndyWu, 2017-02-06 18:14:33]

lgtm

[commit-bot: I haz the power, 2017-02-06 18:14:44]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1486404593916640,
"parent_rev": "21b6f3db42a1ff35b33993812b2fd851922c4770", "commit_rev":
"29b572b3ffeb491584ce5e6cd24a5eca9ac4d738"}

[commit-bot: I haz the power, 2017-02-06 18:15:24]

Description was changed from

==========
android: Crash child immediately on uncaught exception

By installing our own handler that kills the process immediately.
The default handler will pop up a dialog, and the process will stay
alive until that dialog is dismissed by user. This is generally bad
because browser will not notice this crash until user dismisses the
dialog as well.

Note that suppressing this dialog matches the behavior for native
crashes. See breakpad::FinalizeCrashDoneAndroid for the equivalent
logic.

Refactor base a bit so the logic to check for release android build
can be shared.

BUG=680775
==========

to

==========
android: Crash child immediately on uncaught exception

By installing our own handler that kills the process immediately.
The default handler will pop up a dialog, and the process will stay
alive until that dialog is dismissed by user. This is generally bad
because browser will not notice this crash until user dismisses the
dialog as well.

Note that suppressing this dialog matches the behavior for native
crashes. See breakpad::FinalizeCrashDoneAndroid for the equivalent
logic.

Refactor base a bit so the logic to check for release android build
can be shared.

BUG=680775

Review-Url: https://codereview.chromium.org/2675013002
Cr-Commit-Position: refs/heads/master@{#448308}
Committed:
https://chromium.googlesource.com/chromium/src/+/29b572b3ffeb491584ce5e6cd24a...
==========

[commit-bot: I haz the power, 2017-02-06 18:15:26]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/29b572b3ffeb491584ce5e6cd24a...