Don't swap processes for javascript: URLs.

content/browser/site_instance_unittest.cc

Index: content/browser/site_instance_unittest.cc
diff --git a/content/browser/site_instance_unittest.cc b/content/browser/site_instance_unittest.cc
index dcfcd3efbfb01f380891f7330c078f75f2a3c91c..663c7a8e4ca6afc8044c21ec8d7bc1717b37a3f0 100644
--- a/content/browser/site_instance_unittest.cc
+++ b/content/browser/site_instance_unittest.cc
@@ -119,25 +119,29 @@ class TestBrowsingInstance : public BrowsingInstance {
   TestBrowsingInstance(content::BrowserContext* browser_context,
                        int* deleteCounter)
       : BrowsingInstance(browser_context),
-        use_process_per_site(false),
-        deleteCounter_(deleteCounter) {
+        use_process_per_site_(false),
+        delete_counter_(deleteCounter) {

[awong, 2012/01/13 21:19:04]

nit: %s/deleteCounter/delete_counter/g

[nasko, 2012/01/13 21:47:10]

Done.

   }
 
   // Overrides BrowsingInstance::ShouldUseProcessPerSite so that we can test
   // both alternatives without using command-line switches.
   bool ShouldUseProcessPerSite(const GURL& url) {
-    return use_process_per_site;
+    return use_process_per_site_;
   }
 
-  // Set by individual tests.
-  bool use_process_per_site;
+  void set_use_process_per_site(bool use_process_per_site) {
+    use_process_per_site_ = use_process_per_site;
+  }
 
  private:
-  ~TestBrowsingInstance() {
-    (*deleteCounter_)++;
+  virtual ~TestBrowsingInstance() {
+    (*delete_counter_)++;
   }
 
-  int* deleteCounter_;
+  // Set by individual tests.
+  bool use_process_per_site_;
+
+  int* delete_counter_;
 };
 
 class TestSiteInstance : public SiteInstance {
@@ -151,14 +155,19 @@ class TestSiteInstance : public SiteInstance {
     return new TestSiteInstance(browsing_instance, siteDeleteCounter);
   }
 
+  void SetHasProcess(bool process) {

[Charlie Reis, 2012/01/13 21:16:29]

set_has_process, since it's just a setter function.

[nasko, 2012/01/13 21:47:10]

Function is not needed anymore, since I create a real process in the test.

+    has_process_ = process;
+  }
+
  private:
   TestSiteInstance(BrowsingInstance* browsing_instance, int* deleteCounter)
-    : SiteInstance(browsing_instance), deleteCounter_(deleteCounter) {}
-  ~TestSiteInstance() {
-    (*deleteCounter_)++;
+    : SiteInstance(browsing_instance), delete_counter_(deleteCounter) {}
+  virtual ~TestSiteInstance() {
+    (*delete_counter_)++;
   }
 
-  int* deleteCounter_;
+  int* delete_counter_;
+  bool has_process_;
 };
 
 }  // namespace
@@ -366,7 +375,7 @@ TEST_F(SiteInstanceTest, OneSiteInstancePerSite) {
   int deleteCounter = 0;
   TestBrowsingInstance* browsing_instance =
       new TestBrowsingInstance(NULL, &deleteCounter);
-  browsing_instance->use_process_per_site = false;
+  browsing_instance->set_use_process_per_site(false);
 
   const GURL url_a1("http://www.google.com/1.html");
   scoped_refptr<SiteInstance> site_instance_a1(
@@ -395,7 +404,7 @@ TEST_F(SiteInstanceTest, OneSiteInstancePerSite) {
   // browser context) should return a different SiteInstance.
   TestBrowsingInstance* browsing_instance2 =
       new TestBrowsingInstance(NULL, &deleteCounter);
-  browsing_instance2->use_process_per_site = false;
+  browsing_instance2->set_use_process_per_site(false);
   // Ensure the new SiteInstance is ref counted so that it gets deleted.
   scoped_refptr<SiteInstance> site_instance_a2_2(
       browsing_instance2->GetSiteInstanceForURL(url_a2));
@@ -424,7 +433,7 @@ TEST_F(SiteInstanceTest, OneSiteInstancePerSiteInBrowserContext) {
   int deleteCounter = 0;
   TestBrowsingInstance* browsing_instance =
       new TestBrowsingInstance(NULL, &deleteCounter);
-  browsing_instance->use_process_per_site = true;
+  browsing_instance->set_use_process_per_site(true);
 
   const GURL url_a1("http://www.google.com/1.html");
   scoped_refptr<SiteInstance> site_instance_a1(
@@ -456,7 +465,7 @@ TEST_F(SiteInstanceTest, OneSiteInstancePerSiteInBrowserContext) {
   // to make sure it gets deleted.
   scoped_refptr<TestBrowsingInstance> browsing_instance2(
       new TestBrowsingInstance(NULL, &deleteCounter));
-  browsing_instance2->use_process_per_site = true;
+  browsing_instance2->set_use_process_per_site(true);
   EXPECT_EQ(site_instance_a1.get(),
             browsing_instance2->GetSiteInstanceForURL(url_a2));
 
@@ -465,7 +474,7 @@ TEST_F(SiteInstanceTest, OneSiteInstancePerSiteInBrowserContext) {
   scoped_ptr<TestBrowserContext> browser_context(new TestBrowserContext());
   TestBrowsingInstance* browsing_instance3 =
       new TestBrowsingInstance(browser_context.get(), &deleteCounter);
-  browsing_instance3->use_process_per_site = true;
+  browsing_instance3->set_use_process_per_site(true);
   // Ensure the new SiteInstance is ref counted so that it gets deleted.
   scoped_refptr<SiteInstance> site_instance_a2_3(
       browsing_instance3->GetSiteInstanceForURL(url_a2));
@@ -529,7 +538,7 @@ TEST_F(SiteInstanceTest, ProcessSharingByType) {
       GURL(chrome::kChromeUIScheme + std::string("://newtab"))));
   policy->GrantWebUIBindings(webui1_instance->GetProcess()->GetID());
 
-  scoped_refptr<SiteInstance> webui2_instance( CreateSiteInstance(&rph_factory,
+  scoped_refptr<SiteInstance> webui2_instance(CreateSiteInstance(&rph_factory,
       GURL(chrome::kChromeUIScheme + std::string("://history"))));
 
   scoped_ptr<content::RenderProcessHost> dom_host(
@@ -546,3 +555,34 @@ TEST_F(SiteInstanceTest, ProcessSharingByType) {
 
   STLDeleteContainerPointers(hosts.begin(), hosts.end());
 }
+
+// Test to ensure that HasWrongProcessForURL behaves properly for different
+// types of URLs.
+TEST_F(SiteInstanceTest, HasWrongProcessForURL) {
+  scoped_ptr<TestBrowserContext> browser_context(new TestBrowserContext());
+  scoped_ptr<content::RenderProcessHost> host;
+  scoped_refptr<SiteInstance> instance(
+      SiteInstance::CreateSiteInstance(browser_context.get()));
+
+  EXPECT_FALSE(instance->has_site());
+  EXPECT_TRUE(instance->site().is_empty());
+
+  instance->SetSite(GURL("http://evernote.com/"));
+  EXPECT_TRUE(instance->has_site());
+
+  // Check prior to "assigning" a process to the instance, which is expected
+  // to return false due to not being attached to any process yet.
+  EXPECT_FALSE(instance->HasWrongProcessForURL(GURL("http://google.com")));
+
+  // The call to GetProcess actually creates a new real process, which works
+  // fine, but might be a cause for problems in different contexts.
+  host.reset(instance->GetProcess());
+  EXPECT_TRUE(host.get() != NULL);

[awong, 2012/01/13 21:19:04]

EXPECT_TRUE(host.get()) is jsut as common (maybe more common).

Up to you if you like the explicit != NULL.

[nasko, 2012/01/13 21:47:10]

I was trying to be consistent with the rest of the file :).

+  EXPECT_TRUE(instance->HasProcess());
+
+  EXPECT_FALSE(instance->HasWrongProcessForURL(GURL("http://evernote.com")));
+  EXPECT_FALSE(instance->HasWrongProcessForURL(
+      GURL("javascript:alert(document.location.href);")));
+
+  EXPECT_TRUE(instance->HasWrongProcessForURL(GURL("chrome://settings")));
+}