Eliminate SwapOut message parameters, keeping state in RVHM instead.

content/browser/web_contents/render_view_host_manager.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/web_contents/render_view_host_manager.h"
 
 #include <utility>
 
 #include "base/command_line.h"
 #include "base/logging.h"
@@ skipping 171 matching lines @@
     pending_render_view_host_->SetIsLoading(is_loading);
 }
 
 bool RenderViewHostManager::ShouldCloseTabOnUnresponsiveRenderer() {
   if (!cross_navigation_pending_)
     return true;
 
   // 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
     // that the beforeunload handler will later finish and possibly return
     // false (meaning the navigation should not proceed), but we'll ignore it
     // in this case because it took too long.
     if (pending_render_view_host_->are_navigations_suspended())
       pending_render_view_host_->SetNavigationsSuspended(
           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;
 }
 
 void RenderViewHostManager::DidNavigateMainFrame(
     RenderViewHost* render_view_host) {
   if (!cross_navigation_pending_) {
     DCHECK(!pending_render_view_host_);
 
     // We should only hear this from our current renderer.
     DCHECK(render_view_host == render_view_host_);
 
     // Even when there is no pending RVH, there may be a pending Web UI.
     if (pending_web_ui())
       CommitPending();
     return;
   }
 
   if (render_view_host == pending_render_view_host_) {
     // The pending cross-site navigation completed, so show the renderer.
     // 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;
   } else if (render_view_host == render_view_host_) {
     // A navigation in the original page has taken place.  Cancel the pending
     // one.
     CancelPending();
     cross_navigation_pending_ = false;
   } else {
     // No one else should be sending us DidNavigate in this state.
@@ skipping 77 matching lines @@
     delegate_->BeforeUnloadFiredFromRenderManager(proceed, proceed_time,
                                                   &proceed_to_fire_unload);
 
     if (proceed_to_fire_unload) {
       // This is not a cross-site navigation, the tab is being closed.
       render_view_host_->ClosePage();
     }
   }
 }
 
-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;
   DCHECK(pending_render_view_host_);
 
   // 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(
     int type,
     const NotificationSource& source,
     const NotificationDetails& details) {
   switch (type) {
     case NOTIFICATION_RENDERER_PROCESS_CLOSING:
       RendererProcessClosing(
           Source<RenderProcessHost>(source).ptr());
@@ skipping 478 matching lines @@
       render_view_host_->Send(
           new ViewMsg_Stop(render_view_host_->GetRoutingID()));
 
       pending_render_view_host_->SetNavigationsSuspended(true,
                                                          base::TimeTicks());
     }
 
     // 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_);
     cross_navigation_pending_ = true;
 
     // Unless we are transferring an existing request, we should now
     // tell the old render view to run its beforeunload handler, since it
     // doesn't otherwise know that the cross-site request is happening.  This
     // will trigger a call to ShouldClosePage with the reply.
     if (!is_transfer)
@@ skipping 41 matching lines @@
   pending_render_view_host->GetProcess()->RemovePendingView();
 
   // The pending RVH may already be on the swapped out list if we started to
   // swap it back in and then canceled.  If so, make sure it gets swapped out
   // again.  If it's not on the swapped out list (e.g., aborting a pending
   // load), then it's safe to shut down.
   if (IsOnSwappedOutList(pending_render_view_host)) {
     // 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();
   }
 
   pending_web_ui_.reset();
   pending_and_current_web_ui_.reset();
 }
 
 void RenderViewHostManager::RenderViewDeleted(RenderViewHost* rvh) {
@@ skipping 39 matching lines @@
 RenderViewHostImpl* RenderViewHostManager::GetSwappedOutRenderViewHost(
     SiteInstance* instance) {
   RenderViewHostMap::iterator iter = swapped_out_hosts_.find(instance->GetId());
   if (iter != swapped_out_hosts_.end())
     return iter->second;
 
   return NULL;
 }
 
 }  // namespace content