Prevent creating a swapped out RVH in the same SiteInstance as the current one.

content/browser/frame_host/render_view_host_manager.cc

Index: content/browser/frame_host/render_view_host_manager.cc
diff --git a/content/browser/frame_host/render_view_host_manager.cc b/content/browser/frame_host/render_view_host_manager.cc
index 34e2a00c10427fb9d6d29ee521b5b52ec81aabb9..9c2c55172c2e734706d2ebbb69d650332c51a5f3 100644
--- a/content/browser/frame_host/render_view_host_manager.cc
+++ b/content/browser/frame_host/render_view_host_manager.cc
@@ -491,36 +491,47 @@ bool RenderViewHostManager::ShouldSwapProcessesForNavigation(
     const NavigationEntryImpl* new_entry) const {
   DCHECK(new_entry);
 
+  // If new_entry already has a SiteInstance, assume it is correct and use it.
+  if (new_entry->site_instance())
+    return false;
+
   // Check for reasons to swap processes even if we are in a process model that
   // doesn't usually swap (e.g., process-per-tab).
 
-  // For security, we should transition between processes when one is a Web UI
-  // page and one isn't.  If there's no curr_entry, check the current RVH's
-  // site, which might already be committed to a Web UI URL (such as the NTP).
-  const GURL& current_url = (curr_entry) ? curr_entry->GetURL() :
-      render_view_host_->GetSiteInstance()->GetSiteURL();
+  // We use the effective URL here, since that's what is used in the
+  // SiteInstance's site and when we later call IsSameWebSite.  If there is no
+  // curr_entry, check the current SiteInstance's site, which might already be
+  // committed to a Web UI URL (such as the NTP).
   BrowserContext* browser_context =
       delegate_->GetControllerForRenderManager().GetBrowserContext();
+  const GURL& current_url = (curr_entry) ?
+      SiteInstanceImpl::GetEffectiveURL(browser_context, curr_entry->GetURL()) :
+      render_view_host_->GetSiteInstance()->GetSiteURL();
+  const GURL& new_url = SiteInstanceImpl::GetEffectiveURL(browser_context,
+                                                          new_entry->GetURL());
+
+  // For security, we should transition between processes when one is a Web UI
+  // page and one isn't.
   if (WebUIControllerFactoryRegistry::GetInstance()->UseWebUIForURL(
           browser_context, current_url)) {
     // Force swap if it's not an acceptable URL for Web UI.
     // Here, data URLs are never allowed.
     if (!WebUIControllerFactoryRegistry::GetInstance()->IsURLAcceptableForWebUI(
-            browser_context, new_entry->GetURL(), false)) {
+            browser_context, new_url, false)) {
       return true;
     }
   } else {
     // Force swap if it's a Web UI URL.
     if (WebUIControllerFactoryRegistry::GetInstance()->UseWebUIForURL(
-            browser_context, new_entry->GetURL())) {
+            browser_context, new_url)) {
       return true;
     }
   }
 
+  // Check with the content client as well.  Important to pass current_url here,
+  // which uses the SiteInstance's site if there is no curr_entry.
   if (GetContentClient()->browser()->ShouldSwapProcessesForNavigation(
-          render_view_host_->GetSiteInstance(),
-          curr_entry ? curr_entry->GetURL() : GURL(),
-          new_entry->GetURL())) {
+          render_view_host_->GetSiteInstance(), current_url, new_url)) {
     return true;
   }
 
@@ -551,14 +562,22 @@ bool RenderViewHostManager::ShouldReuseWebUI(
 
 SiteInstance* RenderViewHostManager::GetSiteInstanceForEntry(
     const NavigationEntryImpl& entry,
-    SiteInstance* curr_instance) {
-  // NOTE: This is only called when ShouldTransitionCrossSite is true.
-
+    SiteInstance* curr_instance,
+    bool force_swap) {
+  // Determine which SiteInstance to use for navigating to |entry|.
   const GURL& dest_url = entry.GetURL();
   NavigationControllerImpl& controller =
       delegate_->GetControllerForRenderManager();
   BrowserContext* browser_context = controller.GetBrowserContext();
 
+  // If a swap is required, we need to force the SiteInstance AND
+  // BrowsingInstance to be different ones, using CreateForURL.

[nasko, 2013/11/05 18:41:00]

Why is that? Are we guaranteed in this case that there are no scripting
relationships we need to maintain?
If so, force_swap is a bit unclear name for this, since we can do process swap
but still be in the same BrowsingInstance, right?

[Charlie Reis, 2013/11/05 19:13:15]

Yep, that's what the poorly named ShouldSwapProcessesForNavigation is actually
deciding.  (Note that the main change to this function is passing in the result
of that call as a parameter and doing the check up front, rather than calling
ShouldSwap again at the end.)
I agree.  I've switched the name of this parameter to
force_browsing_instance_swap, but the rest of the rename (including
ShouldSwapBrowsingInstancesForNavigation) is coming in a followup CL as I revive
https://codereview.chromium.org/10690048/.

+  if (force_swap) {
+    // We shouldn't be forcing a swap if an entry already has a SiteInstance.
+    CHECK(!entry.site_instance());
+    return SiteInstance::CreateForURL(browser_context, dest_url);
+  }
+
   // If the entry has an instance already we should use it.
   if (entry.site_instance())
     return entry.site_instance();
@@ -685,22 +704,14 @@ SiteInstance* RenderViewHostManager::GetSiteInstanceForEntry(
       !static_cast<SiteInstanceImpl*>(curr_instance)->HasWrongProcessForURL(
           dest_url)) {
     return curr_instance;
-  } else if (ShouldSwapProcessesForNavigation(curr_entry, &entry)) {
-    // When we're swapping, we need to force the site instance AND browsing
-    // instance to be different ones. This addresses special cases where we use
-    // a single BrowsingInstance for all pages of a certain type (e.g., New Tab
-    // Pages), keeping them in the same process. When you navigate away from
-    // that page, we want to explicity ignore that BrowsingInstance and group
-    // this page into the appropriate SiteInstance for its URL.
-    return SiteInstance::CreateForURL(browser_context, dest_url);
-  } else {
-    // 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 curr_instance->GetRelatedSiteInstance(dest_url);
   }
+
+  // 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 curr_instance->GetRelatedSiteInstance(dest_url);
 }
 
 int RenderViewHostManager::CreateRenderView(
@@ -711,6 +722,10 @@ int RenderViewHostManager::CreateRenderView(
   CHECK(instance);
   DCHECK(!swapped_out || hidden); // Swapped out views should always be hidden.
 
+  // We are creating a pending or swapped out RVH here.  We should never create
+  // it in the same SiteInstance as our current RVH.
+  CHECK_NE(render_view_host_->GetSiteInstance(), instance);
+
   // Check if we've already created an RVH for this SiteInstance.  If so, try
   // to re-use the existing one, which has already been initialized.  We'll
   // remove it from the list of swapped out hosts if it commits.
@@ -918,11 +933,18 @@ RenderViewHostImpl* RenderViewHostManager::UpdateRendererStateForNavigate(
   const NavigationEntry* curr_entry =
       delegate_->GetLastCommittedNavigationEntryForRenderManager();
   bool is_guest_scheme = curr_instance->GetSiteURL().SchemeIs(kGuestScheme);
-  bool force_swap = ShouldSwapProcessesForNavigation(curr_entry, &entry);
+  bool force_swap = !is_guest_scheme &&
+      ShouldSwapProcessesForNavigation(curr_entry, &entry);
   if (!is_guest_scheme && (ShouldTransitionCrossSite() || force_swap))
-    new_instance = GetSiteInstanceForEntry(entry, curr_instance);
+    new_instance = GetSiteInstanceForEntry(entry, curr_instance, force_swap);
+
+  // If force_swap is true, we must use a different SiteInstance.  If we didn't,
+  // we would have two RenderViewHosts in the same SiteInstance and the same
+  // tab, resulting in page_id conflicts for their NavigationEntries.
+  if (force_swap)
+    CHECK_NE(new_instance, curr_instance);
 
-  if (!is_guest_scheme && (new_instance != curr_instance || force_swap)) {
+  if (new_instance != curr_instance) {
     // New SiteInstance.
     DCHECK(!cross_navigation_pending_);