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 dest_url,
+    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, dest_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
   // 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 new_instance_descriptor =
+      SiteInstanceDescriptor(current_instance);
   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);
+    new_instance_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.
+  SiteInstance* new_instance =
+      ConvertToSiteInstance(new_instance_descriptor, candidate_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)
     CHECK_NE(new_instance, current_instance);
   return new_instance;
 }
 
-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
   // 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
   // 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.)
     //
     // 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
     // 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
   // 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
   // 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
   // SiteInstance to a RenderViewHost (if it is different than our current
   // 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) {
+  SiteInstance* current_instance = render_frame_host_->GetSiteInstance();
+
+  // Note: If the |candidate_instance| matches the descriptor, it will already
+  // be set to |descriptor.existing_site_instance|.
+  if (descriptor.existing_site_instance)
+    return descriptor.existing_site_instance;
+
+  // Note: If the |candidate_instance| matches the descriptor,
+  // GetRelatedSiteInstance will return it.
+  if (descriptor.new_is_related_to_current)
+    return current_instance->GetRelatedSiteInstance(descriptor.new_site_url);
+
+  if (candidate_instance &&

[Charlie Reis, 2015/04/02 21:23:25]

nit: Add a comment saying that at this point, we need to return an unrelated
SiteInstance for |new_site_url|.  That makes this check and the code below it
easy to follow.

[carlosk, 2015/04/03 16:11:19]

Done.

+      !current_instance->IsRelatedSiteInstance(candidate_instance) &&
+      candidate_instance->GetSiteURL() == descriptor.new_site_url) {
+    return candidate_instance;
+  }
+
+  return SiteInstance::CreateForURL(
+      delegate_->GetControllerForRenderManager().GetBrowserContext(),
+      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