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..badae1dbf2e16303787dcdf74f126af9073d7529 100644
--- a/content/browser/frame_host/render_frame_host_manager.cc
+++ b/content/browser/frame_host/render_frame_host_manager.cc
@@ -743,11 +743,18 @@ RenderFrameHostImpl* RenderFrameHostManager::GetFrameHostForNavigation(
 
   SiteInstance* current_site_instance = render_frame_host_->GetSiteInstance();
 
+  SiteInstance* candidate_site_instance =
+      speculative_render_frame_host_
+          ? speculative_render_frame_host_->GetSiteInstance()

[Charlie Reis, 2015/03/25 00:09:25]

Style nit: All of the other uses of the ternary operator in this file put it at
the end of the previous line.  Please stay consistent (unless git cl format
explicitly overrides this).

[carlosk, 2015/03/30 14:37:37]

This has been git-cl-format-ed.

+          : 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;
 
@@ -773,7 +780,7 @@ RenderFrameHostImpl* RenderFrameHostManager::GetFrameHostForNavigation(
     // 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,

[Charlie Reis, 2015/03/25 00:09:25]

Thanks for catching this!  This is a potentially nasty bug.  Please pull it out
into its own CL ASAP.  (In general, please don't mix unrelated bug fixes into
large CLs like this.)

Do you know what the implications are?  Are they specific to PlzNavigate?

[carlosk, 2015/03/30 14:37:37]

It's been pulled into crrev.com/1012863004.

This only affects PlzNavigate. What would happen is that in a same-site
navigation where the current WebUI should be reused, we'd get a crash when
calling CommitPending when trying to get the |speculative_web_ui_|.

I couldn't find any content unit tests that would cause this state though.

+    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,
@@ -1025,53 +1032,114 @@ 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;
+  SiteInstanceDescriptor descriptor = DetermineSiteInstanceForURL(
+      dest_url, source_instance, dest_instance, transition, dest_is_restore,
+      dest_is_view_source_mode);
 
-  // We do not currently swap processes for navigations in webview tag guests.
-  if (current_instance->GetSiteURL().SchemeIs(kGuestScheme))
-    return current_instance;

[Charlie Reis, 2015/03/25 00:09:25]

In many places, we intentionally return early, to make the rest of the function
easier to read and reason about.  For example, it isn't indented as far, and you
don't have to worry about reading further ahead.  Please don't change this.

[carlosk, 2015/03/30 14:37:38]

Done.

-
-  // 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 =

[Charlie Reis, 2015/03/25 00:09:25]

The rest of this function feels like it should be a general utility function
that converts a SiteInstanceDescriptor into a SiteInstance.  That would make it
easier to write unit tests for.

(That might also let us keep GetSiteInstanceForNavigation as it was before,
eliminate DetermineSiteInstanceForURL, and drop the "Internal" from
DetermineSiteInstanceForURLInternal's name.  We would just call the utility
function at each return point of GetSiteInstanceForNavigation.)

[carlosk, 2015/03/30 14:37:37]

I consolidated some methods as you suggested and reverted
GetSiteInstanceForNavigation to something very similar to what it was before.

I am not sure of what you meant with "general utility function". It sounded like
it should be moved out of RFHM but that would require un-privatizing
SiteInstanceDescriptor and you mentioned that would not be a good thing to do.

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

Yes, I meant a private method in RFHM, as you've done.  Thanks.

       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);
+  SiteInstance* current_instance = render_frame_host_->GetSiteInstance();
+
+  if (candidate_instance &&
+      SiteInstanceMatchesDescription(candidate_instance, descriptor)) {
+    return candidate_instance;
+  }
+
+  if (descriptor.existent_site_instance)
+    return descriptor.existent_site_instance;
+
+  if (descriptor.new_site_is_related_to_current) {
+    return current_instance->GetRelatedSiteInstance(descriptor.new_site_url);
+  }
+
+  return SiteInstance::CreateForURL(browser_context, descriptor.new_site_url);
+}
+
+// PlzNavigate
+bool RenderFrameHostManager::SiteInstanceMatchesDescription(

[carlosk, 2015/03/24 15:30:05]

The merge of GetSiteInstanceForNavigation with IsCorrectSiteInstanceForURL was
on the API level but I kept their logic separate (SiteInstanceMatchesDescription
is the new IsCorrectSiteInstanceForURL) because it is simpler to follow it that
way.

+    SiteInstance* candidate_instance,
+    const SiteInstanceDescriptor& descriptor) {
+  CHECK(candidate_instance);
+
+  if (descriptor.existent_site_instance)
+    return descriptor.existent_site_instance == candidate_instance;
+
+  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 equality check between two boolean values.
+  if (descriptor.new_site_is_related_to_current == candidate_is_related) {

[Charlie Reis, 2015/03/25 00:09:25]

This code feels like it's trying to be too clever, which tends to make things
harder to debug.  Let's make an effort to make it easier to understand.

I'd suggest using != here and returning false early.

Then return whether the site URLs match.

[carlosk, 2015/03/30 14:37:37]

As I finally merged this method into the new descriptor conversion method, this
logic was slightly changed and it made more sense to keep the "==".

This is where I think large files worsen the way we code. For better
organization and clarity I'd prefer to keep this logic smaller and in two
separate methods. But as RFHM already has too many methods, adding yet another
one has the opposite effect. Extracting the SiteInstance logic might not be the
right thing to do but we should consider other options.

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

I don't agree with this conclusion.  There are times when it makes sense to
break up a class, but even when we have a large class that doesn't prevent us
from creating helper methods.

+    return (candidate_instance->GetSiteURL() ==
+            SiteInstanceImpl::GetSiteForURL(

[Charlie Reis, 2015/03/25 00:09:25]

This seems wrong to me.  We shouldn't need to call GetSiteForURL here, since we
should only be storing site URLs in SiteInstanceDescriptor.

[carlosk, 2015/03/30 14:37:38]

You meant that this conversion (from destination URL to site URL) should have
already happened when creating the descriptor, right? I updated the code to do
that.

+                delegate_->GetControllerForRenderManager().GetBrowserContext(),
+                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(
+RenderFrameHostManager::SiteInstanceDescriptor
+RenderFrameHostManager::DetermineSiteInstanceForURL(
+    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;

[Charlie Reis, 2015/03/25 00:09:25]

This isn't needed this early.

[carlosk, 2015/03/30 14:37:37]

It needed to be defined here but with the elimination of the if-block (due to
your other comment on returning early) I updated this part accordingly.

+
+  // 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())
+                      : current_instance->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);

[Charlie Reis, 2015/03/25 00:09:25]

This new check is not sufficient.  If descriptor has a URL and is related, it
might match current_instance.

(Even if that's not possible given how the code is currently structured, we
should enforce it in this CHECK to make sure it doesn't regress.  This gives us
a clear way to reason about this point of the code.)

[carlosk, 2015/03/30 14:37:37]

It is indeed a possible outcome and I updated the code to cover it.

But just checking: that would represent an invalid SiteInstance, right? My
undertanding is that if a SI should be related to another one and have the same
site URL they should in fact be the exact same instance so that pointer
comparisons would return true. Hence we being able to pointer-compare
SiteInstances for equality.

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

I was referring to the case that descriptor.existent_site_instance was null but
its site URL referred to current_instance.  That's a valid descriptor but would
indicate a bug in the code.

That said, I agree that we can do a pointer comparison on the SiteInstances if
we convert from the descriptor before the CHECK_NE.

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

Yes, I had understood that.
I was not suggesting a change here but just trying to confirm if that would be a
representation of an invalid SiteInstance as I thought. And it was as I thought.

+
+  return descriptor;
+}
+
+RenderFrameHostManager::SiteInstanceDescriptor
+RenderFrameHostManager::DetermineSiteInstanceForURLInternal(
     const GURL& dest_url,
     SiteInstance* source_instance,
     SiteInstance* current_instance,
@@ -1080,6 +1148,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 +1161,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 +1182,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 +1246,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);

[carlosk, 2015/03/24 15:30:06]

This whole method seems to be mostly read-only (I didn't look through all the
calls from here though) apart from this SetSite here. Is this something we
should change?

[Charlie Reis, 2015/03/25 00:09:25]

Yes, Camille had a CL that ran into this as well.  I don't have time to look it
up right now, but we might be able to revisit this case.

[carlosk, 2015/03/30 14:37:37]

Acknowledged.

     }
 
-    return current_site_instance;
+    return SiteInstanceDescriptor(current_instance_impl);
   }
 
   // Otherwise, only create a new SiteInstance for a cross-site navigation.
@@ -1191,9 +1258,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 +1277,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 +1285,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(
@@ -1801,7 +1869,7 @@ RenderFrameHostImpl* RenderFrameHostManager::UpdateStateForNavigate(
 
   SiteInstance* current_instance = render_frame_host_->GetSiteInstance();
   scoped_refptr<SiteInstance> new_instance = GetSiteInstanceForNavigation(
-      dest_url, source_instance, dest_instance, transition,
+      dest_url, source_instance, dest_instance, nullptr, transition,
       dest_is_restore, dest_is_view_source_mode);
 
   const NavigationEntry* current_entry =