Fix an issue with SiteInstance where special URLs would not always get grouped

together.

This is also useful for chrome-extension URLs, where we want any URLs for a
given extension to be grouped in the same process.

BUG=11501,11002
Review URL: http://codereview.chromium.org/115003

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@15565 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 56 insertions, 13 deletions

diff --git a/chrome/browser/renderer_host/test_render_view_host.h b/chrome/browser/renderer_host/test_render_view_host.h
index 51acf2d..6088301 100644
--- a/chrome/browser/renderer_host/test_render_view_host.h
+++ b/chrome/browser/renderer_host/test_render_view_host.h
@@ -198,7 +198,16 @@ class RenderViewHostTestHarness : public testing::Test {
   }
 
   TestRenderViewHost* rvh() {
-    return reinterpret_cast<TestRenderViewHost*>(contents_->render_view_host());
+    return static_cast<TestRenderViewHost*>(contents_->render_view_host());
+  }
+
+  TestRenderViewHost* pending_rvh() {
+    return static_cast<TestRenderViewHost*>(
+        contents_->render_manager()->pending_render_view_host());
+  }
+
+  TestRenderViewHost* active_rvh() {
+    return pending_rvh() ? pending_rvh() : rvh();
   }
 
   TestingProfile* profile() {
diff --git a/chrome/browser/tab_contents/render_view_host_manager.cc b/chrome/browser/tab_contents/render_view_host_manager.cc
index 8b2c276..07e4277 100644
--- a/chrome/browser/tab_contents/render_view_host_manager.cc
+++ b/chrome/browser/tab_contents/render_view_host_manager.cc
@@ -374,13 +374,13 @@ SiteInstance* RenderViewHostManager::GetSiteInstanceForEntry(
     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 new ones. This addresses special cases where we use a
-    // single BrowsingInstance for all pages of a certain type (e.g., New Tab
+    // 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 make a
-    // new process.
-    return SiteInstance::CreateSiteInstance(
-        delegate_->GetControllerForRenderManager().profile());
+    // that page, we want to explicity ignore that BrowsingInstance and group
+    // this page into the appropriate SiteInstance for its URL.
+    return SiteInstance::CreateSiteInstanceForURL(
+        delegate_->GetControllerForRenderManager().profile(), dest_url);
   } else {
     // Start the new renderer in a new SiteInstance, but in the current
     // BrowsingInstance.  It is important to immediately give this new
diff --git a/chrome/browser/tab_contents/render_view_host_manager_unittest.cc b/chrome/browser/tab_contents/render_view_host_manager_unittest.cc
index 324c547..108eed7 100644
--- a/chrome/browser/tab_contents/render_view_host_manager_unittest.cc
+++ b/chrome/browser/tab_contents/render_view_host_manager_unittest.cc
@@ -7,7 +7,17 @@
 #include "testing/gtest/include/gtest/gtest.h"
 
 class RenderViewHostManagerTest : public RenderViewHostTestHarness {
-
+ public:
+  void NavigateActiveAndCommit(const GURL& url) {
+    // Note: we navigate the active RenderViewHost because previous navigations
+    // won't have committed yet, so NavigateAndCommit does the wrong thing
+    // for us.
+    controller().LoadURL(url, GURL(), 0);
+    active_rvh()->SendNavigate(
+        static_cast<MockRenderProcessHost*>(active_rvh()->process())->
+            max_page_id() + 1,
+        url);
+  }
 };
 
 // Tests that when you navigate from the New TabPage to another page, and
@@ -19,8 +29,8 @@ TEST_F(RenderViewHostManagerTest, NewTabPageProcesses) {
   GURL dest("http://www.google.com/");
 
   // Navigate our first tab to the new tab page and then to the destination.
-  NavigateAndCommit(ntp);
-  NavigateAndCommit(dest);
+  NavigateActiveAndCommit(ntp);
+  NavigateActiveAndCommit(dest);
 
   // Make a second tab.
   TestTabContents contents2(profile_.get(), NULL);
@@ -36,9 +46,20 @@ TEST_F(RenderViewHostManagerTest, NewTabPageProcesses) {
       pending_render_view_host())->SendNavigate(101, dest);
 
   // The two RVH's should be different in every way.
-  EXPECT_NE(rvh()->process(), contents2.render_view_host()->process());
-  EXPECT_NE(rvh()->site_instance(),
+  EXPECT_NE(active_rvh()->process(), contents2.render_view_host()->process());
+  EXPECT_NE(active_rvh()->site_instance(),
       contents2.render_view_host()->site_instance());
-  EXPECT_NE(rvh()->site_instance()->browsing_instance(),
+  EXPECT_NE(active_rvh()->site_instance()->browsing_instance(),
       contents2.render_view_host()->site_instance()->browsing_instance());
+
+  // Navigate both to the new tab page, and verify that they share a
+  // SiteInstance.
+  NavigateActiveAndCommit(ntp);
+
+  contents2.controller().LoadURL(ntp, GURL(), PageTransition::LINK);
+  static_cast<TestRenderViewHost*>(contents2.render_manager()->
+     pending_render_view_host())->SendNavigate(102, ntp);
+
+  EXPECT_EQ(active_rvh()->site_instance(),
+      contents2.render_view_host()->site_instance());
 }
diff --git a/chrome/browser/tab_contents/site_instance.cc b/chrome/browser/tab_contents/site_instance.cc
index 05b3fbd..e817b8b 100644
--- a/chrome/browser/tab_contents/site_instance.cc
+++ b/chrome/browser/tab_contents/site_instance.cc
@@ -94,6 +94,12 @@ SiteInstance* SiteInstance::CreateSiteInstance(Profile* profile) {
 }
 
 /*static*/
+SiteInstance* SiteInstance::CreateSiteInstanceForURL(Profile* profile,
+                                                     const GURL& url) {
+  return (new BrowsingInstance(profile))->GetSiteInstanceForURL(url);
+}
+
+/*static*/
 GURL SiteInstance::GetSiteForURL(const GURL& url) {
   // URLs with no host should have an empty site.
   GURL site;
diff --git a/chrome/browser/tab_contents/site_instance.h b/chrome/browser/tab_contents/site_instance.h
index 18cebbd..8cf238b 100644
--- a/chrome/browser/tab_contents/site_instance.h
+++ b/chrome/browser/tab_contents/site_instance.h
@@ -110,6 +110,13 @@ class SiteInstance : public base::RefCounted<SiteInstance>,
   // Darin suggests.
   static SiteInstance* CreateSiteInstance(Profile* profile);
 
+  // Factory method to get the appropriate SiteInstance for the given URL, in
+  // a new BrowsingInstance.  Use this instead of CreateSiteInstance when you
+  // know the URL, since it allows special site grouping rules to be applied
+  // (for example, to group chrome-ui pages into the same instance).
+  static SiteInstance* CreateSiteInstanceForURL(Profile* profile,
+                                                const GURL& url);
+
   // Returns the site for the given URL, which includes only the scheme and
   // registered domain.  Returns an empty GURL if the URL has no host.
   static GURL GetSiteForURL(const GURL& url);