Histogram times surrounding render crashes

Histogram times surrounding render crashes

Report exec time, kernal time, and user cpu time
when we have a crash.  Also report non-crashing 
durations for comparison.

r=rvargas
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=128369

[jar (doing other things), 2012-03-21 18:43:18]

As per our discussion, the function call to the OS only seems to "work" on
non-crashed processes.

You can at least comment on this portion of the CL, or I can show the stack
trace above this (notification) call to try to justify that the handle has not
yet been closed.

[rvargas (doing something else), 2012-03-21 22:54:40]

I don't see an obvious problem with this patch... there is a small issue in that
RenderProcessHostImpl::OnChannelError may set the status to
TERMINATION_STATUS_NORMAL_TERMINATION what we are really crashing, but that
shouldn't affect the handle.

BTW, I just tried getting the times at RenderProcessHostImpl::OnChannelError and
I don't see any failure when the process is killed. Probably I'm not testing it
right.

http://codereview.chromium.org/9769011/diff/6001/chrome/browser/metrics/metri...
File chrome/browser/metrics/metrics_service.cc (right):

http://codereview.chromium.org/9769011/diff/6001/chrome/browser/metrics/metri...
chrome/browser/metrics/metrics_service.cc:1372: static base::TimeDelta
FromWinFileTime(FILETIME filetime) {
use Time::FromFileTime() instead

[jar (doing other things), 2012-03-22 01:45:34]

I moved the snatching of stats to in front of the notification.  I'm testing
now.  PTAL

This would probably land on the M18 branch to help gather data.

[rvargas (doing something else), 2012-03-22 01:59:32]

LGTM after the requested fixes.

http://codereview.chromium.org/9769011/diff/15001/chrome/browser/metrics/metr...
File chrome/browser/metrics/metrics_service.cc (right):

http://codereview.chromium.org/9769011/diff/15001/chrome/browser/metrics/metr...
chrome/browser/metrics/metrics_service.cc:1370: void
MetricsService::LogRendererCrash(content::RenderProcessHost* host,
same here

http://codereview.chromium.org/9769011/diff/15001/chrome/browser/metrics/metr...
File chrome/browser/metrics/metrics_service.h (right):

http://codereview.chromium.org/9769011/diff/15001/chrome/browser/metrics/metr...
chrome/browser/metrics/metrics_service.h:274: void
LogRendererCrash(content::RenderProcessHost* host,
nit: this argument should now go on the next line.

http://codereview.chromium.org/9769011/diff/15001/content/public/browser/rend...
File content/public/browser/render_process_host.h (right):

http://codereview.chromium.org/9769011/diff/15001/content/public/browser/rend...
content/public/browser/render_process_host.h:71: have_process_times = true;
We should not set this flag to true (or read the times) if GetProcessTimes
fails.

[jar (doing other things), 2012-03-22 02:06:44]

Changes made per comments by rvargas.

http://codereview.chromium.org/9769011/diff/15001/chrome/browser/metrics/metr...
File chrome/browser/metrics/metrics_service.cc (right):

http://codereview.chromium.org/9769011/diff/15001/chrome/browser/metrics/metr...
chrome/browser/metrics/metrics_service.cc:1370: void
MetricsService::LogRendererCrash(content::RenderProcessHost* host,
On 2012/03/22 01:59:32, rvargas wrote:
> same here

Done.

http://codereview.chromium.org/9769011/diff/15001/chrome/browser/metrics/metr...
File chrome/browser/metrics/metrics_service.h (right):

http://codereview.chromium.org/9769011/diff/15001/chrome/browser/metrics/metr...
chrome/browser/metrics/metrics_service.h:274: void
LogRendererCrash(content::RenderProcessHost* host,
On 2012/03/22 01:59:32, rvargas wrote:
> nit: this argument should now go on the next line.

Done.

http://codereview.chromium.org/9769011/diff/15001/content/public/browser/rend...
File content/public/browser/render_process_host.h (right):

http://codereview.chromium.org/9769011/diff/15001/content/public/browser/rend...
content/public/browser/render_process_host.h:71: have_process_times = true;
On 2012/03/22 01:59:32, rvargas wrote:
> We should not set this flag to true (or read the times) if GetProcessTimes
> fails.

<doh!>  I left out my planned early return.

Added now.

[rvargas (doing something else), 2012-03-22 02:33:40]

LGTM

[jar (doing other things), 2012-03-22 21:20:22]

I pushed the creation of the details object one level higher in the stack.  PTAL

[rvargas (doing something else), 2012-03-22 22:16:30]

Patch set 9 LGTM after the few nits.

https://chromiumcodereview.appspot.com/9769011/diff/29002/content/browser/ren...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://chromiumcodereview.appspot.com/9769011/diff/29002/content/browser/ren...
content/browser/renderer_host/render_process_host_impl.cc:785:
base::ProcessHandle handle = GetHandle();
nit: most of the time we don't keep an extra local variable for stuff like this.
I don't mind it that much, and it was already there... just saying.

https://chromiumcodereview.appspot.com/9769011/diff/29002/content/browser/ren...
content/browser/renderer_host/render_process_host_impl.cc:787: details.status =
base::TERMINATION_STATUS_NORMAL_TERMINATION;
these are the default values

https://chromiumcodereview.appspot.com/9769011/diff/29002/content/browser/ren...
content/browser/renderer_host/render_process_host_impl.cc:954:
base::ProcessHandle handle = GetHandle();
I'd also remove this one. In fact, can we add a TODO somewhere about removing
the handle from details? If the next line closes the handle, we should not keep
a stale value around.

https://chromiumcodereview.appspot.com/9769011/diff/29002/content/public/brow...
File content/public/browser/render_process_host.h (right):

https://chromiumcodereview.appspot.com/9769011/diff/29002/content/public/brow...
content/public/browser/render_process_host.h:57: &win_kernel_time,
&win_user_time)) {
nit: indent under handle

[jar (doing other things), 2012-03-22 22:42:57]

Changes made per comments by rvargas.

https://chromiumcodereview.appspot.com/9769011/diff/29002/content/browser/ren...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://chromiumcodereview.appspot.com/9769011/diff/29002/content/browser/ren...
content/browser/renderer_host/render_process_host_impl.cc:785:
base::ProcessHandle handle = GetHandle();
On 2012/03/22 22:16:30, rvargas wrote:
> nit: most of the time we don't keep an extra local variable for stuff like
this.
> I don't mind it that much, and it was already there... just saying.

Done.

https://chromiumcodereview.appspot.com/9769011/diff/29002/content/browser/ren...
content/browser/renderer_host/render_process_host_impl.cc:787: details.status =
base::TERMINATION_STATUS_NORMAL_TERMINATION;
On 2012/03/22 22:16:30, rvargas wrote:
> these are the default values

Changed to a DCHECK to help with readability (not have to look at class
defaults).

https://chromiumcodereview.appspot.com/9769011/diff/29002/content/browser/ren...
content/browser/renderer_host/render_process_host_impl.cc:954:
base::ProcessHandle handle = GetHandle();
On 2012/03/22 22:16:30, rvargas wrote:
> I'd also remove this one. In fact, can we add a TODO somewhere about removing
> the handle from details? If the next line closes the handle, we should not
keep
> a stale value around.

Done.

https://chromiumcodereview.appspot.com/9769011/diff/29002/content/public/brow...
File content/public/browser/render_process_host.h (right):

https://chromiumcodereview.appspot.com/9769011/diff/29002/content/public/brow...
content/public/browser/render_process_host.h:57: &win_kernel_time,
&win_user_time)) {
On 2012/03/22 22:16:30, rvargas wrote:
> nit: indent under handle

Done.

[rvargas (doing something else), 2012-03-22 22:56:59]

https://chromiumcodereview.appspot.com/9769011/diff/29010/content/browser/ren...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://chromiumcodereview.appspot.com/9769011/diff/29010/content/browser/ren...
content/browser/renderer_host/render_process_host_impl.cc:787: details.exit_code
= 0;
I see... The other two values are also set on the constructor, do you want to
dcheck them too?

I guess that's an inconvenience with the middle of the road approach of the
struct/class :(. Passing only one argument to the constructor, but wanting to
make sure that the other values are set to something that the caller wants. I'd
say either leave all things up to the caller (remove the constructor), or
"trust" the default values.

[jar (doing other things), 2012-03-23 01:13:21]

nit changes per rvargas

https://chromiumcodereview.appspot.com/9769011/diff/29010/content/browser/ren...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://chromiumcodereview.appspot.com/9769011/diff/29010/content/browser/ren...
content/browser/renderer_host/render_process_host_impl.cc:787: details.exit_code
= 0;
For completeness, I always like to initialize all values in a constructor.  For
the other paths, these values are set explicitly (and not even available when
the constructor is called).  I can add DCHECKS to all three... but none of this
"rendundant" code is on a critical perf path.  I wanted to the CL to be clearly
correct, so it is nicer to have the assertions here (when I don't provide
values).

I could have two constructors... both callin a common init() method.... but it
didn't seem worth the effort.

On 2012/03/22 22:56:59, rvargas wrote:
> I see... The other two values are also set on the constructor, do you want to
> dcheck them too?
> 
> I guess that's an inconvenience with the middle of the road approach of the
> struct/class :(. Passing only one argument to the constructor, but wanting to
> make sure that the other values are set to something that the caller wants.
I'd
> say either leave all things up to the caller (remove the constructor), or
> "trust" the default values.

[jam, 2012-03-23 01:21:25]

lgtm to land today and revert tomorrow :)