Prevent unused normal SiteInstances from being used for extensions.

chrome/browser/tab_contents/render_view_host_manager.cc

 // Copyright (c) 2011 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 "chrome/browser/tab_contents/render_view_host_manager.h"
 
 #include "base/command_line.h"
 #include "base/logging.h"
 #include "chrome/browser/dom_ui/dom_ui.h"
 #include "chrome/browser/dom_ui/web_ui_factory.h"
@@ skipping 345 matching lines @@
   // 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 (CommandLine::ForCurrentProcess()->HasSwitch(switches::kProcessPerSite) &&
       entry.transition_type() == PageTransition::GENERATED)
     return curr_instance;
 
   const GURL& dest_url = entry.url();
+  NavigationController& controller = delegate_->GetControllerForRenderManager();
+  Profile* profile = controller.profile();
 
   // 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
   // navigation commits (in DidNavigate), unless the navigation entry was
   // restored or it's a Web UI as described below.
   if (!curr_instance->has_site()) {
     // 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 curr_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.)
-    if (curr_instance->HasRelatedSiteInstance(dest_url)) {
+    if (curr_instance->HasRelatedSiteInstance(dest_url))
       return curr_instance->GetRelatedSiteInstance(dest_url);
-    } else {
-      // 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.
-      //
-      // For Web UI (this mostly comes up for the new tab page), the
-      // SiteInstance has special meaning: we never want to reassign the
-      // process. If you navigate to another site before the Web UI commits,
-      // we still want to create a new process rather than re-using the
-      // existing Web UI process.
-      if (entry.restore_type() != NavigationEntry::RESTORE_NONE ||
-          WebUIFactory::HasWebUIScheme(dest_url))
-        curr_instance->SetSite(dest_url);
-      return curr_instance;
-    }
+
+    // For extensions and Web UI URLs (such as the new tab page), we do not
+    // want to use the curr_instance if it has no site, since it will have a
+    // RenderProcessHost of TYPE_NORMAL.  Create a new SiteInstance for this
+    // URL instead (with the correct process type).
+    if (WebUIFactory::UseWebUIForURL(profile, dest_url))

[brettw, 2011/02/09 07:18:47]

Just to be sure, this doesn't affect "normal" new tab page WebUI loads does it?
I'm a bit confused how your bookmark manager bug will be different than the new
tab page, and we certainly don't want to be discarding processes in that case.

[Charlie Reis, 2011/02/09 07:25:39]

The "normal" NTP case will almost always find an existing SiteInstance in line
381.  I believe this is true even if no NTPs are currently open, as long as one
has been created since browser startup.  (That's what I saw when debugging.)

The bookmark manager is different because it's rarely opened.  Admittedly, it is
possible to get the NTP to hit this logic the first time you view an NTP, but
we'll usually get that out of the way at startup.

[brettw, 2011/02/09 07:31:53]

The startup case is what worries me the most since it looks like we might
discard a process if this happens? Is there any possibility of creating a
renderer for the NTP and then discarding it and creating a new "WebUI" one?

[Charlie Reis, 2011/02/09 18:04:07]

Hmm, yes, that's unfortunate.

Our current behavior is still incorrect-- if the NTP isn't created until we hit
the process limit, it can end up sharing a process with normal pages.  That
process is granted WebUI privileges so that the NTP works.

It would really be better if the NTP case set the site on the SiteInstance much
earlier, so that we could use the current process without fear of having it
shared with normal tabs, and thus not throw it away here.

Let me take a look at browser_navigator.cc to see if that's feasible for WebUI
pages.

+      return SiteInstance::CreateSiteInstanceForURL(profile, dest_url);
+
+    // 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.
+    if (entry.restore_type() != NavigationEntry::RESTORE_NONE)
+      curr_instance->SetSite(dest_url);
+
+    return curr_instance;
   }
 
   // Otherwise, only create a new SiteInstance for 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 = curr_instance->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
   // compare the entry's URL to the last committed entry's URL.
-  NavigationController& controller = delegate_->GetControllerForRenderManager();
   NavigationEntry* curr_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.
     curr_entry = controller.GetEntryAtOffset(-1);
   }
   // If there is no last non-interstitial entry (and curr_instance already
   // has a site), then we must have been opened from another tab.  We want
   // to compare against the URL of the page that opened us, but we can't
   // get to it directly.  The best we can do is check against the site of
   // the SiteInstance.  This will be correct when we intercept links and
   // script-based navigations, but for now, it could place some pages in a
   // new process unnecessarily.  We should only hit this case if a page tries
   // to open a new tab to an interstitial-inducing URL, and then navigates
   // the page to a different same-site URL.  (This seems very unlikely in
   // practice.)
   const GURL& current_url = (curr_entry) ? curr_entry->url() :
       curr_instance->site();
-  Profile* profile = controller.profile();
 
   if (SiteInstance::IsSameWebSite(profile, current_url, 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.
@@ skipping 278 matching lines @@
       Source<NavigationController>(&delegate_->GetControllerForRenderManager()),
       Details<RenderViewHostSwitchedDetails>(&details));
 
   // This will cause the old RenderViewHost to delete itself.
   old_render_view_host->Shutdown();
 
   // Let the task manager know that we've swapped RenderViewHosts, since it
   // might need to update its process groupings.
   delegate_->NotifySwappedFromRenderManager();
 }