PlzNavigate: implement REUSE_COMMITTED_OR_PENDING_SITE for redirects

content/browser/frame_host/render_frame_host_manager.cc

Index: content/browser/frame_host/render_frame_host_manager.cc
diff --git a/content/browser/frame_host/render_frame_host_manager.cc b/content/browser/frame_host/render_frame_host_manager.cc
index 8da3cb7c28df139f4e1521911bdda3154a4c652b..4a894c0ccb637950bb9ebaaa65904676e2606392 100644
--- a/content/browser/frame_host/render_frame_host_manager.cc
+++ b/content/browser/frame_host/render_frame_host_manager.cc
@@ -701,56 +701,19 @@ RenderFrameHostImpl* RenderFrameHostManager::GetFrameHostForNavigation(
     const NavigationRequest& request) {
   CHECK(IsBrowserSideNavigationEnabled());
 
-  SiteInstance* current_site_instance = render_frame_host_->GetSiteInstance();
-
-  SiteInstance* candidate_site_instance =
-      speculative_render_frame_host_
-          ? speculative_render_frame_host_->GetSiteInstance()
-          : nullptr;
-
-  bool was_server_redirect = request.navigation_handle() &&
-                             request.navigation_handle()->WasServerRedirect();
-
-  scoped_refptr<SiteInstance> dest_site_instance = GetSiteInstanceForNavigation(
-      request.common_params().url, request.source_site_instance(),
-      request.dest_site_instance(), candidate_site_instance,
-      request.common_params().transition,
-      request.restore_type() != RestoreType::NONE, request.is_view_source(),
-      was_server_redirect);
-
   // The appropriate RenderFrameHost to commit the navigation.
   RenderFrameHostImpl* navigation_rfh = nullptr;
 
-  // Reuse the current RenderFrameHost if its SiteInstance matches the
-  // navigation's.
-  bool no_renderer_swap = current_site_instance == dest_site_instance.get();
-
-  if (frame_tree_node_->IsMainFrame()) {
-    // Renderer-initiated main frame navigations that may require a
-    // SiteInstance swap are sent to the browser via the OpenURL IPC and are
-    // afterwards treated as browser-initiated navigations. NavigationRequests
-    // marked as renderer-initiated are created by receiving a BeginNavigation
-    // IPC, and will then proceed in the same renderer. In site-per-process
-    // mode, it is possible for renderer-intiated navigations to be allowed to
-    // go cross-process. Check it first.
-    bool can_renderer_initiate_transfer =
-        render_frame_host_->IsRenderFrameLive() &&
-        ShouldMakeNetworkRequestForURL(request.common_params().url) &&
-        IsRendererTransferNeededForNavigation(render_frame_host_.get(),
-                                              request.common_params().url);
+  // First compute the SiteInstance to use for the navigation.
+  SiteInstance* current_site_instance = render_frame_host_->GetSiteInstance();
+  scoped_refptr<SiteInstance> dest_site_instance =
+      GetSiteInstanceForNavigationRequest(request);
 
-    no_renderer_swap |=
-        !request.may_transfer() && !can_renderer_initiate_transfer;
-  } else {
-    // Subframe navigations will use the current renderer, unless specifically
-    // allowed to swap processes.
-    no_renderer_swap |= !CanSubframeSwapProcess(
-        request.common_params().url, request.source_site_instance(),
-        request.dest_site_instance(), was_server_redirect);
-  }
+  // The SiteInstance determines whether to switch RenderFrameHost or not.
+  bool use_current_rfh = current_site_instance == dest_site_instance;
 
   bool notify_webui_of_rf_creation = false;
-  if (no_renderer_swap) {
+  if (use_current_rfh) {
     // GetFrameHostForNavigation will be called more than once during a
     // navigation (currently twice, on request and when it's about to commit in
     // the renderer). In the follow up calls an existing pending WebUI should
@@ -1973,6 +1936,60 @@ bool RenderFrameHostManager::InitRenderView(
   return created;
 }
 
+scoped_refptr<SiteInstance>
+RenderFrameHostManager::GetSiteInstanceForNavigationRequest(
+    const NavigationRequest& request) {
+  // First, check if the navigation can switch SiteInstances. If not, the
+  // navigation should use the current SiteInstance.
+  SiteInstance* current_site_instance = render_frame_host_->GetSiteInstance();
+  bool no_renderer_swap_allowed = false;
+  bool was_server_redirect = request.navigation_handle() &&
+                             request.navigation_handle()->WasServerRedirect();
+
+  if (frame_tree_node_->IsMainFrame()) {
+    // Renderer-initiated main frame navigations that may require a
+    // SiteInstance swap are sent to the browser via the OpenURL IPC and are
+    // afterwards treated as browser-initiated navigations. NavigationRequests
+    // marked as renderer-initiated are created by receiving a BeginNavigation
+    // IPC, and will then proceed in the same renderer. In site-per-process
+    // mode, it is possible for renderer-intiated navigations to be allowed to
+    // go cross-process. Check it first.
+    bool can_renderer_initiate_transfer =
+        render_frame_host_->IsRenderFrameLive() &&
+        ShouldMakeNetworkRequestForURL(request.common_params().url) &&
+        IsRendererTransferNeededForNavigation(render_frame_host_.get(),
+                                              request.common_params().url);
+
+    no_renderer_swap_allowed |=
+        !request.may_transfer() && !can_renderer_initiate_transfer;
+  } else {
+    // Subframe navigations will use the current renderer, unless specifically
+    // allowed to swap processes.
+    no_renderer_swap_allowed |= !CanSubframeSwapProcess(
+        request.common_params().url, request.source_site_instance(),
+        request.dest_site_instance(), was_server_redirect);
+  }

[Charlie Reis, 2017/06/27 18:46:35]

Interesting that we're changing the order here and doing this first.  It's a
behavior change, but I think it's an improvement-- it means we won't create the
candidate_site_instance if it isn't necessary.

(Side note: I hope we can revisit this block in the future, since it's really
hard to follow and could easily have unintended behavior.)

[clamy, 2017/06/28 14:08:32]

I thought it made more sense to re-order it :). I agree that the block is hard
to read. When PlzNavigate launches and we remove the old navigation code path,
we might have the opportunity to improve it.

[Charlie Reis, 2017/06/28 17:05:22]

Acknowledged.

+
+  if (no_renderer_swap_allowed)
+    return scoped_refptr<SiteInstance>(current_site_instance);
+
+  // If the navigation can swap SiteInstances, compute the SiteInstance it
+  // should use.
+  SiteInstance* candidate_site_instance =
+      speculative_render_frame_host_
+          ? speculative_render_frame_host_->GetSiteInstance()
+          : nullptr;
+
+  scoped_refptr<SiteInstance> dest_site_instance = GetSiteInstanceForNavigation(
+      request.common_params().url, request.source_site_instance(),
+      request.dest_site_instance(), candidate_site_instance,
+      request.common_params().transition,
+      request.restore_type() != RestoreType::NONE, request.is_view_source(),
+      was_server_redirect);
+
+  return dest_site_instance;
+}
+
 bool RenderFrameHostManager::InitRenderFrame(
     RenderFrameHostImpl* render_frame_host) {
   if (render_frame_host->IsRenderFrameLive())