Move process-per-site logic from BrowsingInstance to RenderProcessHost.

content/browser/site_instance_impl_unittest.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/command_line.h"
 #include "base/compiler_specific.h"
 #include "base/stl_util.h"
 #include "base/string16.h"
 #include "content/browser/browser_thread_impl.h"
 #include "content/browser/browsing_instance.h"
 #include "content/browser/child_process_security_policy_impl.h"
 #include "content/browser/renderer_host/render_process_host_impl.h"
 #include "content/browser/renderer_host/render_view_host_impl.h"
 #include "content/browser/renderer_host/test_render_view_host.h"
 #include "content/browser/site_instance_impl.h"
 #include "content/browser/web_contents/navigation_entry_impl.h"
 #include "content/browser/web_contents/web_contents_impl.h"
 #include "content/public/browser/web_ui_controller_factory.h"
 #include "content/public/common/content_client.h"
 #include "content/public/common/content_constants.h"
+#include "content/public/common/content_switches.h"
 #include "content/public/common/url_constants.h"
 #include "content/public/test/mock_render_process_host.h"
 #include "content/public/test/test_browser_context.h"
 #include "content/public/test/test_browser_thread.h"
 #include "content/test/test_content_browser_client.h"
 #include "content/test/test_content_client.h"
 #include "googleurl/src/url_util.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 using content::BrowserContext;
@@ skipping 55 matching lines @@
  public:
   SiteInstanceTestBrowserClient()
       : privileged_process_id_(-1) {
   }
 
   virtual content::WebUIControllerFactory*
       GetWebUIControllerFactory() OVERRIDE {
     return &factory_;
   }
 
-  virtual bool ShouldUseProcessPerSite(BrowserContext* browser_context,
-                                       const GURL& effective_url) OVERRIDE {
-    return false;
-  }
-
   virtual bool IsSuitableHost(content::RenderProcessHost* process_host,
                               const GURL& site_url) OVERRIDE {
     return (privileged_process_id_ == process_host->GetID()) ==
         site_url.SchemeIs(kPrivilegedScheme);
   }
 
   void set_privileged_process_id(int process_id) {
     privileged_process_id_ = process_id;
   }
 
@@ skipping 38 matching lines @@
   content::TestBrowserThread ui_thread_;
   content::TestBrowserThread file_user_blocking_thread_;
   content::TestBrowserThread io_thread_;
 
   SiteInstanceTestClient client_;
   SiteInstanceTestBrowserClient browser_client_;
   content::ContentClient* old_client_;
   content::ContentBrowserClient* old_browser_client_;
 };
 
+// Subclass of BrowsingInstance that updates a counter when deleted and
+// returns TestSiteInstances from GetSiteInstanceForURL.
 class TestBrowsingInstance : public BrowsingInstance {
  public:
   TestBrowsingInstance(BrowserContext* browser_context, int* delete_counter)
       : BrowsingInstance(browser_context),
-        use_process_per_site_(false),
         delete_counter_(delete_counter) {
   }
 
-  // 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_;
-  }
-
-  void set_use_process_per_site(bool use_process_per_site) {
-    use_process_per_site_ = use_process_per_site;
-  }
-
   // Make a few methods public for tests.
-  using BrowsingInstance::ShouldUseProcessPerSite;
   using BrowsingInstance::browser_context;
   using BrowsingInstance::HasSiteInstance;
   using BrowsingInstance::GetSiteInstanceForURL;
   using BrowsingInstance::RegisterSiteInstance;
   using BrowsingInstance::UnregisterSiteInstance;
 
  private:
   virtual ~TestBrowsingInstance() {
     (*delete_counter_)++;
   }
 
-  // Set by individual tests.
-  bool use_process_per_site_;
-
   int* delete_counter_;
 };
 
+// Subclass of SiteInstanceImpl that updates a counter when deleted.
 class TestSiteInstance : public SiteInstanceImpl {
  public:
   static TestSiteInstance* CreateTestSiteInstance(
       BrowserContext* browser_context,
       int* site_delete_counter,
       int* browsing_delete_counter) {
     TestBrowsingInstance* browsing_instance =
         new TestBrowsingInstance(browser_context, browsing_delete_counter);
     return new TestSiteInstance(browsing_instance, site_delete_counter);
   }
@@ skipping 198 matching lines @@
 
   // JavaScript links should be considered same site for anything.
   EXPECT_TRUE(SiteInstance::IsSameWebSite(NULL, url_javascript, url_foo));
   EXPECT_TRUE(SiteInstance::IsSameWebSite(NULL, url_javascript, url_foo_https));
   EXPECT_TRUE(SiteInstance::IsSameWebSite(NULL, url_javascript, url_foo_port));
 }
 
 // Test to ensure that there is only one SiteInstance per site in a given
 // BrowsingInstance, when process-per-site is not in use.
 TEST_F(SiteInstanceTest, OneSiteInstancePerSite) {
+  ASSERT_FALSE(CommandLine::ForCurrentProcess()->HasSwitch(
+      switches::kProcessPerSite));
   int delete_counter = 0;
+  scoped_ptr<content::TestBrowserContext> browser_context(
+      new content::TestBrowserContext());
   TestBrowsingInstance* browsing_instance =
-      new TestBrowsingInstance(NULL, &delete_counter);
-  browsing_instance->set_use_process_per_site(false);
+      new TestBrowsingInstance(browser_context.get(), &delete_counter);
 
   const GURL url_a1("http://www.google.com/1.html");
   scoped_refptr<SiteInstanceImpl> site_instance_a1(
       static_cast<SiteInstanceImpl*>(
           browsing_instance->GetSiteInstanceForURL(url_a1)));
   EXPECT_TRUE(site_instance_a1.get() != NULL);
 
   // A separate site should create a separate SiteInstance.
   const GURL url_b1("http://www.yahoo.com/");
   scoped_refptr<SiteInstanceImpl> site_instance_b1(
@@ skipping 10 matching lines @@
   // A second visit to the original site should return the same SiteInstance.
   const GURL url_a2("http://www.google.com/2.html");
   EXPECT_EQ(site_instance_a1.get(),
             browsing_instance->GetSiteInstanceForURL(url_a2));
   EXPECT_EQ(site_instance_a1.get(),
             site_instance_a1->GetRelatedSiteInstance(url_a2));
 
   // A visit to the original site in a new BrowsingInstance (same or different
   // browser context) should return a different SiteInstance.
   TestBrowsingInstance* browsing_instance2 =
-      new TestBrowsingInstance(NULL, &delete_counter);
-  browsing_instance2->set_use_process_per_site(false);
+      new TestBrowsingInstance(browser_context.get(), &delete_counter);
   // Ensure the new SiteInstance is ref counted so that it gets deleted.
   scoped_refptr<SiteInstanceImpl> site_instance_a2_2(
       static_cast<SiteInstanceImpl*>(
           browsing_instance2->GetSiteInstanceForURL(url_a2)));
   EXPECT_NE(site_instance_a1.get(), site_instance_a2_2.get());
   EXPECT_FALSE(site_instance_a1->IsRelatedSiteInstance(site_instance_a2_2));
 
+  // The two SiteInstances for http://google.com should not use the same process
+  // if process-per-site is not enabled.
+  scoped_ptr<content::RenderProcessHost> process_a1(
+      site_instance_a1->GetProcess());
+  scoped_ptr<content::RenderProcessHost> process_a2_2(
+      site_instance_a2_2->GetProcess());
+  EXPECT_NE(process_a1.get(), process_a2_2.get());
+
   // Should be able to see that we do have SiteInstances.
   EXPECT_TRUE(browsing_instance->HasSiteInstance(
       GURL("http://mail.google.com")));
   EXPECT_TRUE(browsing_instance2->HasSiteInstance(
       GURL("http://mail.google.com")));
   EXPECT_TRUE(browsing_instance->HasSiteInstance(
       GURL("http://mail.yahoo.com")));
 
   // Should be able to see that we don't have SiteInstances.
   EXPECT_FALSE(browsing_instance->HasSiteInstance(
       GURL("https://www.google.com")));
   EXPECT_FALSE(browsing_instance2->HasSiteInstance(
       GURL("http://www.yahoo.com")));
 
-  // browsing_instances will be deleted when their SiteInstances are deleted
+  // browsing_instances will be deleted when their SiteInstances are deleted.
+  // The processes will be unregistered when the RPH scoped_ptrs go away.

[awong, 2012/06/27 00:26:54]

Do we have a way to assert these two assumptions in a test?

If there is, then maybe a separate test should be make to exercise the
unregistration code.

[Charlie Reis, 2012/06/27 20:53:43]

Actually, we already have the first assumption tested in the
SiteInstanceDestructor test.  I've added a check for the second assumption in
TearDown, which should help a lot.

 }
 
-// Test to ensure that there is only one SiteInstance per site for an entire
-// BrowserContext, if process-per-site is in use.
+// Test to ensure that there is only one RenderProcessHost per site for an
+// entire BrowserContext, if process-per-site is in use.
 TEST_F(SiteInstanceTest, OneSiteInstancePerSiteInBrowserContext) {
+  CommandLine::ForCurrentProcess()->AppendSwitch(
+      switches::kProcessPerSite);
   int delete_counter = 0;
+  scoped_ptr<content::TestBrowserContext> browser_context(
+      new content::TestBrowserContext());
   TestBrowsingInstance* browsing_instance =
-      new TestBrowsingInstance(NULL, &delete_counter);
-  browsing_instance->set_use_process_per_site(true);
+      new TestBrowsingInstance(browser_context.get(), &delete_counter);
 
   const GURL url_a1("http://www.google.com/1.html");
   scoped_refptr<SiteInstanceImpl> site_instance_a1(
       static_cast<SiteInstanceImpl*>(
           browsing_instance->GetSiteInstanceForURL(url_a1)));
   EXPECT_TRUE(site_instance_a1.get() != NULL);
+  scoped_ptr<content::RenderProcessHost> process_a1(
+      site_instance_a1->GetProcess());
 
   // A separate site should create a separate SiteInstance.
   const GURL url_b1("http://www.yahoo.com/");
   scoped_refptr<SiteInstanceImpl> site_instance_b1(
       static_cast<SiteInstanceImpl*>(
           browsing_instance->GetSiteInstanceForURL(url_b1)));
   EXPECT_NE(site_instance_a1.get(), site_instance_b1.get());
   EXPECT_TRUE(site_instance_a1->IsRelatedSiteInstance(site_instance_b1));
 
   // Getting the new SiteInstance from the BrowsingInstance and from another
   // SiteInstance in the BrowsingInstance should give the same result.
   EXPECT_EQ(site_instance_b1.get(),
             site_instance_a1->GetRelatedSiteInstance(url_b1));
 
   // A second visit to the original site should return the same SiteInstance.
   const GURL url_a2("http://www.google.com/2.html");
   EXPECT_EQ(site_instance_a1.get(),
             browsing_instance->GetSiteInstanceForURL(url_a2));
   EXPECT_EQ(site_instance_a1.get(),
             site_instance_a1->GetRelatedSiteInstance(url_a2));
 
   // A visit to the original site in a new BrowsingInstance (same browser
-  // context) should also return the same SiteInstance.
-  // This BrowsingInstance doesn't get its own SiteInstance within the test, so
-  // it won't be deleted by its children.  Thus, we'll keep a ref count to it
-  // to make sure it gets deleted.
-  scoped_refptr<TestBrowsingInstance> browsing_instance2(
-      new TestBrowsingInstance(NULL, &delete_counter));
-  browsing_instance2->set_use_process_per_site(true);
-  EXPECT_EQ(site_instance_a1.get(),
-            browsing_instance2->GetSiteInstanceForURL(url_a2));
+  // context) should return a different SiteInstance with the same process.
+  TestBrowsingInstance* browsing_instance2 =
+      new TestBrowsingInstance(browser_context.get(), &delete_counter);
+  scoped_refptr<SiteInstanceImpl> site_instance_a1_2(
+      static_cast<SiteInstanceImpl*>(
+          browsing_instance2->GetSiteInstanceForURL(url_a1)));
+  EXPECT_TRUE(site_instance_a1.get() != NULL);

[awong, 2012/06/27 00:26:54]

should we assert that site_instance_a1 != site_instance_a1_2?

[Charlie Reis, 2012/06/27 20:53:43]

Good call.  Added.

+  EXPECT_EQ(process_a1.get(), site_instance_a1_2->GetProcess());
 
   // A visit to the original site in a new BrowsingInstance (different browser
-  // context) should return a different SiteInstance.
-  scoped_ptr<content::TestBrowserContext> browser_context(
+  // context) should return a different SiteInstance with a different process.
+  scoped_ptr<content::TestBrowserContext> browser_context2(
       new content::TestBrowserContext());
   TestBrowsingInstance* browsing_instance3 =
-      new TestBrowsingInstance(browser_context.get(), &delete_counter);
-  browsing_instance3->set_use_process_per_site(true);
-  // Ensure the new SiteInstance is ref counted so that it gets deleted.
+      new TestBrowsingInstance(browser_context2.get(), &delete_counter);
   scoped_refptr<SiteInstanceImpl> site_instance_a2_3(
       static_cast<SiteInstanceImpl*>(
           browsing_instance3->GetSiteInstanceForURL(url_a2)));
-  EXPECT_NE(site_instance_a1.get(), site_instance_a2_3.get());
+  EXPECT_TRUE(site_instance_a2_3.get() != NULL);
+  scoped_ptr<content::RenderProcessHost> process_a2_3(
+      site_instance_a2_3->GetProcess());
+  EXPECT_NE(process_a1.get(), process_a2_3.get());

[awong, 2012/06/27 00:26:54]

Same question about comparing the site instances...

[Charlie Reis, 2012/06/27 20:53:43]

Done.

 
   // Should be able to see that we do have SiteInstances.
   EXPECT_TRUE(browsing_instance->HasSiteInstance(
       GURL("http://mail.google.com")));  // visited before
   EXPECT_TRUE(browsing_instance2->HasSiteInstance(
       GURL("http://mail.google.com")));  // visited before
   EXPECT_TRUE(browsing_instance->HasSiteInstance(
       GURL("http://mail.yahoo.com")));  // visited before
-  EXPECT_TRUE(browsing_instance2->HasSiteInstance(
-      GURL("http://www.yahoo.com")));  // different BI, but same browser context
 
   // Should be able to see that we don't have SiteInstances.
+  EXPECT_FALSE(browsing_instance2->HasSiteInstance(
+      GURL("http://www.yahoo.com")));  // different BI, same browser context
   EXPECT_FALSE(browsing_instance->HasSiteInstance(
       GURL("https://www.google.com")));  // not visited before
   EXPECT_FALSE(browsing_instance3->HasSiteInstance(
       GURL("http://www.yahoo.com")));  // different BI, different context
 
-  // browsing_instances will be deleted when their SiteInstances are deleted
+  // browsing_instances will be deleted when their SiteInstances are deleted.
+  // The processes will be unregistered when the RPH scoped_ptrs go away.
 }
 
 static SiteInstanceImpl* CreateSiteInstance(
     content::RenderProcessHostFactory* factory, const GURL& url) {
   SiteInstanceImpl* instance =
       reinterpret_cast<SiteInstanceImpl*>(
           SiteInstance::CreateForURL(NULL, url));
   instance->set_render_process_host_factory(factory);
   return instance;
 }
@@ skipping 74 matching lines @@
   host.reset(instance->GetProcess());
   EXPECT_TRUE(host.get() != NULL);
   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")));
 }