PlzNavigate: Avoid duplicate SiteInstance creation during navigation.

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 f9e2bf976fcbe85f71d53b78f7fbd8fe5bcf37f9..ef083f7a38a6caa3db65599d3034138af3b68135 100644
--- a/content/browser/frame_host/render_frame_host_manager.cc
+++ b/content/browser/frame_host/render_frame_host_manager.cc
@@ -1033,7 +1033,7 @@ SiteInstance* RenderFrameHostManager::GetSiteInstanceForURL(
   // If a swap is required, we need to force the SiteInstance AND
   // BrowsingInstance to be different ones, using CreateForURL.
   if (force_browsing_instance_swap)
-    return SiteInstance::CreateForURL(browser_context, dest_url);
+    return CreateSiteInstanceForURL(browser_context, dest_url);
 
   // (UGLY) HEURISTIC, process-per-site only:
   //
@@ -1090,13 +1090,13 @@ SiteInstance* RenderFrameHostManager::GetSiteInstanceForURL(
     // should be taken into account when refactoring this method as part of
     // http://crbug.com/123007.
     if (dest_is_view_source_mode)
-      return SiteInstance::CreateForURL(browser_context, dest_url);
+      return CreateSiteInstanceForURL(browser_context, dest_url);
 
     // If we are navigating from a blank SiteInstance to a WebUI, make sure we
     // create a new SiteInstance.
     if (WebUIControllerFactoryRegistry::GetInstance()->UseWebUIForURL(
             browser_context, dest_url)) {
-        return SiteInstance::CreateForURL(browser_context, dest_url);
+      return CreateSiteInstanceForURL(browser_context, dest_url);
     }
 
     // Normally the "site" on the SiteInstance is set lazily when the load
@@ -1146,7 +1146,7 @@ SiteInstance* RenderFrameHostManager::GetSiteInstanceForURL(
   if (current_entry &&
       current_entry->IsViewSourceMode() != dest_is_view_source_mode &&
       !IsRendererDebugURL(dest_url)) {
-    return SiteInstance::CreateForURL(browser_context, dest_url);
+    return CreateSiteInstanceForURL(browser_context, dest_url);
   }
 
   // Use the source SiteInstance in case of data URLs or about:blank pages,
@@ -1175,6 +1175,22 @@ SiteInstance* RenderFrameHostManager::GetSiteInstanceForURL(
   return current_instance->GetRelatedSiteInstance(dest_url);
 }
 
+SiteInstance* RenderFrameHostManager::CreateSiteInstanceForURL(
+    BrowserContext* browser_context,
+    const GURL& dest_url) {
+  if (base::CommandLine::ForCurrentProcess()->HasSwitch(
+          switches::kEnableBrowserSideNavigation) &&
+      speculative_render_frame_host_ &&
+      speculative_render_frame_host_->GetSiteInstance()->GetSiteURL() ==

[nasko, 2015/03/02 23:02:09]

This check doesn't seem quite right. The SiteInstance URL is a "site" URL,
therefore it will contain only scheme + registered domain name. The dest_url
could be any URL, including port, path, and query string.

[nasko, 2015/03/02 23:19:47]

If I follow things right, you probably want to call SiteInstance::GetSiteForURL
with the dest_url and compare its output with the GetSiteURL of the speculative
RFH's SiteInstance.

[carlosk, 2015/03/03 13:42:41]

Yes, indeed. This is the right thing to do.

If the final decision is to go on with adding this logic, I'll also add a
specific test to check this is working as expected.

+          dest_url) {
+    // We already created a new SiteInstance for this specific URL for this same
+    // navigation and stored it in the speculative RenderFrameHost, so there's
+    // no need to create a new one again.
+    return speculative_render_frame_host_->GetSiteInstance();
+  }
+  return SiteInstance::CreateForURL(browser_context, dest_url);
+}
+
 const GURL& RenderFrameHostManager::GetCurrentURLForSiteInstance(
     SiteInstance* current_instance, NavigationEntry* current_entry) {
   // If this is a subframe that is potentially out of process from its parent,