Don't swap out the old RenderFrameHost until the new one commits.

content/browser/frame_host/render_frame_host_manager.cc

 // Copyright 2013 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/frame_host/render_frame_host_manager.h"
 
 #include <utility>
 
 #include "base/command_line.h"
 #include "base/debug/trace_event.h"
@@ skipping 24 matching lines @@
 #include "content/public/browser/notification_types.h"
 #include "content/public/browser/render_widget_host_iterator.h"
 #include "content/public/browser/render_widget_host_view.h"
 #include "content/public/browser/user_metrics.h"
 #include "content/public/browser/web_ui_controller.h"
 #include "content/public/common/content_switches.h"
 #include "content/public/common/url_constants.h"
 
 namespace content {
 
-RenderFrameHostManager::PendingNavigationParams::PendingNavigationParams(
-    const GlobalRequestID& global_request_id,
-    scoped_ptr<CrossSiteTransferringRequest> cross_site_transferring_request,
-    const std::vector<GURL>& transfer_url_chain,
-    Referrer referrer,
-    PageTransition page_transition,
-    int render_frame_id,
-    bool should_replace_current_entry)
-    : global_request_id(global_request_id),
-      cross_site_transferring_request(cross_site_transferring_request.Pass()),
-      transfer_url_chain(transfer_url_chain),
-      referrer(referrer),
-      page_transition(page_transition),
-      render_frame_id(render_frame_id),
-      should_replace_current_entry(should_replace_current_entry) {
-}
-
-RenderFrameHostManager::PendingNavigationParams::~PendingNavigationParams() {}
-
 bool RenderFrameHostManager::ClearRFHsPendingShutdown(FrameTreeNode* node) {
   node->render_manager()->pending_delete_hosts_.clear();
   return true;
 }
 
 RenderFrameHostManager::RenderFrameHostManager(
     FrameTreeNode* frame_tree_node,
     RenderFrameHostDelegate* render_frame_delegate,
     RenderViewHostDelegate* render_view_delegate,
     RenderWidgetHostDelegate* render_widget_delegate,
@@ skipping 140 matching lines @@
       // Notify here as we won't be calling CommitPending (which does the
       // notify).
       delegate_->NotifySwappedFromRenderManager(
           NULL, render_frame_host_.get(), frame_tree_node_->IsMainFrame());
     }
   }
 
   // If entry includes the request ID of a request that is being transferred,
   // the destination render frame will take ownership, so release ownership of
   // the request.
-  if (pending_nav_params_ &&
-      pending_nav_params_->global_request_id ==
+  if (cross_site_transferring_request_.get() &&
+      cross_site_transferring_request_->request_id() ==
           entry.transferred_global_request_id()) {
-    pending_nav_params_->cross_site_transferring_request->ReleaseRequest();
+    cross_site_transferring_request_->ReleaseRequest();
   }
 
   return dest_render_frame_host;
 }
 
 void RenderFrameHostManager::Stop() {
   render_frame_host_->render_view_host()->Stop();
 
   // If we are cross-navigating, we should stop the pending renderers.  This
   // will lead to a DidFailProvisionalLoad, which will properly destroy them.
@@ skipping 90 matching lines @@
 }
 
 void RenderFrameHostManager::OnCrossSiteResponse(
     RenderFrameHostImpl* pending_render_frame_host,
     const GlobalRequestID& global_request_id,
     scoped_ptr<CrossSiteTransferringRequest> cross_site_transferring_request,
     const std::vector<GURL>& transfer_url_chain,
     const Referrer& referrer,
     PageTransition page_transition,
     bool should_replace_current_entry) {
-  // This should be called either when the pending RFH is ready to commit or
-  // when we realize that the current RFH's request requires a transfer.
+  // We should only get here for transfer navigations.  Most cross-process
+  // navigations can just continue and wait to run the unload handler (by
+  // swapping out) when the new navigation commits.
+  CHECK(cross_site_transferring_request.get());
+
+  // A transfer should only have come from our pending or current RFH.
+  // TODO(creis): We need to handle the case that the pending RFH has changed
+  // in the mean time, while this was being posted from the IO thread.  We
+  // should probably cancel the request in that case.
   DCHECK(pending_render_frame_host == pending_render_frame_host_ ||
          pending_render_frame_host == render_frame_host_);
 
-  // TODO(creis): Eventually we will want to check all navigation responses
-  // here, but currently we pass information for a transfer if
-  // ShouldSwapProcessesForRedirect returned true in the network stack.
-  // In that case, we should set up a transfer after the unload handler runs.
-  // If |cross_site_transferring_request| is NULL, we will just run the unload
-  // handler and resume.
-  pending_nav_params_.reset(new PendingNavigationParams(
-      global_request_id, cross_site_transferring_request.Pass(),
-      transfer_url_chain, referrer, page_transition,
-      pending_render_frame_host->GetRoutingID(),
-      should_replace_current_entry));
+  // Store the transferring request so that we can release it if the transfer
+  // navigation matches.
+  cross_site_transferring_request_ = cross_site_transferring_request.Pass();
 
-  // Run the unload handler of the current page.
-  SwapOutOldPage();
+  // Sanity check that the params are for the correct frame and process.
+  // These should match the RenderFrameHost that made the request.
+  // If it started as a cross-process navigation via OpenURL, this is the
+  // pending one.  If it wasn't cross-process until the transfer, this is the
+  // current one.
+  int render_frame_id = pending_render_frame_host_ ?
+      pending_render_frame_host_->GetRoutingID() :
+      render_frame_host_->GetRoutingID();
+  DCHECK_EQ(render_frame_id, pending_render_frame_host->GetRoutingID());
+  int process_id = pending_render_frame_host_ ?
+      pending_render_frame_host_->GetProcess()->GetID() :
+      render_frame_host_->GetProcess()->GetID();
+  DCHECK_EQ(process_id, global_request_id.child_id);
+
+  // Treat the last URL in the chain as the destination and the remainder as
+  // the redirect chain.
+  CHECK(transfer_url_chain.size());
+  GURL transfer_url = transfer_url_chain.back();
+  std::vector<GURL> rest_of_chain = transfer_url_chain;
+  rest_of_chain.pop_back();
+
+  // We don't know whether the original request had |user_action| set to true.
+  // However, since we force the navigation to be in the current tab, it
+  // doesn't matter.
+  pending_render_frame_host->frame_tree_node()->navigator()->RequestTransferURL(
+      pending_render_frame_host,
+      transfer_url,
+      rest_of_chain,
+      referrer,
+      page_transition,
+      CURRENT_TAB,
+      global_request_id,
+      should_replace_current_entry,
+      true);
+  cross_site_transferring_request_.reset();

[clamy, 2014/08/19 14:59:05]

Why do you reset cross_site_transferring_request_ here?

[Charlie Reis, 2014/08/19 16:26:04]

It's only needed during the RequestTransferURL call, after which the transfer
has completed.  I've added a comment to that effect.

I considered finding a way to not store it as state at all, but the lifetime of
that object is a bit subtle and this CL seemed big enough as is.

 }
 
 void RenderFrameHostManager::OnDeferredAfterResponseStarted(
     const GlobalRequestID& global_request_id,
     RenderFrameHostImpl* pending_render_frame_host) {
   DCHECK(!response_started_id_.get());
 
   response_started_id_.reset(new GlobalRequestID(global_request_id));
 }
 
 void RenderFrameHostManager::ResumeResponseDeferredAtStart() {
   DCHECK(response_started_id_.get());
 
   RenderProcessHostImpl* process =
       static_cast<RenderProcessHostImpl*>(render_frame_host_->GetProcess());
   process->ResumeResponseDeferredAtStart(*response_started_id_);
 
   render_frame_host_->ClearPendingTransitionRequestData();
 
   response_started_id_.reset();
 }
 
-void RenderFrameHostManager::SwappedOut(
-    RenderFrameHostImpl* render_frame_host) {
-  // Make sure this is from our current RFH, and that we have a pending
-  // navigation from OnCrossSiteResponse.  (There may be no pending navigation
-  // for data URLs that don't make network requests, for example.)   If not,
-  // just return early and ignore.
-  if (render_frame_host != render_frame_host_ || !pending_nav_params_.get()) {
-    pending_nav_params_.reset();
-    return;
-  }
-
-  // Now that the unload handler has run, we need to either initiate the
-  // pending transfer (if there is one) or resume the paused response (if not).
-  // TODO(creis): The blank swapped out page is visible during this time, but
-  // we can shorten this by delivering the response directly, rather than
-  // forcing an identical request to be made.
-  if (pending_nav_params_->cross_site_transferring_request) {
-    // Sanity check that the params are for the correct frame and process.
-    // These should match the RenderFrameHost that made the request.
-    // If it started as a cross-process navigation via OpenURL, this is the
-    // pending one.  If it wasn't cross-process until the transfer, this is the
-    // current one.
-    int render_frame_id = pending_render_frame_host_ ?
-        pending_render_frame_host_->GetRoutingID() :
-        render_frame_host_->GetRoutingID();
-    DCHECK_EQ(render_frame_id, pending_nav_params_->render_frame_id);
-    int process_id = pending_render_frame_host_ ?
-        pending_render_frame_host_->GetProcess()->GetID() :
-        render_frame_host_->GetProcess()->GetID();
-    DCHECK_EQ(process_id, pending_nav_params_->global_request_id.child_id);
-
-    // Treat the last URL in the chain as the destination and the remainder as
-    // the redirect chain.
-    CHECK(pending_nav_params_->transfer_url_chain.size());
-    GURL transfer_url = pending_nav_params_->transfer_url_chain.back();
-    pending_nav_params_->transfer_url_chain.pop_back();
-
-    // We don't know whether the original request had |user_action| set to true.
-    // However, since we force the navigation to be in the current tab, it
-    // doesn't matter.
-    render_frame_host->frame_tree_node()->navigator()->RequestTransferURL(
-        render_frame_host,
-        transfer_url,
-        pending_nav_params_->transfer_url_chain,
-        pending_nav_params_->referrer,
-        pending_nav_params_->page_transition,
-        CURRENT_TAB,
-        pending_nav_params_->global_request_id,
-        pending_nav_params_->should_replace_current_entry,
-        true);
-  } else if (pending_render_frame_host_) {
-    RenderProcessHostImpl* pending_process =
-        static_cast<RenderProcessHostImpl*>(
-            pending_render_frame_host_->GetProcess());
-    pending_process->ResumeDeferredNavigation(
-        pending_nav_params_->global_request_id);
-  }
-  pending_nav_params_.reset();
-}
-
 void RenderFrameHostManager::DidNavigateFrame(
     RenderFrameHostImpl* render_frame_host) {
   if (!cross_navigation_pending_) {
     DCHECK(!pending_render_frame_host_);
 
     // We should only hear this from our current renderer.
     DCHECK_EQ(render_frame_host_, render_frame_host);
 
     // Even when there is no pending RVH, there may be a pending Web UI.
     if (pending_web_ui())
       CommitPending();
     return;
   }
 
   if (render_frame_host == pending_render_frame_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 RFH first and run its unload
-    // handler, only if it hasn't happened yet.  OK for that to happen in the
-    // background.
-    if (pending_render_frame_host_->HasPendingCrossSiteRequest() &&
-        pending_render_frame_host_->render_view_host()->rvh_state() ==
-            RenderViewHostImpl::STATE_DEFAULT) {
-      SwapOutOldPage();
-    }
-
     CommitPending();
     cross_navigation_pending_ = false;
   } else if (render_frame_host == render_frame_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.
     DCHECK(false);
@@ skipping 26 matching lines @@
   }
 
   // Now delete them.
   while (!ids_to_remove.empty()) {
     delete proxy_hosts_[ids_to_remove.back()];
     proxy_hosts_.erase(ids_to_remove.back());
     ids_to_remove.pop_back();
   }
 }
 
-void RenderFrameHostManager::SwapOutOldPage() {
-  // Should only see this while we have a pending renderer or transfer.
-  CHECK(cross_navigation_pending_ || pending_nav_params_.get());
+void RenderFrameHostManager::SwapOutOldPage(
+    RenderFrameHostImpl* old_render_frame_host) {
+  // Should only see this while we have a pending renderer.
+  CHECK(cross_navigation_pending_);
 
   // Tell the renderer to suppress any further modal dialogs so that we can swap
   // it out.  This must be done before canceling any current dialog, in case
   // there is a loop creating additional dialogs.
   // TODO(creis): Handle modal dialogs in subframe processes.
-  render_frame_host_->render_view_host()->SuppressDialogsUntilSwapOut();
+  old_render_frame_host->render_view_host()->SuppressDialogsUntilSwapOut();
 
   // Now close any modal dialogs that would prevent us from swapping out.  This
   // must be done separately from SwapOut, so that the PageGroupLoadDeferrer is
   // no longer on the stack when we send the SwapOut message.
   delegate_->CancelModalDialogsForRenderManager();
 
   // Create the RenderFrameProxyHost that will replace the
   // RenderFrameHost which is swapping out. If one exists, ensure it is deleted
   // from the map and not leaked.
-  DeleteRenderFrameProxyHost(render_frame_host_->GetSiteInstance());
+  DeleteRenderFrameProxyHost(old_render_frame_host->GetSiteInstance());
 
   RenderFrameProxyHost* proxy = new RenderFrameProxyHost(
-      render_frame_host_->GetSiteInstance(), frame_tree_node_);
+      old_render_frame_host->GetSiteInstance(), frame_tree_node_);
   std::pair<RenderFrameProxyHostMap::iterator, bool> result =
       proxy_hosts_.insert(std::make_pair(
-          render_frame_host_->GetSiteInstance()->GetId(), proxy));
+          old_render_frame_host->GetSiteInstance()->GetId(), proxy));
   CHECK(result.second) << "Inserting a duplicate item.";
 
   // Tell the old frame it is being swapped out.  This will fire the unload
   // handler in the background (without firing the beforeunload handler a second
-  // time).  When the navigation completes, we will send a message to the
-  // ResourceDispatcherHost, allowing the pending RVH's response to resume.
-  render_frame_host_->SwapOut(proxy);
-
-  // 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 RenderFrameHost
-  // is part of a pending cross-site request.
-  if (pending_render_frame_host_) {
-    pending_render_frame_host_->SetHasPendingCrossSiteRequest(false);
-  }
+  // time).  This is done right after we commit the new RenderFrameHost.
+  old_render_frame_host->SwapOut(proxy);
 }
 
 void RenderFrameHostManager::ClearPendingShutdownRFHForSiteInstance(
     int32 site_instance_id,
     RenderFrameHostImpl* rfh) {
   RFHPendingDeleteMap::iterator iter =
       pending_delete_hosts_.find(site_instance_id);
   if (iter != pending_delete_hosts_.end() && iter->second.get() == rfh)
     pending_delete_hosts_.erase(site_instance_id);
 }
@@ skipping 539 matching lines @@
   return MSG_ROUTING_NONE;
 }
 
 void RenderFrameHostManager::CommitPending() {
   // First check whether we're going to want to focus the location bar after
   // this commit.  We do this now because the navigation hasn't formally
   // committed yet, so if we've already cleared |pending_web_ui_| the call chain
   // this triggers won't be able to figure out what's going on.
   bool will_focus_location_bar = delegate_->FocusLocationBarByDefault();
 
-  // We expect SwapOutOldPage to have canceled any modal dialogs and told the
-  // renderer to suppress any further dialogs until it is swapped out.  However,
-  // crash reports indicate that it's still possible for modal dialogs to exist
-  // at this point, which poses a risk if we delete their RenderViewHost below.
-  // Cancel them again to be safe.  http://crbug.com/324320.
-  delegate_->CancelModalDialogsForRenderManager();
-
   // Next commit the Web UI, if any. Either replace |web_ui_| with
   // |pending_web_ui_|, or clear |web_ui_| if there is no pending WebUI, or
   // leave |web_ui_| as is if reusing it.
   DCHECK(!(pending_web_ui_.get() && pending_and_current_web_ui_.get()));
   if (pending_web_ui_) {
     web_ui_.reset(pending_web_ui_.release());
   } else if (!pending_and_current_web_ui_.get()) {
     web_ui_.reset();
   } else {
     DCHECK_EQ(pending_and_current_web_ui_.get(), web_ui_.get());
@@ skipping 32 matching lines @@
   // If the view is gone, then this RenderViewHost died while it was hidden.
   // We ignored the RenderProcessGone call at the time, so we should send it now
   // to make sure the sad tab shows up, etc.
   if (!render_frame_host_->render_view_host()->GetView()) {
     delegate_->RenderProcessGoneFromRenderManager(
         render_frame_host_->render_view_host());
   } else if (!delegate_->IsHidden()) {
     render_frame_host_->render_view_host()->GetView()->Show();
   }
 
-  // If the old view is live and top-level, hide it now that the new one is
-  // visible.
+  // If the old frame is live, swap it out now that the new frame is visible.
   int32 old_site_instance_id =
       old_render_frame_host->GetSiteInstance()->GetId();
-  if (old_render_frame_host->render_view_host()->GetView()) {
-    if (is_main_frame) {
-      old_render_frame_host->render_view_host()->GetView()->Hide();
-      old_render_frame_host->render_view_host()->WasSwappedOut(base::Bind(
+  if (old_render_frame_host->render_view_host()->IsRenderViewLive()) {
+    SwapOutOldPage(old_render_frame_host.get());
+
+    // Schedule the old frame to shut down after it swaps out, if there are no
+    // other active views in its SiteInstance.
+    if (!static_cast<SiteInstanceImpl*>(
+            old_render_frame_host->GetSiteInstance())->active_view_count()) {
+      old_render_frame_host->render_view_host()->SetPendingShutdown(base::Bind(
           &RenderFrameHostManager::ClearPendingShutdownRFHForSiteInstance,
           weak_factory_.GetWeakPtr(),
           old_site_instance_id,
           old_render_frame_host.get()));
-    } else {
-      // TODO(creis): We'll need to set this back to false if we navigate back.
-      old_render_frame_host->set_swapped_out(true);
     }
   }
 
+  // For top-level frames, also hide the old RenderViewHost's view.
+  if (is_main_frame && old_render_frame_host->render_view_host()->GetView())
+    old_render_frame_host->render_view_host()->GetView()->Hide();
+
   // Make sure the size is up to date.  (Fix for bug 1079768.)
   delegate_->UpdateRenderViewSizeForRenderManager();
 
   if (will_focus_location_bar) {
     delegate_->SetFocusToLocationBar(false);
   } else if (focus_render_view &&
              render_frame_host_->render_view_host()->GetView()) {
     render_frame_host_->render_view_host()->GetView()->Focus();
   }
 
   // Notify that we've swapped RenderFrameHosts. We do this before shutting down
   // the RFH so that we can clean up RendererResources related to the RFH first.
   delegate_->NotifySwappedFromRenderManager(
       old_render_frame_host.get(), render_frame_host_.get(), is_main_frame);
 
-  // If the old RFH is not live, just return as there is no work to do.
-  if (!old_render_frame_host->render_view_host()->IsRenderViewLive()) {
+  // If the old RFH is not live, just return as there is no further work to do.
+  if (!old_render_frame_host->render_view_host()->IsRenderViewLive())
     return;
-  }
 
   // If the old RFH is live, we are swapping it out and should keep track of
   // it in case we navigate back to it, or it is waiting for the unload event
   // to execute in the background.
   // TODO(creis): Swap out the subframe in --site-per-process.
-  if (!CommandLine::ForCurrentProcess()->HasSwitch(switches::kSitePerProcess))
+  if (!CommandLine::ForCurrentProcess()->HasSwitch(switches::kSitePerProcess)) {
     DCHECK(old_render_frame_host->is_swapped_out() ||
            !RenderViewHostImpl::IsRVHStateActive(
                old_render_frame_host->render_view_host()->rvh_state()));
+  }
 
   // If the RenderViewHost backing the RenderFrameHost is pending shutdown,
   // the RenderFrameHost should be put in the map of RenderFrameHosts pending
   // shutdown. Otherwise, it is stored in the map of proxy hosts.
   if (old_render_frame_host->render_view_host()->rvh_state() ==
       RenderViewHostImpl::STATE_PENDING_SHUTDOWN) {
     // The proxy for this RenderFrameHost is created when sending the
     // SwapOut message, so check if it already exists and delete it.
     RenderFrameProxyHostMap::iterator iter =
         proxy_hosts_.find(old_site_instance_id);
@@ skipping 191 matching lines @@
     // Suspend the new render view (i.e., don't let it send the cross-site
     // Navigate message) until we hear back from the old renderer's
     // beforeunload handler.  If the handler returns false, we'll have to
     // cancel the request.
     DCHECK(!pending_render_frame_host_->are_navigations_suspended());
     bool is_transfer =
         entry.transferred_global_request_id() != GlobalRequestID();
     if (is_transfer) {
       // We don't need to stop the old renderer or run beforeunload/unload
       // handlers, because those have already been done.
-      DCHECK(pending_nav_params_->global_request_id ==
+      DCHECK(cross_site_transferring_request_->request_id() ==
                 entry.transferred_global_request_id());
     } else {
       // Also make sure the old render view stops, in case a load is in
       // progress.  (We don't want to do this for transfers, since it will
       // interrupt the transfer with an unexpected DidStopLoading.)
       render_frame_host_->render_view_host()->Send(new ViewMsg_Stop(
           render_frame_host_->render_view_host()->GetRoutingID()));
 
       pending_render_frame_host_->SetNavigationsSuspended(true,
                                                           base::TimeTicks());
-
-      // Tell the CrossSiteRequestManager that this RFH 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_frame_host_->SetHasPendingCrossSiteRequest(true);
     }
 
     // We now have a pending RFH.
     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 OnBeforeUnloadACK with the reply.
@@ skipping 150 matching lines @@
 void RenderFrameHostManager::DeleteRenderFrameProxyHost(
     SiteInstance* instance) {
   RenderFrameProxyHostMap::iterator iter = proxy_hosts_.find(instance->GetId());
   if (iter != proxy_hosts_.end()) {
     delete iter->second;
     proxy_hosts_.erase(iter);
   }
 }
 
 }  // namespace content