From 235caa47af34c2f68b26a38d46f1002e56f6cbfe Mon Sep 17 00:00:00 2001 From: "jam@chromium.org" Date: Mon, 13 Apr 2009 23:18:02 +0000 Subject: 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 --- chrome/browser/cert_store.cc | 6 +++++- chrome/browser/metrics/metrics_service.cc | 7 +++---- .../renderer_host/browser_render_process_host.cc | 18 +++++------------- chrome/browser/renderer_host/render_process_host.cc | 12 +++--------- chrome/browser/renderer_host/render_process_host.h | 4 ---- chrome/browser/tab_contents/site_instance.h | 3 +++ chrome/common/notification_type.h | 10 ++++++---- 7 files changed, 25 insertions(+), 35 deletions(-) (limited to 'chrome') 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(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(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(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(this), - Details(&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(this), - Details(&close_expected)); - notified_termination_ = true; - } + NotificationService::current()->Notify( + NotificationType::RENDERER_PROCESS_TERMINATED, + Source(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, 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. -- cgit v1.1