Eliminate SwapOut message parameters, keeping state in RVHM instead.

content/browser/web_contents/render_view_host_manager.cc

Index: content/browser/web_contents/render_view_host_manager.cc
diff --git a/content/browser/web_contents/render_view_host_manager.cc b/content/browser/web_contents/render_view_host_manager.cc
index 3ef63d4a6fed60c3bfe784434b87d592905bed39..9ce3db7f00556e661ca9a826aa97cc4bdaa8a13b 100644
--- a/content/browser/web_contents/render_view_host_manager.cc
+++ b/content/browser/web_contents/render_view_host_manager.cc
@@ -189,8 +189,9 @@ bool RenderViewHostManager::ShouldCloseTabOnUnresponsiveRenderer() {
   // If the tab becomes unresponsive during unload while doing a
   // cross-site navigation, proceed with the navigation.  (This assumes that
   // the pending RenderViewHost is still responsive.)
-  int pending_request_id = pending_render_view_host_->GetPendingRequestId();
-  if (pending_request_id == -1) {
+  bool waiting_for_beforeunload =
+      render_view_host_->is_waiting_for_beforeunload_ack();

[nasko, 2013/06/05 18:17:07]

Why is this checking for beforeunload handler, when the else statement is
pretending the unload handler finished.

[Charlie Reis, 2013/06/05 23:03:34]

Hmm, this old logic may have been buggy.  It assumed that during a cross-process
navigation, we're either waiting for a beforeunload ack or an unload ack.  But
there are periods of time where we're waiting for neither (e.g., after sending
the navigation request but before it gets to OnResponseStarted, and after the
SwapOutACK but before the commit).

Nice catch.  I've updated it to check for is_waiting_for_unload_ack().

I also reversed the order of the checks, because
is_waiting_for_beforeunload_ack() doesn't go back to false.  This was a
regression, but it wasn't caught because the
UnloadTest.CrossSiteInfiniteBeforeUnload* tests were disabled.  I've re-enabled
them to see if the flakiness still exists, since they're able to catch this
regression.

+  if (waiting_for_beforeunload) {
     // Haven't gotten around to starting the request, because we're still
     // waiting for the beforeunload handler to finish.  We'll pretend that it
     // did finish, to let the navigation proceed.  Note that there's a danger
@@ -202,18 +203,11 @@ bool RenderViewHostManager::ShouldCloseTabOnUnresponsiveRenderer() {
           false, base::TimeTicks::Now());
   } else {
     // The request has been started and paused while we're waiting for the
-    // unload handler to finish.  We'll pretend that it did, by notifying the
-    // IO thread to let the response continue.  The pending renderer will then
-    // be swapped in as part of the usual DidNavigate logic.  (If the unload
-    // handler later finishes, this call will be ignored because the state in
-    // CrossSiteResourceHandler will already be cleaned up.)
-    ViewMsg_SwapOut_Params params;
-    params.closing_process_id = render_view_host_->GetProcess()->GetID();
-    params.closing_route_id = render_view_host_->GetRoutingID();
-    params.new_render_process_host_id =
-        pending_render_view_host_->GetProcess()->GetID();
-    params.new_request_id = pending_request_id;
-    current_host()->GetProcess()->SimulateSwapOutACK(params);
+    // unload handler to finish.  We'll pretend that it did.  The pending
+    // renderer will then be swapped in as part of the usual DidNavigate logic.
+    // (If the unload handler later finishes, this call will be ignored because
+    // the pending_nav_params_ state in WebContents will already be cleaned up.)
+    current_host()->OnSwappedOut(true);
   }
   return false;
 }
@@ -237,10 +231,8 @@ void RenderViewHostManager::DidNavigateMainFrame(
     // If it committed without sending network requests (e.g., data URLs),
     // then we still need to swap out the old RVH first and run its unload
     // handler.  OK for that to happen in the background.
-    if (pending_render_view_host_->GetPendingRequestId() == -1) {
-      OnCrossSiteResponse(pending_render_view_host_->GetProcess()->GetID(),
-                          pending_render_view_host_->GetRoutingID());
-    }
+    if (pending_render_view_host_->HasPendingCrossSiteRequest())
+      SwapOutOldPage();
 
     CommitPending();
     cross_navigation_pending_ = false;
@@ -338,8 +330,7 @@ void RenderViewHostManager::ShouldClosePage(
   }
 }
 
-void RenderViewHostManager::OnCrossSiteResponse(int new_render_process_host_id,
-                                                int new_request_id) {
+void RenderViewHostManager::SwapOutOldPage() {
   // Should only see this while we have a pending renderer.
   if (!cross_navigation_pending_)
     return;
@@ -348,16 +339,15 @@ void RenderViewHostManager::OnCrossSiteResponse(int new_render_process_host_id,
   // Tell the old renderer it is being swapped out.  This will fire the unload
   // handler (without firing the beforeunload handler a second time).  When the
   // unload handler finishes and the navigation completes, we will send a
-  // message to the ResourceDispatcherHost with the given pending request IDs,
-  // allowing the pending RVH's response to resume.
-  render_view_host_->SwapOut(new_render_process_host_id, new_request_id);
+  // message to the ResourceDispatcherHost, allowing the pending RVH's response
+  // to resume.
+  render_view_host_->SwapOut();
 
   // ResourceDispatcherHost has told us to run the onunload handler, which
   // means it is not a download or unsafe page, and we are going to perform the
   // navigation.  Thus, we no longer need to remember that the RenderViewHost
   // is part of a pending cross-site request.
-  pending_render_view_host_->SetHasPendingCrossSiteRequest(false,
-                                                           new_request_id);
+  pending_render_view_host_->SetHasPendingCrossSiteRequest(false);
 }
 
 void RenderViewHostManager::Observe(
@@ -856,7 +846,7 @@ RenderViewHostImpl* RenderViewHostManager::UpdateRendererStateForNavigate(
     // Tell the CrossSiteRequestManager that this RVH has a pending cross-site
     // request, so that ResourceDispatcherHost will know to tell us to run the
     // old page's unload handler before it sends the response.
-    pending_render_view_host_->SetHasPendingCrossSiteRequest(true, -1);
+    pending_render_view_host_->SetHasPendingCrossSiteRequest(true);
 
     // We now have a pending RVH.
     DCHECK(!cross_navigation_pending_);
@@ -918,9 +908,7 @@ void RenderViewHostManager::CancelPending() {
     // Any currently suspended navigations are no longer needed.
     pending_render_view_host->CancelSuspendedNavigations();
 
-    // We can pass -1,-1 because there is no pending response in the
-    // ResourceDispatcherHost to unpause.
-    pending_render_view_host->SwapOut(-1, -1);
+    pending_render_view_host->SwapOut();
   } else {
     // We won't be coming back, so shut this one down.
     pending_render_view_host->Shutdown();