Have the unload event execute in background on cross-site navigations

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>
@@ skipping 84 matching lines @@
 using blink::WebMediaPlayerAction;
 using blink::WebPluginAction;
 
 namespace content {
 namespace {
 
 // Delay to wait on closing the WebContents for a beforeunload/unload handler to
 // fire.
 const int kUnloadTimeoutMS = 1000;
 
+// Delay to wait on closing the WebContents for an unload/handler to fire when
+// swapping renderers. The old renderer will allways be sent to the background

[Charlie Reis, 2013/11/26 19:35:00]

nit: always

As Ojan wondered, have you checked whether a long-running unload handler
actually finishes running in the background, or do we kill the process right
away?

+// since the unload handler cannot show dialogues.
+// http://crbug.com/323528.
+const int kUnloadSwapTimeoutMS = 0;
+
 // Translate a WebKit text direction into a base::i18n one.
 base::i18n::TextDirection WebTextDirectionToChromeTextDirection(
     blink::WebTextDirection dir) {
   switch (dir) {
     case blink::WebTextDirectionLeftToRight:
       return base::i18n::LEFT_TO_RIGHT;
     case blink::WebTextDirectionRightToLeft:
       return base::i18n::RIGHT_TO_LEFT;
     default:
       NOTREACHED();
@@ skipping 572 matching lines @@
 }
 
 void RenderViewHostImpl::SuppressDialogsUntilSwapOut() {
   Send(new ViewMsg_SuppressDialogsUntilSwapOut(GetRoutingID()));
 }
 
 void RenderViewHostImpl::SwapOut() {
   // 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.
-  increment_in_flight_event_count();
-  StartHangMonitorTimeout(TimeDelta::FromMilliseconds(kUnloadTimeoutMS));
 
   if (IsRenderViewLive()) {
     Send(new ViewMsg_SwapOut(GetRoutingID()));
+    // The hang monitor will immediately time out, which will cause the old
+    // renderer to go execute the unload event in the background. The swap out
+    // can then proceed without any delay.
+    // http://crbug.com/323528.
+    increment_in_flight_event_count();
+    StartHangMonitorTimeout(TimeDelta::FromMilliseconds(kUnloadSwapTimeoutMS));

[Charlie Reis, 2013/11/26 19:35:00]

Moving these calls inside the if seems like a problem.  For example, if the
render view isn't live, we'll still call decrement_in_flight_event_count() in
OnSwappedOut.

Also, using a timeout of 0 ms seems like it's keeping a lot of complexity around
unnecessarily.  It's great for the measurements to confirm that this will help,
but if we know we're going down this path, can we remove the use of the timer
instead?

   } else {
     // This RenderViewHost doesn't have a live renderer, so just skip the unload
     // event.
     OnSwappedOut(true);
   }
 }
 
 void RenderViewHostImpl::OnSwapOutACK() {
   OnSwappedOut(false);
 }
@@ skipping 1570 matching lines @@
 void RenderViewHostImpl::AttachToFrameTree() {
   FrameTree* frame_tree = delegate_->GetFrameTree();
 
   frame_tree->SwapMainFrame(main_render_frame_host_.get());
   if (main_frame_id() != FrameTreeNode::kInvalidFrameId) {
     frame_tree->OnFirstNavigationAfterSwap(main_frame_id());
   }
 }
 
 }  // namespace content