View-source URLs should use a separate SiteInstance in all cases.

BUG=143255


Review URL: https://chromiumcodereview.appspot.com/10825402

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

2 files changed, 34 insertions, 7 deletions

diff --git a/content/browser/renderer_host/render_view_host_manager_browsertest.cc b/content/browser/renderer_host/render_view_host_manager_browsertest.cc
index c6486b7..657f005 100644
--- a/content/browser/renderer_host/render_view_host_manager_browsertest.cc
+++ b/content/browser/renderer_host/render_view_host_manager_browsertest.cc
@@ -975,7 +975,9 @@ class RenderViewHostObserverArray {
 
 // Test for crbug.com/90867. Make sure we don't leak render view hosts since
 // they may cause crashes or memory corruptions when trying to call dead
-// delegate_.
+// delegate_. This test also verifies crbug.com/117420 and crbug.com/143255 to
+// ensure that a separate SiteInstance is created when navigating to view-source
+// URLs, regardless of current URL.
 IN_PROC_BROWSER_TEST_F(RenderViewHostManagerTest, LeakingRenderViewHosts) {
   // Start two servers with different sites.
   ASSERT_TRUE(test_server()->Start());
@@ -985,20 +987,38 @@ IN_PROC_BROWSER_TEST_F(RenderViewHostManagerTest, LeakingRenderViewHosts) {
       FilePath(FILE_PATH_LITERAL("content/test/data")));
   ASSERT_TRUE(https_server.Start());
 
+  // Observe the created render_view_host's to make sure they will not leak.
+  RenderViewHostObserverArray rvh_observers;
+
+  GURL navigated_url(test_server()->GetURL("files/title2.html"));
+  GURL view_source_url(chrome::kViewSourceScheme + std::string(":") +
+      navigated_url.spec());
+
+  // Let's ensure that when we start with a blank window, navigating away to a
+  // view-source URL, we create a new SiteInstance.
+  content::RenderViewHost* blank_rvh = shell()->web_contents()->
+      GetRenderViewHost();
+  SiteInstance* blank_site_instance = blank_rvh->GetSiteInstance();
+  EXPECT_EQ(shell()->web_contents()->GetURL(), GURL::EmptyGURL());
+  EXPECT_EQ(blank_site_instance->GetSite(), GURL::EmptyGURL());
+  rvh_observers.AddObserverToRVH(blank_rvh);
+
+  // Now navigate to the view-source URL and ensure we got a different
+  // SiteInstance and RenderViewHost.
+  NavigateToURL(shell(), view_source_url);
+  EXPECT_NE(blank_rvh, shell()->web_contents()->GetRenderViewHost());
+  EXPECT_NE(blank_site_instance, shell()->web_contents()->
+      GetRenderViewHost()->GetSiteInstance());
+  rvh_observers.AddObserverToRVH(shell()->web_contents()->GetRenderViewHost());
+
   // Load a random page and then navigate to view-source: of it.
   // This used to cause two RVH instances for the same SiteInstance, which
   // was a problem.  This is no longer the case.
-  GURL navigated_url(test_server()->GetURL("files/title2.html"));
   NavigateToURL(shell(), navigated_url);
   SiteInstance* site_instance1 = shell()->web_contents()->
       GetRenderViewHost()->GetSiteInstance();
-
-  // Observe the newly created render_view_host to make sure it will not leak.
-  RenderViewHostObserverArray rvh_observers;
   rvh_observers.AddObserverToRVH(shell()->web_contents()->GetRenderViewHost());
 
-  GURL view_source_url(chrome::kViewSourceScheme + std::string(":") +
-      navigated_url.spec());
   NavigateToURL(shell(), view_source_url);
   rvh_observers.AddObserverToRVH(shell()->web_contents()->GetRenderViewHost());
   SiteInstance* site_instance2 = shell()->web_contents()->
diff --git a/content/browser/web_contents/render_view_host_manager.cc b/content/browser/web_contents/render_view_host_manager.cc
index a116891..62fec79 100644
--- a/content/browser/web_contents/render_view_host_manager.cc
+++ b/content/browser/web_contents/render_view_host_manager.cc
@@ -488,6 +488,13 @@ SiteInstance* RenderViewHostManager::GetSiteInstanceForEntry(
     if (curr_site_instance->HasWrongProcessForURL(dest_url))
       return curr_site_instance->GetRelatedSiteInstance(dest_url);
 
+    // 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 (entry.IsViewSourceMode())
+      return SiteInstance::CreateForURL(browser_context, 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