Don't swap processes for chrome:// subframes.

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 <iterator>
 
 #include "base/debug/dump_without_crashing.h"
 #include "base/logging.h"
@@ skipping 718 matching lines @@
   //
   // Similarly, HTTP 204 (No Content) responses leave the renderer showing the
   // previous page. The navigation should be allowed to finish without running
   // the unload handler or swapping in the pending RenderFrameHost.
   if (is_download_ || is_stream_ ||
       (response_headers_.get() && response_headers_->response_code() == 204)) {
     return true;
   }
 
   // The content embedder can decide that a transfer to a different process is
-  // required for this URL.
-  bool should_transfer =
-      GetContentClient()->browser()->ShouldSwapProcessesForRedirect(
+  // required for this URL. Start the transfer if needed.
+  if (GetContentClient()->browser()->ShouldSwapProcessesForRedirect(
           frame_tree_node_->navigator()->GetController()->GetBrowserContext(),
-          original_url_, url_);
-
-  RenderFrameHostManager* manager =
-      render_frame_host_->frame_tree_node()->render_manager();
-
-  // 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(

[Charlie Reis, 2017/01/21 00:46:44]

Removing this whole block looks wrong to me.  I expected to only remove this one
line, such that all navigations get sent through
IsRendererTransferNeededForNavigation.

Hopefully we'll see test failures?  If not, we need to look closely at whether
IsRendererTransferNeededForNavigation is still needed.

[nasko, 2017/01/23 16:51:49]

Yes, you are correct. I thought I ran successfully locally tests with this whole
block removed, but apparently it wasn't exactly that.

-          render_frame_host_->GetProcess()->GetID())) {
-    should_transfer |= manager->IsRendererTransferNeededForNavigation(
-        render_frame_host_, url_);
-  }
-
-  // Start the transfer if needed.
-  if (should_transfer) {
+          original_url_, url_)) {
     // This may destroy the NavigationHandle if the transfer fails.
     base::WeakPtr<NavigationHandleImpl> weak_self = weak_factory_.GetWeakPtr();
-    manager->OnCrossSiteResponse(render_frame_host_, request_id_,
-                                 redirect_chain_, sanitized_referrer_,
-                                 transition_, should_replace_current_entry_);
+    render_frame_host_->frame_tree_node()
+        ->render_manager()
+        ->OnCrossSiteResponse(render_frame_host_, request_id_, redirect_chain_,
+                              sanitized_referrer_, transition_,
+                              should_replace_current_entry_);
     if (!weak_self)
       return false;
   }
 
   return true;
 }
 
 void NavigationHandleImpl::RunCompleteCallback(
     NavigationThrottle::ThrottleCheckResult result) {
   DCHECK(result != NavigationThrottle::DEFER);
@@ skipping 29 matching lines @@
       content::AncestorThrottle::MaybeCreateThrottleFor(this);
   if (ancestor_throttle)
     throttles_.push_back(std::move(ancestor_throttle));
 
   throttles_.insert(throttles_.begin(),
                     std::make_move_iterator(throttles_to_register.begin()),
                     std::make_move_iterator(throttles_to_register.end()));
 }
 
 }  // namespace content