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 3344f3dab66dccb585fde4aa1d2720e9838b5bc5..62f94fa002367b1a35f37f0fead3df674180a045 100644
--- a/content/browser/frame_host/render_frame_host_manager.cc
+++ b/content/browser/frame_host/render_frame_host_manager.cc
@@ -41,6 +41,32 @@
 #include "content/public/common/referrer.h"
 #include "content/public/common/url_constants.h"
 
+namespace {
+
+// Stores information regarding a SiteInstance targeted at a specific URL. It
+// can whether point to an existent one or store the details needed to create a
+// new one.
+struct SiteInstanceDescriptor {
+  explicit SiteInstanceDescriptor(content::SiteInstance* site_instance)
+      : existent_site_instance(site_instance),
+        new_site_is_related_to_current(false) {}
+
+  SiteInstanceDescriptor(GURL site_url, bool related_to_current)
+      : existent_site_instance(nullptr),
+        new_site_url(site_url),
+        new_site_is_related_to_current(related_to_current) {}
+
+  // Set with an existent SiteInstance to be reused.
+  content::SiteInstance* existent_site_instance;
+  // In case |existent_site_instance| is null, specify a new site URL.
+  GURL new_site_url;
+  // In case |existent_site_instance| is null, specify if the new site should be
+  // created in new BrowsingInstance or not.
+  bool new_site_is_related_to_current;
+};
+
+}  // namespace
+
 namespace content {
 
 // static
@@ -741,76 +767,63 @@ RenderFrameHostImpl* RenderFrameHostManager::GetFrameHostForNavigation(
   CHECK(base::CommandLine::ForCurrentProcess()->HasSwitch(
       switches::kEnableBrowserSideNavigation));
 
-  SiteInstance* current_site_instance = render_frame_host_->GetSiteInstance();
-
-  scoped_refptr<SiteInstance> dest_site_instance = GetSiteInstanceForNavigation(
-      request.common_params().url, request.source_site_instance(),
-      request.dest_site_instance(), request.common_params().transition,
-      request.restore_type() != NavigationEntryImpl::RESTORE_NONE,
-      request.is_view_source());
-  // The appropriate RenderFrameHost to commit the navigation.
+  // To be set with the (currently) appropriate RenderFrameHost to commit this
+  // 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
-  // non-browser-initiated navigations should be removed (see crbug.com/440266).
-  if (current_site_instance == dest_site_instance.get() ||
-      !request.browser_initiated() ||
-      (!frame_tree_node_->IsMainFrame() &&
-       !base::CommandLine::ForCurrentProcess()->HasSwitch(
-           switches::kSitePerProcess))) {
-    // Reuse the current RFH if its SiteInstance matches the the navigation's
-    // or if this is a subframe navigation. We only swap RFHs for subframes when
-    // --site-per-process is enabled.
-    CleanUpNavigation();
-    navigation_rfh = render_frame_host_.get();
-
-    // As SiteInstances are the same, check if the WebUI should be reused.
-    const NavigationEntry* current_navigation_entry =
-        delegate_->GetLastCommittedNavigationEntryForRenderManager();
-    bool should_reuse_web_ui_ = ShouldReuseWebUI(current_navigation_entry,
-                                                 request.common_params().url);

[carlosk, 2015/03/23 14:31:18]

I fixed this bug (this is the 2nd time I think) of re-defining
|should_reuse_web_ui_| inside GetFrameHostForNavigation. Seemed too simple of a
change to do it in another CL.

-    if (!should_reuse_web_ui_) {
-      speculative_web_ui_ = CreateWebUI(request.common_params().url,
-                                        request.bindings());
-      // Make sure the current RenderViewHost has the right bindings.
-      if (speculative_web_ui() &&
-          !render_frame_host_->GetProcess()->IsIsolatedGuest()) {
-        render_frame_host_->render_view_host()->AllowBindings(
-            speculative_web_ui()->GetBindings());
+  RenderFrameHostToUse rfh_to_use = INVALID;
+  scoped_refptr<SiteInstance> dest_site_instance =
+      ChooseFrameHostForNavigation(request, &rfh_to_use);
+
+  switch (rfh_to_use) {
+    case REUSE_CURRENT: {
+      CleanUpNavigation();
+      navigation_rfh = render_frame_host_.get();
+
+      // As SiteInstances are the same, check if the WebUI should be reused.
+      const NavigationEntry* current_navigation_entry =
+          delegate_->GetLastCommittedNavigationEntryForRenderManager();
+      should_reuse_web_ui_ = ShouldReuseWebUI(current_navigation_entry,
+                                              request.common_params().url);
+      if (!should_reuse_web_ui_) {
+        speculative_web_ui_ =
+            CreateWebUI(request.common_params().url, request.bindings());
+        // Make sure the current RenderViewHost has the right bindings.
+        if (speculative_web_ui() &&
+            !render_frame_host_->GetProcess()->IsIsolatedGuest()) {
+          render_frame_host_->render_view_host()->AllowBindings(
+              speculative_web_ui()->GetBindings());
+        }
       }
+      break;
     }
-  } else {
-    // If the SiteInstance for the final URL doesn't match the one from the
-    // speculatively created RenderFrameHost, create a new RenderFrameHost using
-    // this new SiteInstance.
-    if (!speculative_render_frame_host_ ||
-        speculative_render_frame_host_->GetSiteInstance() !=
-            dest_site_instance.get()) {
+    case NEW_SPECULATIVE: {
+      DCHECK(dest_site_instance);
       CleanUpNavigation();
       bool success = CreateSpeculativeRenderFrameHost(
-          request.common_params().url, current_site_instance,
+          request.common_params().url, render_frame_host_->GetSiteInstance(),
           dest_site_instance.get(), request.bindings());
       DCHECK(success);
+      // fall through
     }
-    DCHECK(speculative_render_frame_host_);
-    navigation_rfh = speculative_render_frame_host_.get();
+    case REUSE_SPECULATIVE: {
+      DCHECK(speculative_render_frame_host_);
+      navigation_rfh = speculative_render_frame_host_.get();
 
-    // Check if our current RFH is live.
-    if (!render_frame_host_->IsRenderFrameLive()) {
-      // The current RFH is not live.  There's no reason to sit around with a
-      // sad tab or a newly created RFH while we wait for the navigation to
-      // complete. Just switch to the speculative RFH now and go back to non
-      // cross-navigating (Note that we don't care about on{before}unload
-      // handlers if the current RFH isn't live.)
-      CommitPending();
+      // Check if our current RFH is live.
+      if (!render_frame_host_->IsRenderFrameLive()) {
+        // The current RFH is not live.  There's no reason to sit around with a
+        // sad tab or a newly created RFH while we wait for the navigation to
+        // complete. Just switch to the speculative RFH now and go back to non
+        // cross-navigating (Note that we don't care about on{before}unload
+        // handlers if the current RFH isn't live.)
+        CommitPending();

[carlosk, 2015/03/23 14:31:18]

This early commit when the current frame is not live might be run *before*
commit time, on a possibly non-final URL. This is a bug, right?

+      }
+      break;
     }
+    default: { NOTREACHED(); }

[carlosk, 2015/03/23 14:31:18]

I already added the adequate line breaks on my machine.

[carlosk, 2015/03/23 17:43:01]

Surprisingly, this is the formating that git cl format enforces.

   }
+
   DCHECK(navigation_rfh &&
          (navigation_rfh == render_frame_host_.get() ||
           navigation_rfh == speculative_render_frame_host_.get()));
@@ -1021,6 +1034,66 @@ bool RenderFrameHostManager::ShouldReuseWebUI(
           controller.GetBrowserContext(), new_url));
 }
 
+// PlzNavigate
+scoped_refptr<SiteInstance>
+RenderFrameHostManager::ChooseFrameHostForNavigation(
+    const NavigationRequest& request,
+    RenderFrameHostToUse* rfh_to_use) {
+  CHECK(base::CommandLine::ForCurrentProcess()->HasSwitch(
+      switches::kEnableBrowserSideNavigation));
+
+  if (speculative_render_frame_host_ &&
+      IsCorrectSiteInstanceForURL(
+          speculative_render_frame_host_->GetSiteInstance(),
+          request.common_params().url, request.source_site_instance(),
+          request.dest_site_instance(), request.common_params().transition,
+          request.restore_type() != NavigationEntryImpl::RESTORE_NONE,
+          request.is_view_source())) {
+    *rfh_to_use = REUSE_SPECULATIVE;
+    return nullptr;
+  }
+
+  SiteInstance* current_site_instance = render_frame_host_->GetSiteInstance();
+  scoped_refptr<SiteInstance> dest_site_instance = GetSiteInstanceForNavigation(
+      request.common_params().url, request.source_site_instance(),
+      request.dest_site_instance(), request.common_params().transition,
+      request.restore_type() != NavigationEntryImpl::RESTORE_NONE,
+      request.is_view_source());
+
+  // 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
+  // non-browser-initiated navigations should be removed (see crbug.com/440266).
+  if (current_site_instance == dest_site_instance.get() ||
+      !request.browser_initiated() ||
+      (!frame_tree_node_->IsMainFrame() &&
+       !base::CommandLine::ForCurrentProcess()->HasSwitch(
+           switches::kSitePerProcess))) {
+    // Reuse the current RFH if its SiteInstance matches the the navigation's,
+    // if it's renderer initiated or if this is a subframe navigation. We only
+    // swap RFHs for subframes when --site-per-process is enabled.
+    *rfh_to_use = REUSE_CURRENT;
+    return nullptr;
+  }
+
+  // If the SiteInstance for the final URL doesn't match the one from the
+  // speculatively created RenderFrameHost, create a new RenderFrameHost using
+  // this new SiteInstance.
+  if (!speculative_render_frame_host_ ||
+      speculative_render_frame_host_->GetSiteInstance() !=
+          dest_site_instance.get()) {
+    *rfh_to_use = NEW_SPECULATIVE;
+    return dest_site_instance;
+  }
+
+  *rfh_to_use = REUSE_SPECULATIVE;
+  return nullptr;
+}
+
 SiteInstance* RenderFrameHostManager::GetSiteInstanceForNavigation(
     const GURL& dest_url,
     SiteInstance* source_instance,
@@ -1028,50 +1101,113 @@ SiteInstance* RenderFrameHostManager::GetSiteInstanceForNavigation(
     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;
+  SiteInstanceDescriptor descriptor = DetermineSiteInstanceForURL(
+      dest_url, source_instance, dest_instance, transition, dest_is_restore,
+      dest_is_view_source_mode);
+
+  if (descriptor.existent_site_instance)
+    return descriptor.existent_site_instance;
+
+  if (descriptor.new_site_is_related_to_current) {
+    return render_frame_host_->GetSiteInstance()->GetRelatedSiteInstance(
+        descriptor.new_site_url);
+  }
+
+  return SiteInstance::CreateForURL(
+      delegate_->GetControllerForRenderManager().GetBrowserContext(),
+      descriptor.new_site_url);
+}
+
+// PlzNavigate
+bool RenderFrameHostManager::IsCorrectSiteInstanceForURL(
+    SiteInstance* candidate_instance,
+    const GURL& dest_url,
+    SiteInstance* source_instance,
+    SiteInstance* dest_instance,
+    ui::PageTransition transition,
+    bool dest_is_restore,
+    bool dest_is_view_source_mode) {
+  CHECK(candidate_instance);
+
+  SiteInstanceDescriptor descriptor = DetermineSiteInstanceForURL(
+      dest_url, source_instance, dest_instance, transition, dest_is_restore,
+      dest_is_view_source_mode);
+
+  if (descriptor.existent_site_instance)
+    return descriptor.existent_site_instance == candidate_instance;

[carlosk, 2015/03/23 14:31:18]

Confirming: there should be no two SiteInstances under the same browsing
instance with the same site URL, correct? IIUC, if two SiteInstances have the
same site URL and the same browsing instance, they should in fact be the exact
same SiteInstance.

 
-  // 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);
-  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);
+  bool candidate_is_related =
+      render_frame_host_->GetSiteInstance()->IsRelatedSiteInstance(
+          candidate_instance);
+
+  // Note: this handles both the cases when a) the new site should be related
+  // and the candidate is and b) the new site should not be related and the
+  // candidate is not. Hence the boolean equality check.
+  if (descriptor.new_site_is_related_to_current == candidate_is_related) {
+    return (candidate_instance->GetSiteURL() ==
+            SiteInstanceImpl::GetSiteForURL(browser_context,
+                                            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 false;
 }
 
-SiteInstance* RenderFrameHostManager::GetSiteInstanceForURL(
+SiteInstanceDescriptor RenderFrameHostManager::DetermineSiteInstanceForURL(

[carlosk, 2015/03/23 14:31:18]

I am returning SiteInstanceDescriptor by value here and in the *Internal method
because it makes everything simpler. The coding style guide doesn't mention
anything against it and even seems to consider it  preferable
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Copyable_Mova...).
The GURL is the only variable sized member we might worry about. WDYT?

[clamy, 2015/03/23 15:26:44]

Seems reasonable.

+    const GURL& dest_url,
+    SiteInstance* source_instance,
+    SiteInstance* dest_instance,
+    ui::PageTransition transition,
+    bool dest_is_restore,
+    bool dest_is_view_source_mode) {
+  SiteInstance* current_instance = render_frame_host_->GetSiteInstance();
+  SiteInstanceDescriptor descriptor = SiteInstanceDescriptor(current_instance);
+  bool force_browsing_instance_swap = false;
+
+  // We do not currently swap processes for navigations in webview tag guests.
+  if (!current_instance->GetSiteURL().SchemeIs(kGuestScheme)) {
+    // 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.
+    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;
+    force_browsing_instance_swap = ShouldSwapBrowsingInstancesForNavigation(
+        current_effective_url, current_is_view_source_mode, dest_instance,
+        SiteInstanceImpl::GetEffectiveURL(browser_context, dest_url),
+        dest_is_view_source_mode);
+    if (ShouldTransitionCrossSite() || force_browsing_instance_swap) {
+      descriptor = DetermineSiteInstanceForURLInternal(
+          dest_url, source_instance, current_instance, dest_instance,
+          transition, dest_is_restore, dest_is_view_source_mode,
+          force_browsing_instance_swap);
+    }
+  }
+
+  // If |force_browsing_instance_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_browsing_instance_swap && descriptor.existent_site_instance)
+    CHECK_NE(current_instance, descriptor.existent_site_instance);
+
+  return descriptor;
+}
+
+SiteInstanceDescriptor
+RenderFrameHostManager::DetermineSiteInstanceForURLInternal(
     const GURL& dest_url,
     SiteInstance* source_instance,
     SiteInstance* current_instance,
@@ -1080,6 +1216,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 +1229,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);
 
   // (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 +1250,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);
     }
 
     // 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);
 
     // 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);
 
     // 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);
     }
 
     // Normally the "site" on the SiteInstance is set lazily when the load
@@ -1179,10 +1314,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 +1326,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 +1345,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);
   }
 
   // Use the source SiteInstance in case of data URLs or about:blank pages,
@@ -1218,25 +1353,26 @@ 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);
 }
 
 const GURL& RenderFrameHostManager::GetCurrentURLForSiteInstance(