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 29771b2ab65da73c748a60643a0634acce2ec5a2..07625382111530cddf6b1505de9bb77f5821f635 100644
--- a/content/browser/frame_host/render_frame_host_manager.cc
+++ b/content/browser/frame_host/render_frame_host_manager.cc
@@ -743,11 +743,18 @@ RenderFrameHostImpl* RenderFrameHostManager::GetFrameHostForNavigation(
 
   SiteInstance* current_site_instance = render_frame_host_->GetSiteInstance();
 
+  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(), request.common_params().transition,
+      request.dest_site_instance(), candidate_site_instance,
+      request.common_params().transition,
       request.restore_type() != NavigationEntryImpl::RESTORE_NONE,
       request.is_view_source());
+
   // The appropriate RenderFrameHost to commit the navigation.
   RenderFrameHostImpl* navigation_rfh = nullptr;
 
@@ -893,6 +900,15 @@ void RenderFrameHostManager::Observe(
   }
 }
 
+RenderFrameHostManager::SiteInstanceDescriptor::SiteInstanceDescriptor(
+    GURL site_url,

[Charlie Reis, 2015/03/31 06:18:18]

This is the wrong name if it's not already the Site URL.  Perhaps url or
dest_url would be better.

[carlosk, 2015/03/31 16:38:19]

Done.

+    bool related_to_current,
+    BrowserContext* browser_context)
+    : existing_site_instance(nullptr),
+      new_is_related_to_current(related_to_current) {
+  new_site_url = SiteInstanceImpl::GetSiteForURL(browser_context, site_url);
+}
+
 // static
 bool RenderFrameHostManager::ClearProxiesInSiteInstance(
     int32 site_instance_id,
@@ -1025,17 +1041,17 @@ SiteInstance* RenderFrameHostManager::GetSiteInstanceForNavigation(
     const GURL& dest_url,
     SiteInstance* source_instance,
     SiteInstance* dest_instance,
+    SiteInstance* candidate_instance,
     ui::PageTransition transition,
     bool dest_is_restore,
     bool dest_is_view_source_mode) {
   SiteInstance* current_instance = render_frame_host_->GetSiteInstance();
-  SiteInstance* new_instance = current_instance;
 
   // We do not currently swap processes for navigations in webview tag guests.
   if (current_instance->GetSiteURL().SchemeIs(kGuestScheme))
     return current_instance;
 
-  // Determine if we need a new BrowsingInstance for this entry.  If true, this

[Charlie Reis, 2015/03/31 06:18:18]

nit: Please do not add churn on these lines just to remove a space.  Same below.

[carlosk, 2015/03/31 16:38:19]

I reverted it here but as below I'm also adding further clarifications I let the
change as is.

[Charlie Reis, 2015/03/31 20:19:58]

That's fine in the cases where you're actually changing the comment, but there's
a bunch of comments below where the only change is the removal of a space. 
Please remove that unnecessary churn.  (To make this concrete, it matters when
we're digging through source history to find why a comment was added.  It's
distracting to have to sift through unrelated CLs that simply change spacing.)

[carlosk, 2015/04/01 15:25:29]

Sorry about that, I forgot about those. The plain space removals are now
reverted.

+  // Determine if we need a new BrowsingInstance for this entry. If true, this
   // implies that it will get a new SiteInstance (and likely process), and that
   // other tabs in the current BrowsingInstance will be unable to script it.
   // This is used for cases that require a process swap even in the
@@ -1057,21 +1073,30 @@ SiteInstance* RenderFrameHostManager::GetSiteInstanceForNavigation(
       dest_instance,
       SiteInstanceImpl::GetEffectiveURL(browser_context, dest_url),
       dest_is_view_source_mode);
+  SiteInstanceDescriptor descriptor = SiteInstanceDescriptor(current_instance);

[Charlie Reis, 2015/03/31 06:18:18]

descriptor isn't a useful name, since it's not clear which one it's for. 
new_instance or new_instance_descriptor would be better, as before.

[carlosk, 2015/03/31 16:38:19]

Done.

   if (ShouldTransitionCrossSite() || force_swap) {
-    new_instance = GetSiteInstanceForURL(
-        dest_url, source_instance, current_instance, dest_instance,
-        transition, dest_is_restore, dest_is_view_source_mode, force_swap);
+    descriptor = DetermineSiteInstanceForURL(
+        dest_url, source_instance, current_instance, dest_instance, transition,
+        dest_is_restore, dest_is_view_source_mode, force_swap);
+  }
+
+  // If |force_swap| is true, we must use a different SiteInstance than the
+  // current one. If we didn't, we would have two RenderFrameHosts in the same
+  // SiteInstance and the same frame, resulting in page_id conflicts for their
+  // NavigationEntries.
+  if (force_swap) {
+    if (descriptor.existing_site_instance) {
+      CHECK_NE(current_instance, descriptor.existing_site_instance);
+    } else if (descriptor.new_is_related_to_current) {
+      CHECK_NE(current_instance->GetSiteURL(), descriptor.new_site_url);
+    }
   }
 
-  // If force_swap is true, we must use a different SiteInstance.  If we didn't,
-  // we would have two RenderFrameHosts in the same SiteInstance and the same
-  // frame, resulting in page_id conflicts for their NavigationEntries.
-  if (force_swap)
-    CHECK_NE(new_instance, current_instance);
-  return new_instance;
+  return ConvertToSiteInstance(descriptor, candidate_instance);

[Charlie Reis, 2015/03/31 06:18:18]

If we put the ConvertToSiteInstance call earlier (line 1083), could we leave the
force_swap CHECK as it was originally (doing a pointer compare as you suggest)?

[carlosk, 2015/03/31 16:38:19]

Done.

 }
 
-SiteInstance* RenderFrameHostManager::GetSiteInstanceForURL(
+RenderFrameHostManager::SiteInstanceDescriptor
+RenderFrameHostManager::DetermineSiteInstanceForURL(
     const GURL& dest_url,
     SiteInstance* source_instance,
     SiteInstance* current_instance,
@@ -1080,6 +1105,8 @@ SiteInstance* RenderFrameHostManager::GetSiteInstanceForURL(
     bool dest_is_restore,
     bool dest_is_view_source_mode,
     bool force_browsing_instance_swap) {
+  SiteInstanceImpl* current_instance_impl =
+      static_cast<SiteInstanceImpl*>(current_instance);
   NavigationControllerImpl& controller =
       delegate_->GetControllerForRenderManager();
   BrowserContext* browser_context = controller.GetBrowserContext();
@@ -1091,18 +1118,18 @@ SiteInstance* RenderFrameHostManager::GetSiteInstanceForURL(
       CHECK(!dest_instance->IsRelatedSiteInstance(
                 render_frame_host_->GetSiteInstance()));
     }
-    return dest_instance;
+    return SiteInstanceDescriptor(dest_instance);
   }
 
   // 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 SiteInstanceDescriptor(dest_url, false, browser_context);
 
   // (UGLY) HEURISTIC, process-per-site only:
   //
   // If this navigation is generated, then it probably corresponds to a search
-  // query.  Given that search results typically lead to users navigating to
+  // query. Given that search results typically lead to users navigating to
   // other sites, we don't really want to use the search engine hostname to
   // determine the site instance for this navigation.
   //
@@ -1112,55 +1139,52 @@ SiteInstance* RenderFrameHostManager::GetSiteInstanceForURL(
   if (base::CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kProcessPerSite) &&
       ui::PageTransitionCoreTypeIs(transition, ui::PAGE_TRANSITION_GENERATED)) {
-    return current_instance;
+    return SiteInstanceDescriptor(current_instance_impl);
   }
 
-  SiteInstanceImpl* current_site_instance =
-      static_cast<SiteInstanceImpl*>(current_instance);
-
   // If we haven't used our SiteInstance (and thus RVH) yet, then we can use it
-  // for this entry.  We won't commit the SiteInstance to this site until the
+  // for this entry. We won't commit the SiteInstance to this site until the
   // navigation commits (in DidNavigate), unless the navigation entry was
   // restored or it's a Web UI as described below.
-  if (!current_site_instance->HasSite()) {
+  if (!current_instance_impl->HasSite()) {
     // If we've already created a SiteInstance for our destination, we don't
-    // want to use this unused SiteInstance; use the existing one.  (We don't
-    // do this check if the current_instance has a site, because for now, we
-    // want to compare against the current URL and not the SiteInstance's site.
-    // In this case, there is no current URL, so comparing against the site is
-    // ok.  See additional comments below.)
+    // want to use this unused SiteInstance; use the existing one. (We don't
+    // do this check if the current_instance_impl has a site, because for now,
+    // we want to compare against the current URL and not the SiteInstance's
+    // site. In this case, there is no current URL, so comparing against the
+    // site is ok. See additional comments below.)
     //
     // Also, if the URL should use process-per-site mode and there is an
-    // existing process for the site, we should use it.  We can call
+    // existing process for the site, we should use it. We can call
     // GetRelatedSiteInstance() for this, which will eagerly set the site and
     // thus use the correct process.
     bool use_process_per_site =
         RenderProcessHost::ShouldUseProcessPerSite(browser_context, dest_url) &&
         RenderProcessHostImpl::GetProcessHostForSite(browser_context, dest_url);
-    if (current_site_instance->HasRelatedSiteInstance(dest_url) ||
+    if (current_instance_impl->HasRelatedSiteInstance(dest_url) ||
         use_process_per_site) {
-      return current_site_instance->GetRelatedSiteInstance(dest_url);
+      return SiteInstanceDescriptor(dest_url, true, browser_context);
     }
 
     // For extensions, Web UI URLs (such as the new tab page), and apps we do
-    // not want to use the current_instance if it has no site, since it will
-    // have a RenderProcessHost of PRIV_NORMAL.  Create a new SiteInstance for
-    // this URL instead (with the correct process type).
-    if (current_site_instance->HasWrongProcessForURL(dest_url))
-      return current_site_instance->GetRelatedSiteInstance(dest_url);
+    // not want to use the current_instance_impl if it has no site, since it
+    // will have a RenderProcessHost of PRIV_NORMAL. Create a new SiteInstance
+    // for this URL instead (with the correct process type).
+    if (current_instance_impl->HasWrongProcessForURL(dest_url))
+      return SiteInstanceDescriptor(dest_url, true, browser_context);
 
     // View-source URLs must use a new SiteInstance and BrowsingInstance.
     // TODO(nasko): This is the same condition as later in the function. This
     // 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 SiteInstanceDescriptor(dest_url, false, browser_context);
 
     // 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 SiteInstanceDescriptor(dest_url, false, browser_context);
     }
 
     // Normally the "site" on the SiteInstance is set lazily when the load
@@ -1179,10 +1203,10 @@ SiteInstance* RenderFrameHostManager::GetSiteInstanceForURL(
     // See http://crbug.com/386542.
     if (dest_is_restore &&
         GetContentClient()->browser()->ShouldAssignSiteForURL(dest_url)) {
-      current_site_instance->SetSite(dest_url);
+      current_instance_impl->SetSite(dest_url);
     }
 
-    return current_site_instance;
+    return SiteInstanceDescriptor(current_instance_impl);
   }
 
   // Otherwise, only create a new SiteInstance for a cross-site navigation.
@@ -1191,9 +1215,9 @@ SiteInstance* RenderFrameHostManager::GetSiteInstanceForURL(
   // will be able to enforce that all entries in a SiteInstance actually have
   // the same site, and it will be safe to compare the URL against the
   // SiteInstance's site, as follows:
-  // const GURL& current_url = current_instance->site();
+  // const GURL& current_url = current_instance_impl->site();
   // For now, though, we're in a hybrid model where you only switch
-  // SiteInstances if you type in a cross-site URL.  This means we have to
+  // SiteInstances if you type in a cross-site URL. This means we have to
   // compare the entry's URL to the last committed entry's URL.
   NavigationEntry* current_entry = controller.GetLastCommittedEntry();
   if (interstitial_page_) {
@@ -1210,7 +1234,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 SiteInstanceDescriptor(dest_url, false, browser_context);
   }
 
   // Use the source SiteInstance in case of data URLs or about:blank pages,
@@ -1218,25 +1242,61 @@ SiteInstance* RenderFrameHostManager::GetSiteInstanceForURL(
   // SiteInstance.
   GURL about_blank(url::kAboutBlankURL);
   if (source_instance &&
-      (dest_url == about_blank || dest_url.scheme() == url::kDataScheme))
-    return source_instance;
+      (dest_url == about_blank || dest_url.scheme() == url::kDataScheme)) {
+    return SiteInstanceDescriptor(source_instance);
+  }
 
   // Use the current SiteInstance for same site navigations, as long as the
-  // process type is correct.  (The URL may have been installed as an app since
+  // process type is correct. (The URL may have been installed as an app since
   // the last time we visited it.)
   const GURL& current_url =
-      GetCurrentURLForSiteInstance(current_instance, current_entry);
+      GetCurrentURLForSiteInstance(current_instance_impl, current_entry);
   if (SiteInstance::IsSameWebSite(browser_context, current_url, dest_url) &&
-      !current_site_instance->HasWrongProcessForURL(dest_url)) {
-    return current_instance;
+      !current_instance_impl->HasWrongProcessForURL(dest_url)) {
+    return SiteInstanceDescriptor(current_instance_impl);
   }
 
   // Start the new renderer in a new SiteInstance, but in the current
-  // BrowsingInstance.  It is important to immediately give this new
+  // BrowsingInstance. It is important to immediately give this new
   // SiteInstance to a RenderViewHost (if it is different than our current
-  // SiteInstance), so that it is ref counted.  This will happen in
+  // SiteInstance), so that it is ref counted. This will happen in
   // CreateRenderView.
-  return current_instance->GetRelatedSiteInstance(dest_url);
+  return SiteInstanceDescriptor(dest_url, true, browser_context);
+}
+
+SiteInstance* RenderFrameHostManager::ConvertToSiteInstance(
+    const SiteInstanceDescriptor& descriptor,
+    SiteInstance* candidate_instance) {
+  BrowserContext* browser_context =

[Charlie Reis, 2015/03/31 06:18:18]

We don't need this until the last line, and most of the branches don't need it
at all.  Let's move it to the end.

[carlosk, 2015/03/31 16:38:19]

Done.

+      delegate_->GetControllerForRenderManager().GetBrowserContext();
+  SiteInstance* current_instance = render_frame_host_->GetSiteInstance();
+
+  if (candidate_instance) {

[Charlie Reis, 2015/03/31 06:18:18]

This control flow seems odd to me.  The only case that really matters in this
block is returning candidate_instance if neither it nor descriptor are related
to the current and if their sites match.

The existing_site_instance case is handled fine by line 1292, so at least we can
move that up before this block.

In the is_related case, GetRelatedSiteInstance will return candidate_instance if
their sites match, so that seems like it's covered by line 1295.  (Note: there's
a race possible where that might not be true, as noted on site_instance_map_ in
browsing_instance.h.  However, I don't think that race is necessary in
PlzNavigate, since we should always be able to assign sites in advance.  That's
something we should aim for if possible.)

[carlosk, 2015/03/31 16:38:19]

Yes, I didn't notice the coincidences in logic that you pointed out here. I
updated accordingly and added notes to clarify the logic in regards to the
candidate.

I'm not sure about the race conditions but it seems to me that they won't happen
as all SiteInstance manipulations in PlzNavigate are handled (serially) by the
UI thread.

+    if (descriptor.existing_site_instance &&
+        descriptor.existing_site_instance == candidate_instance) {
+      return candidate_instance;
+    }
+    bool candidate_is_related =
+        render_frame_host_->GetSiteInstance()->IsRelatedSiteInstance(

[Charlie Reis, 2015/03/31 06:18:18]

You've already declared current_instance, so there's no need to call
GetSiteInstance() again.

[carlosk, 2015/03/31 16:38:19]

Done.

+            candidate_instance);
+    if (descriptor.new_is_related_to_current == candidate_is_related &&
+        candidate_instance->GetSiteURL() == descriptor.new_site_url) {
+      // At this point there's two possibilities a) the new site should be
+      // related and the candidate effectively is or b) the new site should not
+      // be related and the candidate is not. In either case we need to verify
+      // if the site URL matches the candidate's.
+      return candidate_instance;
+    }
+  }
+
+  if (descriptor.existing_site_instance)
+    return descriptor.existing_site_instance;
+
+  if (descriptor.new_is_related_to_current) {

[Charlie Reis, 2015/03/31 06:18:19]

No braces.

[carlosk, 2015/03/31 16:38:19]

Done.

+    return current_instance->GetRelatedSiteInstance(descriptor.new_site_url);
+  }
+
+  return SiteInstance::CreateForURL(browser_context, descriptor.new_site_url);
 }
 
 const GURL& RenderFrameHostManager::GetCurrentURLForSiteInstance(
@@ -1801,7 +1861,7 @@ RenderFrameHostImpl* RenderFrameHostManager::UpdateStateForNavigate(
 
   SiteInstance* current_instance = render_frame_host_->GetSiteInstance();
   scoped_refptr<SiteInstance> new_instance = GetSiteInstanceForNavigation(
-      dest_url, source_instance, dest_instance, transition,
+      dest_url, source_instance, dest_instance, nullptr, transition,
       dest_is_restore, dest_is_view_source_mode);
 
   const NavigationEntry* current_entry =