Prevent RFH from attempting to transfer after it's been swapped out

content/browser/frame_host/navigation_handle_impl.cc

 // Copyright 2015 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/navigation_handle_impl.h"
 
 #include <utility>
 
 #include "base/logging.h"
 #include "content/browser/browsing_data/clear_site_data_throttle.h"
@@ skipping 647 matching lines @@
   // The content embedder can decide that a transfer to a different process is
   // required for this URL.
   bool should_transfer =
       GetContentClient()->browser()->ShouldSwapProcessesForRedirect(
           frame_tree_node_->navigator()->GetController()->GetBrowserContext(),
           original_url_, url_);
 
   RenderFrameHostManager* manager =
       render_frame_host_->frame_tree_node()->render_manager();
 
+  // It is possible for the RenderFrameHost to have been marked as pending
+  // deletion following the commit of another ongoing navigation. In that case,
+  // don't attempt to transfer, but don't resume the navigation either.
+  if (render_frame_host_ != manager->current_frame_host() &&

[Charlie Reis, 2016/09/30 20:25:26]

We used to use is_active() for this (in OnCrossSiteResponse).  That's probably a
better check to use here, given that we're getting |manager| from
|render_frame_host_| anyway.

[clamy, 2016/10/04 11:54:41]

Acknowledged.

+      render_frame_host_ != manager->pending_frame_host()) {
+    return false;

[clamy, 2016/09/30 14:06:15]

This will just suspend the navigation. Alternatively, I could reset the
NavigationHandle in the RFH, which would just cancel the navigation.
Other option: we delete the NavHandle when swapping out the RFH, this would
ensure the navigation will eventually stop & that no updates are sent to the
embedder about a navigation in an RFH that is pending deletion.

[Charlie Reis, 2016/09/30 20:25:26]

We definitely want to cancel the navigation.  The old code did that by deleting
cross_site_transferring_request (unless leak_requests_for_testing_ was true).
I like this idea.  Can we ensure the navigation gets canceled if the IO thread
posts its task and no handle is found?  And if we did that, could we put a
CHECK(render_frame_host_->is_active()) here?

[nasko, 2016/09/30 23:21:33]

+1 on cancelling the navigation. Keeping it suspended only creates ambiguity.

[clamy, 2016/10/04 11:54:41]

Done. The cancellation if the NavigationHandle isn't found has been in place
since the previous patch (it was what was causing issues on Android). I've added
a check a bit earlier at the start of the check for transfer sequence.

+  }
+
   // In the site-per-process model, the RenderFrameHostManager may also decide
   // (independently from the content embedder's ShouldSwapProcessesForRedirect
   // above) that a process transfer is needed. Process transfers are skipped for
   // WebUI processes for now, since e.g. chrome://settings has multiple
   // "cross-site" chrome:// frames, and that doesn't yet work cross-process.
   if (SiteIsolationPolicy::AreCrossProcessFramesPossible() &&
       !ChildProcessSecurityPolicyImpl::GetInstance()->HasWebUIBindings(
           render_frame_host_->GetProcess()->GetID())) {
     should_transfer |= manager->IsRendererTransferNeededForNavigation(
         render_frame_host_, url_);
@@ skipping 45 matching lines @@
     throttles_to_register.push_back(std::move(clear_site_data_throttle));
 
   if (throttles_to_register.size() > 0) {
     throttles_.insert(throttles_.begin(), throttles_to_register.begin(),
                       throttles_to_register.end());
     throttles_to_register.weak_clear();
   }
 }
 
 }  // namespace content