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 164 matching lines @@
       enabled_bindings_(0),
       navigations_suspended_(false),
       has_accessed_initial_document_(false),
       is_swapped_out_(swapped_out),
       is_subframe_(false),
       main_frame_id_(-1),
       run_modal_reply_msg_(NULL),
       run_modal_opener_id_(MSG_ROUTING_NONE),
       is_waiting_for_beforeunload_ack_(false),
       is_waiting_for_unload_ack_(false),
+      should_be_preserved_on_swap_out_(false),
+      need_to_perform_clean_up_on_swapped_out_(false),
       has_timed_out_on_unload_(false),
       unload_ack_is_for_cross_site_transition_(false),
       are_javascript_messages_suppressed_(false),
       sudden_termination_allowed_(false),
       render_view_termination_status_(base::TERMINATION_STATUS_STILL_RUNNING) {
   DCHECK(instance_.get());
   CHECK(delegate_);  // http://crbug.com/82827
 
   if (main_frame_routing_id == MSG_ROUTING_NONE)
     main_frame_routing_id = GetProcess()->GetNextRoutingID();
@@ skipping 492 matching lines @@
     manager->OnCrossSiteResponse(this, global_request_id, is_transfer,
                                  transfer_url_chain, referrer, page_transition,
                                  frame_id, should_replace_current_entry);
   }
 }
 
 void RenderViewHostImpl::SuppressDialogsUntilSwapOut() {
   Send(new ViewMsg_SuppressDialogsUntilSwapOut(GetRoutingID()));
 }
 
 void RenderViewHostImpl::SwapOut() {

[Charlie Reis, 2013/12/09 20:12:29]

I don't think we need most of this method, and we can get rid of OnSwapOutACK.

Seems like this method should just send the ViewMsg_SwapOut message if
IsRenderViewLive(), then call delegate_->SwappedOut.  All the timer stuff can go
away.

   // 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.
+  // Start a timer to kill the renderer if the unload event takes too long to
+  // execute in the background.
+  // http://crbug.com/323528.
   increment_in_flight_event_count();
   StartHangMonitorTimeout(TimeDelta::FromMilliseconds(kUnloadTimeoutMS));
 
+  // If the RenderViewHost doesn't have a live renderer, the unload event is
+  // skipped. Otherwise, the RenderViewHost will remain alive until the unload
+  // event has finished executing in the background (or timed out).
+  bool preserve_render_view_host = false;
+  bool timed_out = true;
   if (IsRenderViewLive()) {
     Send(new ViewMsg_SwapOut(GetRoutingID()));
-  } else {
-    // This RenderViewHost doesn't have a live renderer, so just skip the unload
-    // event.
-    OnSwappedOut(true);
+    preserve_render_view_host = true;
+    timed_out = false;
   }
+  OnSwappedOut(timed_out, preserve_render_view_host);
 }
 
 void RenderViewHostImpl::OnSwapOutACK() {
-  OnSwappedOut(false);
+  OnSwappedOut(false, false);
 }
 
-void RenderViewHostImpl::OnSwappedOut(bool timed_out) {
-  // Stop the hang monitor now that the unload handler has finished.
-  decrement_in_flight_event_count();
-  StopHangMonitorTimeout();
-  is_waiting_for_unload_ack_ = false;
-  has_timed_out_on_unload_ = timed_out;
+void RenderViewHostImpl::OnSwappedOut(bool timed_out,
+                                      bool preserve_render_view_host) {
+
+  should_be_preserved_on_swap_out_ = preserve_render_view_host;
+  if (!preserve_render_view_host) {
+    // Stop the hang monitor now that the unload handler has finished.
+    decrement_in_flight_event_count();
+    StopHangMonitorTimeout();
+    is_waiting_for_unload_ack_ = false;
+    has_timed_out_on_unload_ = timed_out;
+  }
   delegate_->SwappedOut(this);
 }
 
 void RenderViewHostImpl::WasSwappedOut() {
+  // 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);
+
+  // The RenderView should not exit yet. It still has to execute the unload

[Charlie Reis, 2013/12/09 20:12:29]

This doesn't make sense to me.  The renderer won't try to process the
WasSwappedOut message until it finishes processing the SwapOut message anyway. 
There's no reason to avoid sending this.

+  // event.
+  if (should_be_preserved_on_swap_out_) {
+    need_to_perform_clean_up_on_swapped_out_ = true;
+    return;
+  }
+  need_to_perform_clean_up_on_swapped_out_ = false;

[pasko, 2013/12/10 19:59:29]

when is it the case that (should_be_preserved_on_swap_out_ !=
need_to_perform_clean_up_on_swapped_out_)?

+
   // 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, consider it eligible to be killed, 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 active widget hosts for the process, which
     // is equivalent to views using the process as of this writing.
     scoped_ptr<RenderWidgetHostIterator> widgets(
@@ skipping 1539 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