Don't overload the meaning of the RENDERER_PROCESS_TERMINATED notification, instead create a new one for crashing.

The old way of using notifications was incorrect since a RenderProcessHost might have sent only one notification even though a new renderer might have been created after a crash.

BUG=9379

Review URL: http://codereview.chromium.org/66069

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@13629 0039d316-1c4b-4281-b951-d872f2087c98

7 files changed, 25 insertions, 35 deletions

diff --git a/chrome/browser/cert_store.cc b/chrome/browser/cert_store.cc
index 7687a9f..84e6ab6 100644
--- a/chrome/browser/cert_store.cc
+++ b/chrome/browser/cert_store.cc
@@ -41,6 +41,9 @@ CertStore::CertStore() : next_cert_id_(1) {
   NotificationService::current()->AddObserver(this,
       NotificationType::RENDERER_PROCESS_TERMINATED,
       NotificationService::AllSources());
+  NotificationService::current()->AddObserver(this,
+      NotificationType::RENDERER_PROCESS_CRASHED,
+      NotificationService::AllSources());
 }
 
 CertStore::~CertStore() {
@@ -135,7 +138,8 @@ void CertStore::RemoveCertsForRenderProcesHost(int process_id) {
 void CertStore::Observe(NotificationType type,
                         const NotificationSource& source,
                         const NotificationDetails& details) {
-  DCHECK(type == NotificationType::RENDERER_PROCESS_TERMINATED);
+  DCHECK(type == NotificationType::RENDERER_PROCESS_TERMINATED ||
+         type == NotificationType::RENDERER_PROCESS_CRASHED);
   RenderProcessHost* rph = Source<RenderProcessHost>(source).ptr();
   DCHECK(rph);
   RemoveCertsForRenderProcesHost(rph->pid());
diff --git a/chrome/browser/metrics/metrics_service.cc b/chrome/browser/metrics/metrics_service.cc
index 2d5b502..1e447ea 100644
--- a/chrome/browser/metrics/metrics_service.cc
+++ b/chrome/browser/metrics/metrics_service.cc
@@ -507,9 +507,8 @@ void MetricsService::Observe(NotificationType type,
       LogLoadStarted();
       break;
 
-    case NotificationType::RENDERER_PROCESS_TERMINATED:
-      if (!*Details<bool>(details).ptr())
-        LogRendererCrash();
+    case NotificationType::RENDERER_PROCESS_CRASHED:
+      LogRendererCrash();
       break;
 
     case NotificationType::RENDERER_PROCESS_HANG:
@@ -792,7 +791,7 @@ void MetricsService::ListenerRegistration(bool start_listening) {
   AddOrRemoveObserver(this, NotificationType::LOAD_STOP, start_listening);
   AddOrRemoveObserver(this, NotificationType::RENDERER_PROCESS_IN_SBOX,
                       start_listening);
-  AddOrRemoveObserver(this, NotificationType::RENDERER_PROCESS_TERMINATED,
+  AddOrRemoveObserver(this, NotificationType::RENDERER_PROCESS_CRASHED,
                       start_listening);
   AddOrRemoveObserver(this, NotificationType::RENDERER_PROCESS_HANG,
                       start_listening);
diff --git a/chrome/browser/renderer_host/browser_render_process_host.cc b/chrome/browser/renderer_host/browser_render_process_host.cc
index 486f91d..882937e 100644
--- a/chrome/browser/renderer_host/browser_render_process_host.cc
+++ b/chrome/browser/renderer_host/browser_render_process_host.cc
@@ -657,24 +657,16 @@ void BrowserRenderProcessHost::OnChannelError() {
 
   DCHECK(process_.handle());
   DCHECK(channel_.get());
-  base::ProcessHandle process = process_.handle();
 
-  bool clean_shutdown = !base::DidProcessCrash(process);
+  if (base::DidProcessCrash(process_.handle())) {
+    NotificationService::current()->Notify(
+        NotificationType::RENDERER_PROCESS_CRASHED,
+        Source<RenderProcessHost>(this), NotificationService::NoDetails());
+  }
 
   process_.Close();
-
   channel_.reset();
 
-  if (!notified_termination_) {
-    // If |close_expected| is false, it means the renderer process went away
-    // before the web views expected it; count it as a crash.
-    NotificationService::current()->Notify(
-        NotificationType::RENDERER_PROCESS_TERMINATED,
-        Source<RenderProcessHost>(this),
-        Details<bool>(&clean_shutdown));
-    notified_termination_ = true;
-  }
-
   // This process should detach all the listeners, causing the object to be
   // deleted. We therefore need a stack copy of the web view list to avoid
   // crashing when checking for the termination condition the last time.
diff --git a/chrome/browser/renderer_host/render_process_host.cc b/chrome/browser/renderer_host/render_process_host.cc
index 8cafe1e..0fdde82 100644
--- a/chrome/browser/renderer_host/render_process_host.cc
+++ b/chrome/browser/renderer_host/render_process_host.cc
@@ -65,7 +65,6 @@ bool RenderProcessHost::run_renderer_in_process_ = false;
 
 RenderProcessHost::RenderProcessHost(Profile* profile)
     : max_page_id_(-1),
-      notified_termination_(false),
       pid_(-1),
       profile_(profile) {
 }
@@ -87,14 +86,9 @@ void RenderProcessHost::Release(int listener_id) {
 
   // When no other owners of this object, we can delete ourselves
   if (listeners_.IsEmpty()) {
-    if (!notified_termination_) {
-      bool close_expected = true;
-      NotificationService::current()->Notify(
-          NotificationType::RENDERER_PROCESS_TERMINATED,
-          Source<RenderProcessHost>(this),
-          Details<bool>(&close_expected));
-      notified_termination_ = true;
-    }
+    NotificationService::current()->Notify(
+        NotificationType::RENDERER_PROCESS_TERMINATED,
+        Source<RenderProcessHost>(this), NotificationService::NoDetails());
     if (pid_ >= 0) {
       all_hosts.Remove(pid_);
       pid_ = -1;
diff --git a/chrome/browser/renderer_host/render_process_host.h b/chrome/browser/renderer_host/render_process_host.h
index 6e59166..f972d39 100644
--- a/chrome/browser/renderer_host/render_process_host.h
+++ b/chrome/browser/renderer_host/render_process_host.h
@@ -188,10 +188,6 @@ class RenderProcessHost : public IPC::Channel::Sender,
   // The maximum page ID we've ever seen from the renderer process.
   int32 max_page_id_;
 
-  // Indicates whether we have notified that the process has terminated. We
-  // only want to send this out once.
-  bool notified_termination_;
-
  private:
   int pid_;
 
diff --git a/chrome/browser/tab_contents/site_instance.h b/chrome/browser/tab_contents/site_instance.h
index 34cef93..2e6a51b 100644
--- a/chrome/browser/tab_contents/site_instance.h
+++ b/chrome/browser/tab_contents/site_instance.h
@@ -145,6 +145,9 @@ class SiteInstance : public base::RefCounted<SiteInstance>,
   const RenderProcessHostFactory* render_process_host_factory_;
 
   // Current RenderProcessHost that is rendering pages for this SiteInstance.
+  // This pointer will only change once the RenderProcessHost is destructed.  It
+  // will still remain the same even if the process crashes, since in that
+  // scenario the RenderProcessHost remains the same.
   RenderProcessHost* process_;
 
   // The current max_page_id in the SiteInstance's RenderProcessHost.  If the
diff --git a/chrome/common/notification_type.h b/chrome/common/notification_type.h
index 942c072..e2b9da2 100644
--- a/chrome/common/notification_type.h
+++ b/chrome/common/notification_type.h
@@ -272,12 +272,14 @@ class NotificationType {
     // details are expected.
     CWINDOW_CLOSED,
 
-    // Indicates that a render process has terminated. The source will be the
-    // RenderProcessHost that corresponds to the process, and the details is a
-    // bool specifying whether the termination was expected, i.e. if false it
-    // means the process crashed.
+    // Indicates that a RenderProcessHost is destructing. The source will be the
+    // RenderProcessHost that corresponds to the process.
     RENDERER_PROCESS_TERMINATED,
 
+    // Indicates that a render process has crashed.  The source will be the
+    // corresponding RenderProcessHost.
+    RENDERER_PROCESS_CRASHED,
+
     // Indicates that a render process has become unresponsive for a period of
     // time. The source will be the RenderWidgetHost that corresponds to the
     // hung view, and no details are expected.