Re-land "Add stats to distinguish android renderer crashes."

Re-land "Add stats to distinguish android renderer crashes."

This CL is a precursor to expanding Clank stability scope
to all OOM protected renderers that crash while Chrome is
in the foreground.

The difference in scope is effectively the number of renderers
that crash as its corresponding tabs are all backgrounded.
This transition time is one second before the OOM protection
is dropped. Most, if not all, such renderer crashes are due
to the Android OOM killer. Overall difference in proposed
stability stats will not be much different, and will be easily
seen with the new histogram.

Expanding scope allows us to have native record which of
the renderer crashes actually results in a minidump file
being generated, thus demystifying whether the renderer
was OOM killed or actually crashed. This native stat can
be made more robust in the future, but this CL paves the
foundation, assuming Tab.RendererExitStatus is comparable
to Tab.RendererCrashStatus.

When the renderer is cleanly shutdown, there are no details
and no need to record any stats. Thus we skip stat recording
for NOTIFICATION_RENDERER_PROCESS_TERMINATED.

BUG=510327, 560813
Committed: https://crrev.com/273fee6d7f3ab923ba539c52c2a35a20844bdca4
Cr-Commit-Position: refs/heads/master@{#362511}

[Peter Wen, 2015-11-24 20:36:23]

Description was changed from

==========
Re-land "Add stats to distinguish android renderer crashes.

This CL is a precursor to expanding Clank stability scope
to all OOM protected renderers that crash while Chrome is
in the foreground.

The difference in scope is effectively the number of renderers
that crash as its corresponding tabs are all backgrounded.
This transition time is one second before the OOM protection
is dropped. Most, if not all, such renderer crashes are due
to the Android OOM killer. Overall difference in proposed
stability stats will not be much different, and will be easily
seen with the new histogram.

Expanding scope allows us to have native record which of
the renderer crashes actually results in a minidump file
being generated, thus demystifying whether the renderer
was OOM killed or actually crashed. This native stat can
be made more robust in the future, but this CL paves the
foundation, assuming Tab.RendererExitStatus is comparable
to Tab.RendererCrashStatus."

When the renderer is cleanly shutdown, there are no details
and no need to record any stats. Thus we skip stat recording
for NOTIFICATION_RENDERER_PROCESS_TERMINATED.

BUG=510327,560813
==========

to

==========
Re-land "Add stats to distinguish android renderer crashes."

This CL is a precursor to expanding Clank stability scope
to all OOM protected renderers that crash while Chrome is
in the foreground.

The difference in scope is effectively the number of renderers
that crash as its corresponding tabs are all backgrounded.
This transition time is one second before the OOM protection
is dropped. Most, if not all, such renderer crashes are due
to the Android OOM killer. Overall difference in proposed
stability stats will not be much different, and will be easily
seen with the new histogram.

Expanding scope allows us to have native record which of
the renderer crashes actually results in a minidump file
being generated, thus demystifying whether the renderer
was OOM killed or actually crashed. This native stat can
be made more robust in the future, but this CL paves the
foundation, assuming Tab.RendererExitStatus is comparable
to Tab.RendererCrashStatus.

When the renderer is cleanly shutdown, there are no details
and no need to record any stats. Thus we skip stat recording
for NOTIFICATION_RENDERER_PROCESS_TERMINATED.

BUG=510327,560813
==========

[Peter Wen, 2015-11-24 20:38:52]

wnwen@chromium.org changed reviewers:
+ yfriedman@chromium.org

[Peter Wen, 2015-11-24 20:38:53]

Hi Yaron and Primiano,

Fixed the NPE. For the TERMINATED notification, no details are provided, but for
the CLOSED notification, details are always provided.

Thanks, is there a way to run on the perf bots prior to committing?

Peter

[Yaron, 2015-11-24 21:17:37]

lgtm - FYI for next time please include the original as ps1 so it's easier to
diff

[Peter Wen, 2015-11-24 22:15:13]

On 2015/11/24 21:17:37, Yaron wrote:
> lgtm - FYI for next time please include the original as ps1 so it's easier to
> diff

Ah, right! Will do next time, thanks. :)

[Peter Wen, 2015-11-24 22:15:54]

wnwen@chromium.org changed reviewers:
+ cpu@chromium.org, isherman@chromium.org

[Peter Wen, 2015-11-24 22:15:54]

+cpu@ for OWNERS components/crash/*
+isherman@ for OWNERS tools/metrics/*

Thanks!

[Ilya Sherman, 2015-11-25 05:38:17]

Looks like there are no changes to the metrics code that I previously reviewed
-- right?  If so, LGTM.  In the future, feel free to TBR reviewers who have
previously reviewed the code, if the code they were reviewing hasn't changed.

[Peter Wen, 2015-11-25 14:36:33]

On 2015/11/25 05:38:17, Ilya Sherman wrote:
> Looks like there are no changes to the metrics code that I previously reviewed
> -- right?  If so, LGTM.  In the future, feel free to TBR reviewers who have
> previously reviewed the code, if the code they were reviewing hasn't changed.

Thanks Ilya, will TBR in the future.

@Carlos - new changes are exclusively in crash_dump_manager_android.cc.

[Peter Wen, 2015-11-30 18:26:41]

On 2015/11/25 14:36:33, Peter Wen wrote:
> On 2015/11/25 05:38:17, Ilya Sherman wrote:
> > Looks like there are no changes to the metrics code that I previously
reviewed
> > -- right?  If so, LGTM.  In the future, feel free to TBR reviewers who have
> > previously reviewed the code, if the code they were reviewing hasn't
changed.
> 
> Thanks Ilya, will TBR in the future.
> 
> @Carlos - new changes are exclusively in crash_dump_manager_android.cc.

PTAL - friendly post-holiday ping. :)

[cpu_(ooo_6.6-7.5), 2015-11-30 23:54:54]

lgtm w/ nits

https://codereview.chromium.org/1473753002/diff/1/components/crash/content/br...
File components/crash/content/browser/crash_dump_manager_android.cc (right):

https://codereview.chromium.org/1473753002/diff/1/components/crash/content/br...
components/crash/content/browser/crash_dump_manager_android.cc:164: /* app_state
*/ base::android::APPLICATION_STATE_UNKNOWN);
still finding the /*  */ inline comments horrible fyi, not to mention that the
right hand side already has the word 'status' and 'app state'

https://codereview.chromium.org/1473753002/diff/1/components/crash/content/br...
File components/crash/content/browser/crash_dump_manager_android.h (right):

https://codereview.chromium.org/1473753002/diff/1/components/crash/content/br...
components/crash/content/browser/crash_dump_manager_android.h:63: COUNT = 6
that is some strange enum style, for one you can remove the numbers and second
the count one sould be valid_miniump_while_count or someting like that.

[Peter Wen, 2015-12-01 18:40:41]

Thanks for the review!

https://codereview.chromium.org/1473753002/diff/1/components/crash/content/br...
File components/crash/content/browser/crash_dump_manager_android.cc (right):

https://codereview.chromium.org/1473753002/diff/1/components/crash/content/br...
components/crash/content/browser/crash_dump_manager_android.cc:164: /* app_state
*/ base::android::APPLICATION_STATE_UNKNOWN);
On 2015/11/30 23:54:54, cpu wrote:
> still finding the /*  */ inline comments horrible fyi, not to mention that the
> right hand side already has the word 'status' and 'app state'

Removed.

These were relics from when booleans were passed in. Thanks for noticing. :)

https://codereview.chromium.org/1473753002/diff/1/components/crash/content/br...
File components/crash/content/browser/crash_dump_manager_android.h (right):

https://codereview.chromium.org/1473753002/diff/1/components/crash/content/br...
components/crash/content/browser/crash_dump_manager_android.h:63: COUNT = 6
On 2015/11/30 23:54:54, cpu wrote:
> that is some strange enum style, for one you can remove the numbers and second
> the count one sould be valid_miniump_while_count or someting like that.

Done.

[Peter Wen, 2015-12-01 18:44:20]

The CQ bit was checked by wnwen@chromium.org

[Peter Wen, 2015-12-01 18:44:21]

The patchset sent to the CQ was uploaded after l-g-t-m from cpu@chromium.org,
yfriedman@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/1473753002/#ps20001
(title: "Fix per review.")

[commit-bot: I haz the power, 2015-12-01 18:45:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1473753002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1473753002/20001

[commit-bot: I haz the power, 2015-12-01 18:52:48]

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

[commit-bot: I haz the power, 2015-12-01 18:52:48]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Peter Wen, 2015-12-01 20:34:29]

The CQ bit was checked by wnwen@chromium.org

[Peter Wen, 2015-12-01 20:34:29]

The patchset sent to the CQ was uploaded after l-g-t-m from cpu@chromium.org,
yfriedman@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/1473753002/#ps40001
(title: "Rebase.")

[Peter Wen, 2015-12-01 20:48:18]

Patchset #3 (id:40001) has been deleted

[commit-bot: I haz the power, 2015-12-01 20:52:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1473753002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1473753002/20001

[commit-bot: I haz the power, 2015-12-01 21:51:32]

Description was changed from

==========
Re-land "Add stats to distinguish android renderer crashes."

This CL is a precursor to expanding Clank stability scope
to all OOM protected renderers that crash while Chrome is
in the foreground.

The difference in scope is effectively the number of renderers
that crash as its corresponding tabs are all backgrounded.
This transition time is one second before the OOM protection
is dropped. Most, if not all, such renderer crashes are due
to the Android OOM killer. Overall difference in proposed
stability stats will not be much different, and will be easily
seen with the new histogram.

Expanding scope allows us to have native record which of
the renderer crashes actually results in a minidump file
being generated, thus demystifying whether the renderer
was OOM killed or actually crashed. This native stat can
be made more robust in the future, but this CL paves the
foundation, assuming Tab.RendererExitStatus is comparable
to Tab.RendererCrashStatus.

When the renderer is cleanly shutdown, there are no details
and no need to record any stats. Thus we skip stat recording
for NOTIFICATION_RENDERER_PROCESS_TERMINATED.

BUG=510327,560813
==========

to

==========
Re-land "Add stats to distinguish android renderer crashes."

This CL is a precursor to expanding Clank stability scope
to all OOM protected renderers that crash while Chrome is
in the foreground.

The difference in scope is effectively the number of renderers
that crash as its corresponding tabs are all backgrounded.
This transition time is one second before the OOM protection
is dropped. Most, if not all, such renderer crashes are due
to the Android OOM killer. Overall difference in proposed
stability stats will not be much different, and will be easily
seen with the new histogram.

Expanding scope allows us to have native record which of
the renderer crashes actually results in a minidump file
being generated, thus demystifying whether the renderer
was OOM killed or actually crashed. This native stat can
be made more robust in the future, but this CL paves the
foundation, assuming Tab.RendererExitStatus is comparable
to Tab.RendererCrashStatus.

When the renderer is cleanly shutdown, there are no details
and no need to record any stats. Thus we skip stat recording
for NOTIFICATION_RENDERER_PROCESS_TERMINATED.

BUG=510327,560813
==========

[commit-bot: I haz the power, 2015-12-01 21:51:33]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-12-01 21:52:24]

Description was changed from

==========
Re-land "Add stats to distinguish android renderer crashes."

This CL is a precursor to expanding Clank stability scope
to all OOM protected renderers that crash while Chrome is
in the foreground.

The difference in scope is effectively the number of renderers
that crash as its corresponding tabs are all backgrounded.
This transition time is one second before the OOM protection
is dropped. Most, if not all, such renderer crashes are due
to the Android OOM killer. Overall difference in proposed
stability stats will not be much different, and will be easily
seen with the new histogram.

Expanding scope allows us to have native record which of
the renderer crashes actually results in a minidump file
being generated, thus demystifying whether the renderer
was OOM killed or actually crashed. This native stat can
be made more robust in the future, but this CL paves the
foundation, assuming Tab.RendererExitStatus is comparable
to Tab.RendererCrashStatus.

When the renderer is cleanly shutdown, there are no details
and no need to record any stats. Thus we skip stat recording
for NOTIFICATION_RENDERER_PROCESS_TERMINATED.

BUG=510327,560813
==========

to

==========
Re-land "Add stats to distinguish android renderer crashes."

This CL is a precursor to expanding Clank stability scope
to all OOM protected renderers that crash while Chrome is
in the foreground.

The difference in scope is effectively the number of renderers
that crash as its corresponding tabs are all backgrounded.
This transition time is one second before the OOM protection
is dropped. Most, if not all, such renderer crashes are due
to the Android OOM killer. Overall difference in proposed
stability stats will not be much different, and will be easily
seen with the new histogram.

Expanding scope allows us to have native record which of
the renderer crashes actually results in a minidump file
being generated, thus demystifying whether the renderer
was OOM killed or actually crashed. This native stat can
be made more robust in the future, but this CL paves the
foundation, assuming Tab.RendererExitStatus is comparable
to Tab.RendererCrashStatus.

When the renderer is cleanly shutdown, there are no details
and no need to record any stats. Thus we skip stat recording
for NOTIFICATION_RENDERER_PROCESS_TERMINATED.

BUG=510327,560813

Committed: https://crrev.com/273fee6d7f3ab923ba539c52c2a35a20844bdca4
Cr-Commit-Position: refs/heads/master@{#362511}
==========

[commit-bot: I haz the power, 2015-12-01 21:52:25]

Patchset 2 (id:??) landed as
https://crrev.com/273fee6d7f3ab923ba539c52c2a35a20844bdca4
Cr-Commit-Position: refs/heads/master@{#362511}