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

content/browser/frame_host/render_view_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_view_host_manager.h"
 
 #include <utility>
 
 #include "base/command_line.h"
 #include "base/debug/trace_event.h"
@@ skipping 473 matching lines @@
   return
       !CommandLine::ForCurrentProcess()->HasSwitch(switches::kSingleProcess) &&
       !CommandLine::ForCurrentProcess()->HasSwitch(switches::kProcessPerTab);
 }
 
 bool RenderViewHostManager::ShouldSwapProcessesForNavigation(
     const NavigationEntry* curr_entry,
     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;
   }
 
   if (!curr_entry)
     return false;
 
   // We can't switch a RenderView between view source and non-view source mode
   // without screwing up the session history sometimes (when navigating between
   // "view-source:http://foo.com/" and "http://foo.com/", WebKit doesn't treat
   // it as a new navigation). So require a view switch.
@@ skipping 10 matching lines @@
       delegate_->GetControllerForRenderManager();
   return curr_entry && web_ui_.get() &&
       (WebUIControllerFactoryRegistry::GetInstance()->GetWebUIType(
           controller.GetBrowserContext(), curr_entry->GetURL()) ==
        WebUIControllerFactoryRegistry::GetInstance()->GetWebUIType(
           controller.GetBrowserContext(), new_entry->GetURL()));
 }
 
 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();
 
   // (UGLY) HEURISTIC, process-per-site only:
   //
   // If this navigation is generated, then it probably corresponds to a search
   // query.  Given that search results typically lead to users navigating to
   // other sites, we don't really want to use the search engine hostname to
   // determine the site instance for this navigation.
@@ skipping 106 matching lines @@
     return SiteInstance::CreateForURL(browser_context, dest_url);
   }
 
   // Use the current SiteInstance for same site navigations, as long as the
   // process type is correct.  (The URL may have been installed as an app since
   // the last time we visited it.)
   if (SiteInstance::IsSameWebSite(browser_context, current_url, dest_url) &&
       !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(
     SiteInstance* instance,
     int opener_route_id,
     bool swapped_out,
     bool hidden) {
   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.
   RenderViewHostImpl* new_render_view_host = static_cast<RenderViewHostImpl*>(
       GetSwappedOutRenderViewHost(instance));
   if (new_render_view_host) {
     // Prevent the process from exiting while we're trying to use it.
     if (!swapped_out)
       new_render_view_host->GetProcess()->AddPendingView();
   } else {
@@ skipping 187 matching lines @@
   // don't have to worry about this SiteInstance's ref count dropping to zero.
   SiteInstance* curr_instance = render_view_host_->GetSiteInstance();
 
   // Determine if we need a new SiteInstance for this entry.
   // Again, new_instance won't be deleted before the end of this method, so it
   // is safe to use a normal pointer here.
   SiteInstance* new_instance = curr_instance;
   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 (!is_guest_scheme && (new_instance != 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 (new_instance != curr_instance) {
     // New SiteInstance.
     DCHECK(!cross_navigation_pending_);
 
     // This will possibly create (set to NULL) a Web UI object for the pending
     // page. We'll use this later to give the page special access. This must
     // happen before the new renderer is created below so it will get bindings.
     // It must also happen after the above conditional call to CancelPending(),
     // otherwise CancelPending may clear the pending_web_ui_ and the page will
     // not have its bindings set appropriately.
     SetPendingWebUI(entry);
@@ skipping 174 matching lines @@
 RenderViewHostImpl* RenderViewHostManager::GetSwappedOutRenderViewHost(
     SiteInstance* instance) {
   RenderViewHostMap::iterator iter = swapped_out_hosts_.find(instance->GetId());
   if (iter != swapped_out_hosts_.end())
     return iter->second;
 
   return NULL;
 }
 
 }  // namespace content