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 23 matching lines @@
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/notification_types.h"
 #include "content/public/browser/render_widget_host_iterator.h"
 #include "content/public/browser/render_widget_host_view.h"
 #include "content/public/browser/user_metrics.h"
 #include "content/public/browser/web_ui_controller.h"
 #include "content/public/common/content_switches.h"
 #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
 bool RenderFrameHostManager::ClearRFHsPendingShutdown(FrameTreeNode* node) {
   node->render_manager()->pending_delete_hosts_.clear();
   return true;
 }
 
 RenderFrameHostManager::RenderFrameHostManager(
     FrameTreeNode* frame_tree_node,
@@ skipping 680 matching lines @@
   RenderFrameHostImpl* dest_rfh = GetFrameHostForNavigation(request);
   DCHECK(dest_rfh);
 }
 
 // PlzNavigate
 RenderFrameHostImpl* RenderFrameHostManager::GetFrameHostForNavigation(
     const NavigationRequest& request) {
   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();
+  RenderFrameHostToUse rfh_to_use = INVALID;
+  scoped_refptr<SiteInstance> dest_site_instance =
+      ChooseFrameHostForNavigation(request, &rfh_to_use);
 
-    // 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());
+  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()));
 
   // If the RenderFrame that needs to navigate is not live (its process was just
   // created or has crashed), initialize it.
   if (!navigation_rfh->IsRenderFrameLive()) {
     // Recreate the opener chain.
     int opener_route_id = delegate_->CreateOpenerRenderViewsForRenderManager(
         navigation_rfh->GetSiteInstance());
@@ skipping 190 matching lines @@
     const GURL& new_url) const {
   NavigationControllerImpl& controller =
       delegate_->GetControllerForRenderManager();
   return current_entry && web_ui_.get() &&
       (WebUIControllerFactoryRegistry::GetInstance()->GetWebUIType(
           controller.GetBrowserContext(), current_entry->GetURL()) ==
        WebUIControllerFactoryRegistry::GetInstance()->GetWebUIType(
           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,
     SiteInstance* dest_instance,
     ui::PageTransition transition,
     bool dest_is_restore,
     bool dest_is_view_source_mode) {
+  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.

+
+  BrowserContext* browser_context =
+      delegate_->GetControllerForRenderManager().GetBrowserContext();
+  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));
+  }
+
+  return false;
+}
+
+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();
-  SiteInstance* new_instance = current_instance;
+  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))
-    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);
+  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_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_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;
 }
 
-SiteInstance* RenderFrameHostManager::GetSiteInstanceForURL(
+SiteInstanceDescriptor
+RenderFrameHostManager::DetermineSiteInstanceForURLInternal(
     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);
 
   // (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);
     }
 
     // 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
     // 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);
   }

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

There's this |current_entry| change here in case the last committed entry is an
interstitial. As the current entry is used before in
DetermineSiteInstanceForURL, is this special treatment also needed there?

 
   // 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);
   }
 
   // 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);
 }
 
 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 769 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