Fixing a problem, where a hung renderer process is not killed when navigating away

content/browser/renderer_host/render_view_host.cc

Index: content/browser/renderer_host/render_view_host.cc
diff --git a/content/browser/renderer_host/render_view_host.cc b/content/browser/renderer_host/render_view_host.cc
index e193dc2e71f586ccbb9a6cb3ec494e4606e7833a..c5d20af3945be78d210c3d86988f421e2cd6e4d2 100644
--- a/content/browser/renderer_host/render_view_host.cc
+++ b/content/browser/renderer_host/render_view_host.cc
@@ -41,6 +41,7 @@
 #include "content/public/browser/notification_details.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/notification_types.h"
+#include "content/public/browser/render_process_host.h"
 #include "content/public/browser/render_view_host_delegate.h"
 #include "content/public/browser/render_view_host_observer.h"
 #include "content/public/browser/user_metrics.h"
@@ -380,6 +381,12 @@ void RenderViewHost::WasSwappedOut() {
   // Don't bother reporting hung state anymore.
   StopHangMonitorTimeout();
 
+  // If we are still waiting on the unload handler to be run, we consider
+  // the process hung and we should terminate it if there are no other tabs
+  // using the process. If there are other views using this process, the
+  // unresponsive renderer timeout will catch it.
+  bool hung = is_waiting_for_unload_ack_;
+
   // Now that we're no longer the active RVH in the tab, start filtering out
   // most IPC messages.  Usually the renderer will have stopped sending
   // messages as of OnSwapOutACK.  However, we may have timed out waiting
@@ -388,6 +395,29 @@ void RenderViewHost::WasSwappedOut() {
   // still allow synchronous messages through).
   SetSwappedOut(true);
 
+  // If we are not running the renderer in process and no other tab is using
+  // the hung process, kill it, assuming it is a real process (unit tests don't
+  // have real processes).
+  if (hung) {
+    base::ProcessHandle process_handle = process_->GetHandle();
+    if (!content::RenderProcessHost::run_renderer_in_process() &&
+        process_->TerminateOnUnresponsiveAllowed() && process_handle) {
+      // We expect the delegate for this RVH to be TabContents, as it is the
+      // only class that swaps render view hosts on navigation.

[Charlie Reis, 2012/03/02 00:16:43]

Nit: swaps out

[nasko, 2012/03/02 18:40:36]

Done.

+      DCHECK(delegate_->GetRenderViewType() == content::VIEW_TYPE_TAB_CONTENTS);
+
+      // If we load data URLs, such as about:blank, there will be no network

[Charlie Reis, 2012/03/02 00:16:43]

nit: data URLs or about:blank

[nasko, 2012/03/02 18:40:36]

Done.

+      // request and we arrive here without timeout. Since the TabContents

[Charlie Reis, 2012/03/02 00:16:43]

we arrive here without setting an unresponsiveness timer.

[nasko, 2012/03/02 18:40:36]

Done.

+      // sets the sudden termination allowed on real timeout, respect it and

[Charlie Reis, 2012/03/02 00:16:43]

nit: sets SuddenTerminationAllowed in the case that a timer is set.

[nasko, 2012/03/02 18:40:36]

Done.

+      // don't kill the process. This allows a corner case where a navigation

[Charlie Reis, 2012/03/02 00:16:43]

I think the problem I'm having is that this feels out of order.  We should
probably describe the common case first-- only kill the process if TabContents
sets SuddenTerminationAllowed, since that indicates the timer expired.

Then explain the corner case (data/blank URLs that don't hit that logic) and why
we chose not to deal with it.

[nasko, 2012/03/02 18:40:36]

Rewrote the comment, hopefully it is better.

[Charlie Reis, 2012/03/02 19:17:02]

Much better, thanks!

+      // to a data URL will leave a process running, if the beforeunload handler
+      // completes fine, but the unload handler hangs. At this time, the
+      // complexity to solve this edge case is not worthwhile.
+      if (SuddenTerminationAllowed())
+        base::KillProcess(process_handle, content::RESULT_CODE_HUNG, false);
+    }
+  }
+
   // Inform the renderer that it can exit if no one else is using it.
   Send(new ViewMsg_WasSwappedOut(routing_id()));
 }