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

content/browser/renderer_host/render_view_host_impl.cc

Index: content/browser/renderer_host/render_view_host_impl.cc
diff --git a/content/browser/renderer_host/render_view_host_impl.cc b/content/browser/renderer_host/render_view_host_impl.cc
index e31de0f182530c5e9c400978464681233c61a479..3477bedd637f11de2c2e3340451d968ff13129a3 100644
--- a/content/browser/renderer_host/render_view_host_impl.cc
+++ b/content/browser/renderer_host/render_view_host_impl.cc
@@ -13,6 +13,7 @@
 #include "base/i18n/rtl.h"
 #include "base/json/json_reader.h"
 #include "base/message_loop.h"
+#include "base/metrics/histogram.h"
 #include "base/stl_util.h"
 #include "base/string_util.h"
 #include "base/time.h"
@@ -384,6 +385,9 @@ void RenderViewHostImpl::FirePageBeforeUnload(bool for_cross_site_transition) {
     // handler.
     is_waiting_for_beforeunload_ack_ = true;
     unload_ack_is_for_cross_site_transition_ = for_cross_site_transition;
+    // Increment the in-flight event count, to ensure that input events won't
+    // cancel the timeout timer.
+    IncrementInFlightEventCount();
     StartHangMonitorTimeout(TimeDelta::FromMilliseconds(kUnloadTimeoutMS));
     send_should_close_start_time_ = base::TimeTicks::Now();
     Send(new ViewMsg_ShouldClose(GetRoutingID()));
@@ -396,6 +400,9 @@ void RenderViewHostImpl::SwapOut(int new_render_process_host_id,
   // this RVH with the pending RVH.
   is_waiting_for_unload_ack_ = true;
   // Start the hang monitor in case the renderer hangs in the unload handler.
+  // Increment the in-flight event count, to ensure that input events won't
+  // cancel the timeout timer.
+  IncrementInFlightEventCount();
   StartHangMonitorTimeout(TimeDelta::FromMilliseconds(kUnloadTimeoutMS));
 
   ViewMsg_SwapOut_Params params;
@@ -409,14 +416,16 @@ void RenderViewHostImpl::SwapOut(int new_render_process_host_id,
     // This RenderViewHost doesn't have a live renderer, so just skip the unload
     // event.  We must notify the ResourceDispatcherHost on the IO thread,
     // which we will do through the RenderProcessHost's widget helper.
-    GetProcess()->CrossSiteSwapOutACK(params);
+    GetProcess()->SimulateSwapOutACK(params);
   }
 }
 
-void RenderViewHostImpl::OnSwapOutACK() {
+void RenderViewHostImpl::OnSwapOutACK(bool timed_out) {
   // Stop the hang monitor now that the unload handler has finished.
+  DecrementInFlightEventCount();
   StopHangMonitorTimeout();
   is_waiting_for_unload_ack_ = false;
+  has_timed_out_on_unload_ = timed_out;
   delegate_->SwappedOut(this);
 }
 
@@ -424,6 +433,12 @@ void RenderViewHostImpl::WasSwappedOut() {
   // Don't bother reporting hung state anymore.
   StopHangMonitorTimeout();
 
+  // If we have timed out on running the unload handler, 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 = has_timed_out_on_unload_;
+
   // 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
@@ -432,6 +447,45 @@ void RenderViewHostImpl::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 = GetProcess()->GetHandle();
+    int views = 0;
+
+    // Count the number of listeners for the process, which is equivalent to
+    // views using the process as of this writing.

[Charlie Reis, 2012/04/06 22:34:14]

We can update this comment now that the API has changed from listeners to
widgets.

[nasko, 2012/04/10 00:16:37]

Done.

+    content::RenderProcessHost::RenderWidgetHostsIterator iter(
+        GetProcess()->GetRenderWidgetHostsIterator());
+    for (; !iter.IsAtEnd(); iter.Advance())
+      ++views;
+
+    if (!content::RenderProcessHost::run_renderer_in_process() &&
+        process_handle && views <= 1) {
+      // We expect the delegate for this RVH to be TabContents, as it is the
+      // only class that swaps out render view hosts on navigation.
+      DCHECK(delegate_->GetRenderViewType() == content::VIEW_TYPE_TAB_CONTENTS);

[Charlie Reis, 2012/04/06 22:34:14]

nit: Let's use DCHECK_EQ, which prints better error messages.

[nasko, 2012/04/10 00:16:37]

Done.

+
+      // Kill the process only if TabContents sets SuddenTerminationAllowed,
+      // which indicates that the timer has expired.
+      // This is not the case if we load data URLs or about:blank. The reason
+      // is that there is no network requests and this code is hit without
+      // setting the unresponsiveness timer. This allows a corner case where a
+      // navigation 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);
+        // Log a histogram point to help us diagnose how many of those kills
+        // we have performed. 1 is the enum value for RendererType Normal for
+        // the histogram.

[Charlie Reis, 2012/04/06 22:34:14]

What are the other RendererTypes?  It's possible to be one of the other types,
right?

[nasko, 2012/04/10 00:16:37]

No, it is an enum of Renderer and Extension. We don't kill extensions in this
code path, so we are safe with just this value. Also, the enum is defined in
histogram.xml and there is no symbolic constant to use, hence I put the actual
comment.

[Charlie Reis, 2012/04/10 01:19:35]

Actually, I think it would be possible to kill extensions with this.  For
example, an options page for an extension is loaded as a web page running in the
extension process.

[nasko, 2012/04/10 14:31:26]

Considering this code is in the content module, which is not aware of
extensions, do we even have this information here? If not, might be better to
just not log the type.

[Charlie Reis, 2012/04/10 17:22:04]

Yeah, this doesn't matter enough to plumb it through.  It won't really give us
any more useful data in the histogram, since extensions hanging in unload
handlers aren't qualitatively different.  No worries.

+        UMA_HISTOGRAM_PERCENTAGE(
+            "BrowserRenderProcessHost.ChildKillsUnresponsive", 1);
+      }
+    }
+  }
+
   // Inform the renderer that it can exit if no one else is using it.
   Send(new ViewMsg_WasSwappedOut(GetRoutingID()));
 }
@@ -622,7 +676,8 @@ void RenderViewHostImpl::JavaScriptDialogClosed(IPC::Message* reply_msg,
   bool is_waiting =
       is_waiting_for_beforeunload_ack_ || is_waiting_for_unload_ack_;
   if (is_waiting)
-    StartHangMonitorTimeout(TimeDelta::FromMilliseconds(kUnloadTimeoutMS));
+    StartHangMonitorTimeout(TimeDelta::FromMilliseconds(
+        success ? kUnloadTimeoutMS : hung_renderer_delay_ms_));

[Charlie Reis, 2012/04/06 22:34:14]

A comment explaining why we use the short vs the long delay would help.

Also, do we want to increment the in-flight event count in this case as well? 
We're still expecting to hear the ShouldCloseACK.

[nasko, 2012/04/10 00:16:37]

Yes, we are still expecting the ACK, but the count is still there. If we
increment, then we have to decrement first in the OnMsgRunBeforeUnloadConfirm
function, where we stop the timer. Technically, we do have an outstanding ACK
even in-flight, but we have an interrupt with JavaScript prompt.

 
   ViewHostMsg_RunJavaScriptMessage::WriteReplyParams(reply_msg,
                                                      success, user_input);
@@ -1310,6 +1365,7 @@ void RenderViewHostImpl::OnMsgShouldCloseACK(
     bool proceed,
     const base::TimeTicks& renderer_before_unload_start_time,
     const base::TimeTicks& renderer_before_unload_end_time) {
+  DecrementInFlightEventCount();
   StopHangMonitorTimeout();
   // If this renderer navigated while the beforeunload request was in flight, we
   // may have cleared this state in OnMsgNavigate, in which case we can ignore
@@ -1726,6 +1782,8 @@ void RenderViewHostImpl::SetSwappedOut(bool is_swapped_out) {
   // can cause navigations to be ignored in OnMsgNavigate.
   is_waiting_for_beforeunload_ack_ = false;
   is_waiting_for_unload_ack_ = false;
+

[Charlie Reis, 2012/04/06 22:34:14]

nit: No whitespace needed here.  This falls in the same category.

[nasko, 2012/04/10 00:16:37]

Done.

+  has_timed_out_on_unload_ = false;
 }
 
 void RenderViewHostImpl::ClearPowerSaveBlockers() {