When cross-site navigations are cancelled, delete the request being transferred

content/browser/frame_host/render_frame_host_manager.cc

Index: content/browser/frame_host/render_frame_host_manager.cc
===================================================================
--- content/browser/frame_host/render_frame_host_manager.cc	(revision 250813)
+++ content/browser/frame_host/render_frame_host_manager.cc	(working copy)
@@ -18,6 +18,7 @@
 #include "content/browser/frame_host/navigation_entry_impl.h"
 #include "content/browser/frame_host/render_frame_host_factory.h"
 #include "content/browser/frame_host/render_frame_host_impl.h"
+#include "content/browser/renderer_host/cross_site_request_transfer.h"
 #include "content/browser/renderer_host/render_process_host_impl.h"
 #include "content/browser/renderer_host/render_view_host_factory.h"
 #include "content/browser/renderer_host/render_view_host_impl.h"
@@ -37,20 +38,16 @@
 
 namespace content {
 
-RenderFrameHostManager::PendingNavigationParams::PendingNavigationParams()
-    : is_transfer(false), frame_id(-1), should_replace_current_entry(false) {
-}
-
 RenderFrameHostManager::PendingNavigationParams::PendingNavigationParams(
     const GlobalRequestID& global_request_id,
-    bool is_transfer,
+    scoped_ptr<CrossSiteRequestTransfer> cross_site_request_transfer,
     const std::vector<GURL>& transfer_url_chain,
     Referrer referrer,
     PageTransition page_transition,
     int64 frame_id,
     bool should_replace_current_entry)
     : global_request_id(global_request_id),
-      is_transfer(is_transfer),
+      cross_site_request_transfer(cross_site_request_transfer.Pass()),
       transfer_url_chain(transfer_url_chain),
       referrer(referrer),
       page_transition(page_transition),
@@ -279,7 +276,7 @@
   // 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_->is_transfer) {
+  if (pending_nav_params_->cross_site_request_transfer) {
     // 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());
@@ -328,7 +325,7 @@
   // 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_->is_transfer) {
+  if (pending_nav_params_->cross_site_request_transfer) {
     // 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());
@@ -480,7 +477,7 @@
 void RenderFrameHostManager::OnCrossSiteResponse(
     RenderViewHost* pending_render_view_host,
     const GlobalRequestID& global_request_id,
-    bool is_transfer,
+    scoped_ptr<CrossSiteRequestTransfer> cross_site_request_transfer,
     const std::vector<GURL>& transfer_url_chain,
     const Referrer& referrer,
     PageTransition page_transition,
@@ -496,10 +493,11 @@
   // 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 is_transfer is false, we will just run the unload handler and resume.
+  // If |cross_site_request_transfer| is NULL, we will just run the unload
+  // handler and resume.
   pending_nav_params_.reset(new PendingNavigationParams(
-      global_request_id, is_transfer, transfer_url_chain, referrer,
-      page_transition, frame_id, should_replace_current_entry));
+      global_request_id, cross_site_request_transfer.Pass(), transfer_url_chain,
+      referrer, page_transition, frame_id, should_replace_current_entry));
 
   // Run the unload handler of the current page.
   SwapOutOldPage();
@@ -1222,6 +1220,15 @@
   if (force_swap)
     CHECK_NE(new_instance, current_instance);
 
+  // If entry includes the request ID of a request that is being transferred,
+  // either the old renderer is about to to reclaim it, or a new one is going

[Charlie Reis, 2014/02/13 22:12:19]

nit: to to

I'm not sure I follow this.  Do you mean cancel or abort by "reclaim"?  Can you
specify where the new renderer claims ownership?  (I think you mean
RDH::UpdateRequestForTransfer?)

Also, is there a reason this code is here?  It's currently in the middle of the
decision of whether to swap processes, but that doesn't seem to matter for it. 
If you want it to happen on any navigation that passes through RFHM, maybe
RFHM::Navigate is a better spot?

[mmenke, 2014/02/14 16:30:02]

I mean the current_instance can take ownership - I assume this happens in the
case where we think we have a cross-process navigation, but it turns out we
don't, so the first renderer takes back ownership of the request.  I've
simplified the comment.
You're right...  I had thought this should only run on cross-site navigations,
and since we had special logic for that case here, I put it here.  But then I
found out we sometimes transfer the request to current_instance.  By moving it,
we also now handle the case where CreateRenderFrame fails.

+  // claim ownership, so release ownership of the request.
+  if (pending_nav_params_ &&
+      pending_nav_params_->global_request_id ==
+          entry.transferred_global_request_id()) {
+    pending_nav_params_->cross_site_request_transfer->ReleaseRequest();
+  }
+
   if (new_instance != current_instance) {
     // New SiteInstance: create a pending RFH to navigate.
     DCHECK(!cross_navigation_pending_);