Histogram times surrounding render crashes

content/browser/renderer_host/render_process_host_impl.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // Represents the browser side of the browser <--> renderer communication
 // channel. There will be one RenderProcessHost per renderer process.
 
 #include "content/browser/renderer_host/render_process_host_impl.h"
 
 #if defined(OS_WIN)
@@ skipping 764 matching lines @@
 
   // Test if there's an unload listener.
   // NOTE: It's possible that an onunload listener may be installed
   // while we're shutting down, so there's a small race here.  Given that
   // the window is small, it's unlikely that the web page has much
   // state that will be lost by not calling its unload handlers properly.
   if (!SuddenTerminationAllowed())
     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;
 }
 
 void RenderProcessHostImpl::DumpHandles() {
 #if defined(OS_WIN)
   Send(new ChildProcessMsg_DumpHandles());
   return;
 #endif
 
@@ skipping 143 matching lines @@
   tracked_objects::ThreadData::Status status =
       tracked_objects::ThreadData::status();
   Send(new ChildProcessMsg_SetProfilerStatus(status));
 }
 
 void RenderProcessHostImpl::OnChannelError() {
   if (!channel_.get())
     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);
       return;
     }
   }
 #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
 }
 
 content::BrowserContext* RenderProcessHostImpl::GetBrowserContext() const {
   return browser_context_;
 }
 
 int RenderProcessHostImpl::GetID() const {
   return id_;
 }
@@ skipping 213 matching lines @@
   // Now pick a random suitable renderer, if we have any.
   if (!suitable_renderers.empty()) {
     int suitable_count = static_cast<int>(suitable_renderers.size());
     int random_index = base::RandInt(0, suitable_count - 1);
     return suitable_renderers[random_index];
   }
 
   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();
   gpu_message_filter_ = NULL;
 
   IDMap<RenderWidgetHost>::iterator iter(&render_widget_hosts_);
   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();
   }
 
   ClearTransportDIBCache();
 
   // this object is not deleted at this point and may be reused later.
   // TODO(darin): clean this up
 }
 
 void RenderProcessHostImpl::OnShutdownRequest() {
@@ skipping 84 matching lines @@
 void RenderProcessHostImpl::OnRevealFolderInOS(const FilePath& path) {
   // Only honor the request if appropriate persmissions are granted.
   if (ChildProcessSecurityPolicyImpl::GetInstance()->CanReadFile(GetID(),
                                                                  path))
     content::GetContentClient()->browser()->OpenItem(path);
 }
 
 void RenderProcessHostImpl::OnSavedPageAsMHTML(int job_id, int64 data_size) {
   MHTMLGenerationManager::GetInstance()->MHTMLGenerated(job_id, data_size);
 }