PlzNavigate: Avoid duplicate SiteInstance creation during navigation.

content/browser/frame_host/render_frame_host_manager.cc

 // Copyright 2013 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/render_frame_host_manager.h"
 
 #include <utility>
 
 #include "base/command_line.h"
 #include "base/logging.h"
@@ skipping 725 matching lines @@
 }
 
 // PlzNavigate
 RenderFrameHostImpl* RenderFrameHostManager::GetFrameHostForNavigation(
     const NavigationRequest& request) {
   CHECK(base::CommandLine::ForCurrentProcess()->HasSwitch(
       switches::kEnableBrowserSideNavigation));
 
   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;
 
   // Renderer-initiated 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 that sent the IPC due to the condition
   // below.
   // TODO(carlosk): Once there is support for cross-process scripting check for
@@ skipping 125 matching lines @@
     case NOTIFICATION_RENDERER_PROCESS_CLOSING:
       RendererProcessClosing(
           Source<RenderProcessHost>(source).ptr());
       break;
 
     default:
       NOTREACHED();
   }
 }
 
+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,
     FrameTreeNode* node) {
   RenderFrameProxyHostMap::iterator iter =
       node->render_manager()->proxy_hosts_.find(site_instance_id);
   if (iter != node->render_manager()->proxy_hosts_.end()) {
     RenderFrameProxyHost* proxy = iter->second;
     // Delete the proxy.  If it is for a main frame (and thus the RFH is stored
     // in the proxy) and it was still pending swap out, move the RFH to the
@@ skipping 112 matching lines @@
       (WebUIControllerFactoryRegistry::GetInstance()->GetWebUIType(
           controller.GetBrowserContext(), current_entry->GetURL()) ==
        WebUIControllerFactoryRegistry::GetInstance()->GetWebUIType(
           controller.GetBrowserContext(), new_url));
 }
 
 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
   // process-per-tab model, such as WebUI pages.
   // TODO(clamy): Remove the dependency on the current entry.
   const NavigationEntry* current_entry =
       delegate_->GetLastCommittedNavigationEntryForRenderManager();
   BrowserContext* browser_context =
       delegate_->GetControllerForRenderManager().GetBrowserContext();
   const GURL& current_effective_url = current_entry ?
       SiteInstanceImpl::GetEffectiveURL(browser_context,
                                         current_entry->GetURL()) :
       render_frame_host_->GetSiteInstance()->GetSiteURL();
   bool current_is_view_source_mode = current_entry ?
       current_entry->IsViewSourceMode() : dest_is_view_source_mode;
   bool force_swap = ShouldSwapBrowsingInstancesForNavigation(
       current_effective_url,
       current_is_view_source_mode,
       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.  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;
+  // 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);
+    }
+  }
+
+  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,
     SiteInstance* dest_instance,
     ui::PageTransition transition,
     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();
 
   // If the entry has an instance already we should use it.
   if (dest_instance) {
     // If we are forcing a swap, this should be in a different BrowsingInstance.
     if (force_browsing_instance_swap) {
       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.
   //
   // NOTE: This can be removed once we have a way to transition between
   //       RenderViews in response to a link click.
   //
   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
     // actually commits. This is to support better process sharing in case
     // the site redirects to some other site: we want to use the destination
     // site in the site instance.
     //
     // In the case of session restore, as it loads all the pages immediately
     // we need to set the site first, otherwise after a restore none of the
     // pages would share renderers in process-per-site.
     //
     // The embedder can request some urls never to be assigned to SiteInstance
     // through the ShouldAssignSiteForURL() content client method, so that
     // renderers created for particular chrome urls (e.g. the chrome-native://
     // scheme) can be reused for subsequent navigations in the same WebContents.
     // 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.
 
   // TODO(creis): Once we intercept links and script-based navigations, we
   // 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_) {
     // The interstitial is currently the last committed entry, but we want to
     // compare against the last non-interstitial entry.
     current_entry = controller.GetEntryAtOffset(-1);
   }
 
   // View-source URLs must use a new SiteInstance and BrowsingInstance.
   // We don't need a swap when going from view-source to a debug URL like
   // chrome://crash, however.
   // TODO(creis): Refactor this method so this duplicated code isn't needed.
   // See http://crbug.com/123007.
   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,
   // because the content is then controlled and/or scriptable by the source
   // 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(
     SiteInstance* current_instance, NavigationEntry* current_entry) {
   // If this is a subframe that is potentially out of process from its parent,
   // don't consider using current_entry's url for SiteInstance selection, since
   // current_entry's url is for the main frame and may be in a different site
   // than this frame.
   // TODO(creis): Remove this when we can check the FrameNavigationEntry's url.
   // See http://crbug.com/369654
@@ skipping 544 matching lines @@
   // If we are currently navigating cross-process, we want to get back to normal
   // and then navigate as usual.
   if (cross_navigation_pending_) {
     if (pending_render_frame_host_)
       CancelPending();
     cross_navigation_pending_ = false;
   }
 
   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 =
       delegate_->GetLastCommittedNavigationEntryForRenderManager();
 
   DCHECK(!cross_navigation_pending_);
 
   if (new_instance.get() != current_instance) {
     TRACE_EVENT_INSTANT2(
         "navigation",
@@ skipping 204 matching lines @@
 void RenderFrameHostManager::DeleteRenderFrameProxyHost(
     SiteInstance* instance) {
   RenderFrameProxyHostMap::iterator iter = proxy_hosts_.find(instance->GetId());
   if (iter != proxy_hosts_.end()) {
     delete iter->second;
     proxy_hosts_.erase(iter);
   }
 }
 
 }  // namespace content