Histogram times surrounding render crashes

content/browser/renderer_host/render_process_host_impl.cc

Index: content/browser/renderer_host/render_process_host_impl.cc
===================================================================
--- content/browser/renderer_host/render_process_host_impl.cc	(revision 127735)
+++ content/browser/renderer_host/render_process_host_impl.cc	(working copy)
@@ -782,8 +782,11 @@
     return false;
 
   // Store the handle before it gets changed.
-  base::ProcessHandle handle = GetHandle();
-  ProcessDied(handle, base::TERMINATION_STATUS_NORMAL_TERMINATION, 0, false);
+  RendererClosedDetails details(GetHandle());
+  DCHECK_EQ(details.status, base::TERMINATION_STATUS_NORMAL_TERMINATION);
+  details.exit_code = 0;

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

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]

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:

+  details.was_alive = false;
+  ProcessDied(&details);
   fast_shutdown_started_ = true;
   return true;
 }
@@ -947,19 +950,16 @@
     return;
 
   // Store the handle before it gets changed.
-  base::ProcessHandle handle = GetHandle();
-
+  RendererClosedDetails details(GetHandle());
   // child_process_launcher_ can be NULL in single process mode or if fast
   // termination happened.
-  int exit_code = 0;
-  base::TerminationStatus status =
-      child_process_launcher_.get() ?
-      child_process_launcher_->GetChildTerminationStatus(&exit_code) :
+  details.status = child_process_launcher_.get() ?
+      child_process_launcher_->GetChildTerminationStatus(&details.exit_code) :
       base::TERMINATION_STATUS_NORMAL_TERMINATION;
 
 #if defined(OS_WIN)
   if (!run_renderer_in_process()) {
-    if (status == base::TERMINATION_STATUS_STILL_RUNNING) {
+    if (details.status == base::TERMINATION_STATUS_STILL_RUNNING) {
       HANDLE process = child_process_launcher_->GetHandle();
       child_process_watcher_.StartWatching(
           new base::WaitableEvent(process), this);
@@ -967,19 +967,20 @@
     }
   }
 #endif
-  ProcessDied(handle, status, exit_code, false);
-}
+  details.was_alive = false;
+  ProcessDied(&details);
+  }
 
 // Called when the renderer process handle has been signaled.
 void RenderProcessHostImpl::OnWaitableEventSignaled(
     base::WaitableEvent* waitable_event) {
 #if defined (OS_WIN)
-  base::ProcessHandle handle = GetHandle();
-  int exit_code = 0;
-  base::TerminationStatus status =
-      base::GetTerminationStatus(waitable_event->Release(), &exit_code);
+  RendererClosedDetails details(GetHandle());
+  details.status = base::GetTerminationStatus(waitable_event->Release(),
+                                              &details.exit_code);
   delete waitable_event;
-  ProcessDied(handle, status, exit_code, true);
+  details.was_alive = true;
+  ProcessDied(&details);
 #endif
 }
 
@@ -1213,21 +1214,17 @@
   return NULL;
 }
 
-void RenderProcessHostImpl::ProcessDied(base::ProcessHandle handle,
-                                           base::TerminationStatus status,
-                                           int exit_code,
-                                           bool was_alive) {
+void RenderProcessHostImpl::ProcessDied(RendererClosedDetails* details) {
   // Our child process has died.  If we didn't expect it, it's a crash.
   // In any case, we need to let everyone know it's gone.
   // The OnChannelError notification can fire multiple times due to nested sync
   // calls to a renderer. If we don't have a valid channel here it means we
   // already handled the error.
 
-  RendererClosedDetails details(handle, status, exit_code, was_alive);
   content::NotificationService::current()->Notify(
       content::NOTIFICATION_RENDERER_PROCESS_CLOSED,
       content::Source<RenderProcessHost>(this),
-      content::Details<RendererClosedDetails>(&details));
+      content::Details<RendererClosedDetails>(details));
 
   child_process_launcher_.reset();
   channel_.reset();
@@ -1237,8 +1234,8 @@
   while (!iter.IsAtEnd()) {
     RenderWidgetHostImpl::From(iter.GetCurrentValue())->OnMessageReceived(
         ViewHostMsg_RenderViewGone(iter.GetCurrentKey(),
-                                   static_cast<int>(status),
-                                   exit_code));
+                                   static_cast<int>(details->status),
+                                   details->exit_code));
     iter.Advance();
   }