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

content/browser/renderer_host/render_view_host_manager_browsertest.cc

 // Copyright (c) 2012 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 "base/file_util.h"
 #include "base/memory/ref_counted.h"
 #include "base/path_service.h"
 #include "base/utf_string_conversions.h"
 #include "content/browser/renderer_host/render_view_host_impl.h"
 #include "content/browser/site_instance_impl.h"
@@ skipping 957 matching lines @@
 
   void RemoveObserver(RVHObserver* observer) {
     observers_.remove(observer);
   }
 
   std::list<RVHObserver*> observers_;
 };
 
 // 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());
   net::TestServer https_server(
       net::TestServer::TYPE_HTTPS,
       net::TestServer::kLocalhost,
       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 blank window, navigating away from it,
+  // we create a new SiteInstance.

[Charlie Reis, 2012/08/16 22:28:09]

This comment isn't quite right.  This should only be true when navigating to
view-source.  Otherwise we should re-use the about:blank page's SiteInstance.

nit: with a

[nasko, 2012/08/16 22:59:23]

Done.

+  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);
+
+  // Explicit navigation to "about:blank" should use the same SiteInstance and
+  // RenderViewHost.

[Charlie Reis, 2012/08/16 22:28:09]

This sort of confuses the issue.  By putting any navigation here, we could be
affecting what happens in the view-source case below.  (Imagine if we only
re-used the about:blank SiteInstance if no other navigations had taken place. 
Then this test would miss the bug.)

I don't think we need this block.

[nasko, 2012/08/16 22:59:23]

Done.

+  NavigateToURL(shell(), GURL(chrome::kAboutBlankURL));
+  EXPECT_EQ(shell()->web_contents()->GetURL(), GURL(chrome::kAboutBlankURL));
+  EXPECT_EQ(blank_rvh, shell()->web_contents()->GetRenderViewHost());
+  EXPECT_EQ(blank_site_instance, shell()->web_contents()->GetRenderViewHost()->
+      GetSiteInstance());
+
+  // Now navigate to the view-source URL and ensure we got different

[Charlie Reis, 2012/08/16 22:28:09]

nit: got a

[nasko, 2012/08/16 22:59:23]

Done.

+  // 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()->
       GetRenderViewHost()->GetSiteInstance();
 
   // Ensure that view-source navigations force a new SiteInstance.
   EXPECT_NE(site_instance1, site_instance2);
 
   // Now navigate to a different instance so that we swap out again.
   NavigateToURL(shell(), https_server.GetURL("files/title2.html"));
   rvh_observers.AddObserverToRVH(shell()->web_contents()->GetRenderViewHost());
 
   // This used to leak a render view host.
   shell()->Close();
 
   RunAllPendingInMessageLoop();  // Needed on ChromeOS.
 
   EXPECT_EQ(0U, rvh_observers.GetNumObservers());
 }
 
 }  // namespace content