Remove incorrect sanity checks in RenderFrameHostManager::OnCrossSiteResponse.

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 <stddef.h>
 
 #include <algorithm>
 #include <utility>
@@ skipping 401 matching lines @@
       if (IsBrowserSideNavigationEnabled() && speculative_render_frame_host_)
         CleanUpNavigation();
 
       // This is not a cross-process navigation; the tab is being closed.
       render_frame_host_->render_view_host()->ClosePage();
     }
   }
 }
 
 void RenderFrameHostManager::OnCrossSiteResponse(
     RenderFrameHostImpl* pending_render_frame_host,

[Charlie Reis, 2016/02/26 23:51:19]

Maybe we should rename this while we're at it?  Something like
transferring_render_frame_host?  (I'm open to better suggestions, but the
current name is admittedly confusing.)

[alexmos, 2016/02/27 00:20:28]

Good idea, done.

     const GlobalRequestID& global_request_id,
     scoped_ptr<CrossSiteTransferringRequest> cross_site_transferring_request,
     const std::vector<GURL>& transfer_url_chain,
     const Referrer& referrer,
     ui::PageTransition page_transition,
     bool should_replace_current_entry) {
   // 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);
@@ skipping 18 matching lines @@
   transfer_navigation_handle_ =
       pending_render_frame_host->PassNavigationHandleOwnership();
   DCHECK(transfer_navigation_handle_);
 
   // Set the transferring RenderFrameHost as not loading, so that it does not
   // emit a DidStopLoading notification if it is destroyed when creating the
   // new navigating RenderFrameHost.
   pending_render_frame_host->set_is_loading(false);
 
   // 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_ ?
+  // 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.
+  //
+  // Note that having a pending RenderFrameHost does not imply that it was the
+  // one that made the request.  Suppose that during a pending cross-site
+  // navigation, the frame performs a different same-site navigation which
+  // redirects cross-site.  In this case, there will be a pending
+  // RenderFrameHost, but this request is made by the current RenderFrameHost.
+  // Later, this will create a new pending RenderFrameHost and clean up the old
+  // one.
+  int request_routing_id = pending_render_frame_host->GetRoutingID();
+  int pending_routing_id = pending_render_frame_host_ ?
+      pending_render_frame_host_->GetRoutingID() :
+      MSG_ROUTING_NONE;
+  DCHECK(request_routing_id == pending_routing_id ||
+         request_routing_id == render_frame_host_->GetRoutingID());

[Charlie Reis, 2016/02/26 23:51:19]

This seems redundant with line 438.  (It kind of was before, too, except for the
case where it was wrong.)  :)

We're basically just saying that the RFH parameter should match either the
current or pending RFH, and that's handled above.

[alexmos, 2016/02/27 00:20:28]

Acknowledged.

+  int process_id = request_routing_id == pending_routing_id ?

[Charlie Reis, 2016/02/26 23:51:19]

Looking closer, I don't think this first part is right.  The pending and current
RFHs could have the same routing ID but different process IDs, and this would
make us pick the pending RFH's process ID even if the request came from the
current RFH.

But maybe we don't need this.  The process_id DCHECK below is basically just
comparing the RFH parameter's GetProcess()->GetID() with
global_request_id.child_id now.  We could do that if we wanted, but it doesn't
seem likely to fail in practice.

[alexmos, 2016/02/27 00:20:28]

Indeed, you're right.  Thanks for catching it!  I agree the check on line 438 is
enough, so I've removed both of these DCHECKs and merged the comment here to
line 438.

       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();
@@ skipping 2018 matching lines @@
 int RenderFrameHostManager::GetOpenerRoutingID(SiteInstance* instance) {
   if (!frame_tree_node_->opener())
     return MSG_ROUTING_NONE;
 
   return frame_tree_node_->opener()
       ->render_manager()
       ->GetRoutingIdForSiteInstance(instance);
 }
 
 }  // namespace content