Improve reporting of subprocess crashes....

Improve reporting of subprocess crashes.

-Split extension renderer crashes out of the existing UMA renderer crash metric 
into its own metric.

-Add a new metric for the sum of all ChildProcessHost crashes.

-Add histograms for each crash type.


BUG=28022
TEST=We should start getting more crash reports in UMA and histograms.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=33794

[kuchhal, 2009-12-03 22:47:25]

http://codereview.chromium.org/468005/diff/1/5
File chrome/browser/metrics/metrics_service.cc (right):

http://codereview.chromium.org/468005/diff/1/5#newcode1696
chrome/browser/metrics/metrics_service.cc:1696:
IncrementPrefValue(prefs::kStabilityChildProcessCrashCount);
May be move this down in switch case? Also I think if we do this, we will count
plugin crashes twice (once in kStabilityChildProcessCrashCount and also in
kStabilityPluginCrashes). May be we should exclude plugin crashes from this
value?

http://codereview.chromium.org/468005/diff/1/8
File chrome/browser/renderer_host/browser_render_process_host.cc (right):

http://codereview.chromium.org/468005/diff/1/8#newcode817
chrome/browser/renderer_host/browser_render_process_host.cc:817:
extension_process_ ? 2 : 1);
Do we need this since we anyway have individual field for Extension renderer
crashes?

http://codereview.chromium.org/468005/diff/1/2
File chrome/common/child_process_host.cc (right):

http://codereview.chromium.org/468005/diff/1/2#newcode190
chrome/common/child_process_host.cc:190:
UMA_HISTOGRAM_COUNTS("ChildProcess.Crashes", this->type());
shouldnt this be inside if (did_crash)?

[asargent_no_longer_on_chrome, 2009-12-03 23:52:01]

http://codereview.chromium.org/468005/diff/1/5
File chrome/browser/metrics/metrics_service.cc (right):

http://codereview.chromium.org/468005/diff/1/5#newcode1696
chrome/browser/metrics/metrics_service.cc:1696:
IncrementPrefValue(prefs::kStabilityChildProcessCrashCount);
On 2009/12/03 22:47:25, kuchhal wrote:
> May be move this down in switch case? Also I think if we do this, we will
count
> plugin crashes twice (once in kStabilityChildProcessCrashCount and also in
> kStabilityPluginCrashes). May be we should exclude plugin crashes from this
> value?

These are both good suggestions. (I had thought about excluding plugins myself,
but was on the fence about it).

http://codereview.chromium.org/468005/diff/1/8
File chrome/browser/renderer_host/browser_render_process_host.cc (right):

http://codereview.chromium.org/468005/diff/1/8#newcode817
chrome/browser/renderer_host/browser_render_process_host.cc:817:
extension_process_ ? 2 : 1);
On 2009/12/03 22:47:25, kuchhal wrote:
> Do we need this since we anyway have individual field for Extension renderer
> crashes?
It's not strictly necessary, but I think it might be nice to have alongside the
other new histogram for child process crashes.

http://codereview.chromium.org/468005/diff/1/2
File chrome/common/child_process_host.cc (right):

http://codereview.chromium.org/468005/diff/1/2#newcode190
chrome/common/child_process_host.cc:190:
UMA_HISTOGRAM_COUNTS("ChildProcess.Crashes", this->type());
On 2009/12/03 22:47:25, kuchhal wrote:
> shouldnt this be inside if (did_crash)?

Yes, good catch.

[kuchhal, 2009-12-04 00:04:09]

lgtm.

minor nit below.

http://codereview.chromium.org/468005/diff/5001/5005
File chrome/browser/metrics/metrics_service.cc (right):

http://codereview.chromium.org/468005/diff/5001/5005#newcode1713
chrome/browser/metrics/metrics_service.cc:1713: if (child_details->type() !=
ChildProcessInfo::PLUGIN_PROCESS) {
Can you please add a comment here explaining why we are excluding plugin
processes?

[jar (doing other things), 2009-12-18 16:28:14]

See comments below.

http://codereview.chromium.org/468005/diff/3007/3013
File chrome/browser/metrics/metrics_log.cc (right):

http://codereview.chromium.org/468005/diff/3007/3013#newcode452
chrome/browser/metrics/metrics_log.cc:452:
WriteIntAttribute("extensionrenderercrashcount", count);
Do you have corresponding changes to the protobuffer and logging code to support
this?

http://codereview.chromium.org/468005/diff/3007/3011
File chrome/browser/metrics/metrics_service.cc (right):

http://codereview.chromium.org/468005/diff/3007/3011#newcode1715
chrome/browser/metrics/metrics_service.cc:1715: if (child_details->type() !=
ChildProcessInfo::PLUGIN_PROCESS) {
There is a surprising asymmetry in this code.  I'd expect an else clause here to
record the other crashes... or I'd expect this section to only have
stats-counter increments, and then this crash counter to be incremented along
side the other tally.

http://codereview.chromium.org/468005/diff/3007/3014
File chrome/browser/renderer_host/browser_render_process_host.cc (right):

http://codereview.chromium.org/468005/diff/3007/3014#newcode817
chrome/browser/renderer_host/browser_render_process_host.cc:817:
extension_process_ ? 2 : 1);
I assume you're using 2 an 1 as specific bucket types for counters.  I't might
be nice to use an enum, with a good name, and not just have raw constants
appearing here.

http://codereview.chromium.org/468005/diff/3007/3009
File chrome/common/child_process_info.h (right):

http://codereview.chromium.org/468005/diff/3007/3009#newcode18
chrome/common/child_process_info.h:18: UNKNOWN_PROCESS = 1,
This change appears to re-number all entries in this list.  
Were these entries never used before?

[asargent_no_longer_on_chrome, 2010-01-05 01:01:19]

FYI, I had already committed this change, but I will create a new CL to follow
up on your concerns. Thanks for the review!

http://codereview.chromium.org/468005/diff/3007/3013
File chrome/browser/metrics/metrics_log.cc (right):

http://codereview.chromium.org/468005/diff/3007/3013#newcode452
chrome/browser/metrics/metrics_log.cc:452:
WriteIntAttribute("extensionrenderercrashcount", count);
On 2009/12/18 16:28:14, jar wrote:
> Do you have corresponding changes to the protobuffer and logging code to
support
> this?

I was under the impression that doing this here is sufficient for the client
side of things. Evan mentioned in the bug that we have someone (Jeff Bailey I
believe) working on the UMA server side, and I was planning on asking him to
make the relevant server changes. Please let me know if there's more that needs
to happen in the client than this code here which puts the new value in the xml
log.

http://codereview.chromium.org/468005/diff/3007/3011
File chrome/browser/metrics/metrics_service.cc (right):

http://codereview.chromium.org/468005/diff/3007/3011#newcode1715
chrome/browser/metrics/metrics_service.cc:1715: if (child_details->type() !=
ChildProcessInfo::PLUGIN_PROCESS) {
On 2009/12/18 16:28:14, jar wrote:
> There is a surprising asymmetry in this code.  I'd expect an else clause here
to
> record the other crashes... or I'd expect this section to only have
> stats-counter increments, and then this crash counter to be incremented along
> side the other tally.

The cause of this weirdness is probably that the stats struct is used to count
per-plugin values, whereas I am keeping a global counter of all non-renderer
child process crashes.

I'll prepare a separate CL that tries to make this a little more clear.

http://codereview.chromium.org/468005/diff/3007/3014
File chrome/browser/renderer_host/browser_render_process_host.cc (right):

http://codereview.chromium.org/468005/diff/3007/3014#newcode817
chrome/browser/renderer_host/browser_render_process_host.cc:817:
extension_process_ ? 2 : 1);
On 2009/12/18 16:28:14, jar wrote:
> I assume you're using 2 an 1 as specific bucket types for counters.  I't might
> be nice to use an enum, with a good name, and not just have raw constants
> appearing here.

That's a good suggestion. I'll add this to to my follow-up CL.

http://codereview.chromium.org/468005/diff/3007/3009
File chrome/common/child_process_info.h (right):

http://codereview.chromium.org/468005/diff/3007/3009#newcode18
chrome/common/child_process_info.h:18: UNKNOWN_PROCESS = 1,
On 2009/12/18 16:28:14, jar wrote:
> This change appears to re-number all entries in this list.  
> Were these entries never used before?

Their ordinal values were never used before as far as I could tell, and the
history of the file shows people adding new ones in random order (not just at
the end), which doesn't appear to have been a problem.

This reminds me though - maybe I'll add a unit test that enforces the no-delete
no-reorder policy which we now depend on.