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

content/browser/renderer_host/render_view_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.
 
 #include "content/browser/renderer_host/render_view_host_impl.h"
 
 #include <set>
 #include <string>
 #include <utility>
 #include <vector>
 
 #include "base/command_line.h"
 #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"
 #include "base/utf_string_conversions.h"
 #include "base/values.h"
 #include "content/browser/child_process_security_policy_impl.h"
 #include "content/browser/cross_site_request_manager.h"
 #include "content/browser/gpu/gpu_surface_tracker.h"
 #include "content/browser/host_zoom_map_impl.h"
 #include "content/browser/in_process_webkit/dom_storage_context_impl.h"
@@ skipping 351 matching lines @@
     // (if there was a cross-site "close" request pending when the user clicked
     // the close button). We want to keep the "for cross site" flag only if
     // both the old and the new ones are also for cross site.
     unload_ack_is_for_cross_site_transition_ =
         unload_ack_is_for_cross_site_transition_ && for_cross_site_transition;
   } else {
     // Start the hang monitor in case the renderer hangs in the beforeunload
     // 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()));
   }
 }
 
 void RenderViewHostImpl::SwapOut(int new_render_process_host_id,
                                  int new_request_id) {
   // This will be set back to false in OnSwapOutACK, just before we replace
   // 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;
   params.closing_process_id = GetProcess()->GetID();
   params.closing_route_id = GetRoutingID();
   params.new_render_process_host_id = new_render_process_host_id;
   params.new_request_id = new_request_id;
   if (IsRenderViewLive()) {
     Send(new ViewMsg_SwapOut(GetRoutingID(), params));
   } else {
     // 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);
 }
 
 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
   // for that message, and additional IPC messages may keep streaming in.
   // We filter them out, as long as that won't cause problems (e.g., we
   // 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()));
 }
 
 void RenderViewHostImpl::ClosePage() {
   // Start the hang monitor in case the renderer hangs in the unload handler.
   is_waiting_for_unload_ack_ = true;
   StartHangMonitorTimeout(TimeDelta::FromMilliseconds(kUnloadTimeoutMS));

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

We'll need to increment the in-flight event count here, too, right?

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

Yes. Good catch!

 
   if (IsRenderViewLive()) {
     // TODO(creis): Should this be moved to Shutdown?  It may not be called for
     // RenderViewHosts that have been swapped out.
     content::NotificationService::current()->Notify(
         content::NOTIFICATION_RENDER_VIEW_HOST_WILL_CLOSE_RENDER_VIEW,
         content::Source<RenderViewHost>(this),
         content::NotificationService::NoDetails());
 
     Send(new ViewMsg_ClosePage(GetRoutingID()));
@@ skipping 162 matching lines @@
   return observer.value()->DeepCopy();
 }
 
 void RenderViewHostImpl::JavaScriptDialogClosed(IPC::Message* reply_msg,
                                                 bool success,
                                                 const string16& user_input) {
   GetProcess()->SetIgnoreInputEvents(false);
   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);
   Send(reply_msg);
 
   // If we are waiting for an unload or beforeunload ack and the user has
   // suppressed messages, kill the tab immediately; a page that's spamming
   // alerts in onbeforeunload is presumably malicious, so there's no point in
   // continuing to run its script and dragging out the process.
   // This must be done after sending the reply since RenderView can't close
@@ skipping 667 matching lines @@
 }
 
 void RenderViewHostImpl::OnUserGesture() {
   delegate_->OnUserGesture();
 }
 
 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
   // this message.
   if (!is_waiting_for_beforeunload_ack_ || is_swapped_out_)
     return;
 
   is_waiting_for_beforeunload_ack_ = false;
 
   RenderViewHostDelegate::RendererManagement* management_delegate =
@@ skipping 396 matching lines @@
 }
 
 void RenderViewHostImpl::SetSwappedOut(bool is_swapped_out) {
   is_swapped_out_ = is_swapped_out;
 
   // Whenever we change swap out state, we should not be waiting for
   // beforeunload or unload acks.  We clear them here to be safe, since they
   // 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() {
   STLDeleteValues(&power_save_blockers_);
 }
 
 }  // namespace content