From 1dfa950a8dcbc268b1d036f26ef731b6b6aa702a Mon Sep 17 00:00:00 2001 From: "mpcomplete@google.com" Date: Mon, 15 Jun 2009 20:28:09 +0000 Subject: Group renderer processes by privilige when we hit the max process count. BUG=12128 TEST=Create a bunch of tabs (40+) and make sure New Tab pages are always grouped in a process with other chrome internal pages, extensions are always grouped together, and regular web pages are never in a process with extensions or New Tab pages. Review URL: http://codereview.chromium.org/126002 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@18428 0039d316-1c4b-4281-b951-d872f2087c98 --- .../renderer_host/mock_render_process_host.cc | 1 + .../browser/renderer_host/render_process_host.cc | 41 ++++++++--- chrome/browser/renderer_host/render_process_host.h | 24 +++++-- chrome/browser/tab_contents/site_instance.cc | 18 ++++- chrome/browser/tab_contents/site_instance.h | 4 ++ .../browser/tab_contents/site_instance_unittest.cc | 84 ++++++++++++++++++---- 6 files changed, 144 insertions(+), 28 deletions(-) (limited to 'chrome') diff --git a/chrome/browser/renderer_host/mock_render_process_host.cc b/chrome/browser/renderer_host/mock_render_process_host.cc index 2e6e61e..8cf0e10 100644 --- a/chrome/browser/renderer_host/mock_render_process_host.cc +++ b/chrome/browser/renderer_host/mock_render_process_host.cc @@ -12,6 +12,7 @@ MockRenderProcessHost::MockRenderProcessHost(Profile* profile) } MockRenderProcessHost::~MockRenderProcessHost() { + RemoveFromList(); delete transport_dib_; } diff --git a/chrome/browser/renderer_host/render_process_host.cc b/chrome/browser/renderer_host/render_process_host.cc index 349f8e2..49e70da 100644 --- a/chrome/browser/renderer_host/render_process_host.cc +++ b/chrome/browser/renderer_host/render_process_host.cc @@ -6,12 +6,13 @@ #include "base/rand_util.h" #include "base/sys_info.h" +#include "chrome/browser/child_process_security_policy.h" #include "chrome/common/chrome_constants.h" #include "chrome/common/notification_service.h" namespace { -unsigned int GetMaxRendererProcessCount() { +size_t GetMaxRendererProcessCount() { // Defines the maximum number of renderer processes according to the // amount of installed memory as reported by the OS. The table // values are calculated by assuming that you want the renderers to @@ -21,7 +22,7 @@ unsigned int GetMaxRendererProcessCount() { // If you modify this table you need to adjust browser\browser_uitest.cc // to match the expected number of processes. - static const int kMaxRenderersByRamTier[] = { + static const size_t kMaxRenderersByRamTier[] = { 3, // less than 256MB 6, // 256MB 9, // 512MB @@ -39,7 +40,7 @@ unsigned int GetMaxRendererProcessCount() { 40 // 3584MB }; - static unsigned int max_count = 0; + static size_t max_count = 0; if (!max_count) { size_t memory_tier = base::SysInfo::AmountOfPhysicalMemoryMB() / 256; if (memory_tier >= arraysize(kMaxRenderersByRamTier)) @@ -52,8 +53,24 @@ unsigned int GetMaxRendererProcessCount() { // Returns true if the given host is suitable for launching a new view // associated with the given profile. -static bool IsSuitableHost(Profile* profile, RenderProcessHost* host) { - return host->profile() == profile; +static bool IsSuitableHost(RenderProcessHost* host, Profile* profile, + RenderProcessHost::Type type) { + // If the host doesn't have a PID yet, we don't know what it will be used + // for, so just say it's unsuitable to be safe. + if (host->pid() == -1) + return false; + + if (host->profile() != profile) + return false; + + RenderProcessHost::Type host_type = RenderProcessHost::TYPE_NORMAL; + if (ChildProcessSecurityPolicy::GetInstance()->HasDOMUIBindings(host->pid())) + host_type = RenderProcessHost::TYPE_DOMUI; + if (ChildProcessSecurityPolicy::GetInstance()-> + HasExtensionBindings(host->pid())) + host_type = RenderProcessHost::TYPE_EXTENSION; + + return host_type == type; } // the global list of all renderer processes @@ -127,8 +144,7 @@ RenderProcessHost* RenderProcessHost::FromID(int render_process_id) { // static bool RenderProcessHost::ShouldTryToUseExistingProcessHost() { - unsigned int renderer_process_count = - static_cast(all_hosts.size()); + size_t renderer_process_count = all_hosts.size(); // NOTE: Sometimes it's necessary to create more render processes than // GetMaxRendererProcessCount(), for instance when we want to create @@ -141,13 +157,15 @@ bool RenderProcessHost::ShouldTryToUseExistingProcessHost() { } // static -RenderProcessHost* RenderProcessHost::GetExistingProcessHost(Profile* profile) { +RenderProcessHost* RenderProcessHost::GetExistingProcessHost(Profile* profile, + Type type) { // First figure out which existing renderers we can use. std::vector suitable_renderers; suitable_renderers.reserve(size()); for (iterator iter = begin(); iter != end(); ++iter) { - if (IsSuitableHost(profile, iter->second)) + if (run_renderer_in_process() || + IsSuitableHost(iter->second, profile, type)) suitable_renderers.push_back(iter->second); } @@ -170,3 +188,8 @@ void RenderProcessHost::SetProcessID(int pid) { pid_ = pid; all_hosts.AddWithID(this, pid); } + +void RenderProcessHost::RemoveFromList() { + if (all_hosts.Lookup(pid_)) + all_hosts.Remove(pid_); +} diff --git a/chrome/browser/renderer_host/render_process_host.h b/chrome/browser/renderer_host/render_process_host.h index 44990d7..ef79711 100644 --- a/chrome/browser/renderer_host/render_process_host.h +++ b/chrome/browser/renderer_host/render_process_host.h @@ -28,6 +28,16 @@ class RenderProcessHost : public IPC::Channel::Sender, public: typedef IDMap::const_iterator iterator; + // We classify renderers according to their highest privilege, and try + // to group pages into renderers with similar privileges. + // Note: it may be possible for a renderer to have multiple privileges, + // in which case we call it an "extension" renderer. + enum Type { + TYPE_NORMAL, // Normal renderer, no extra privileges. + TYPE_DOMUI, // Renderer with DOMUI privileges, like New Tab. + TYPE_EXTENSION, // Renderer with extension privileges. + }; + RenderProcessHost(Profile* profile); virtual ~RenderProcessHost(); @@ -174,15 +184,19 @@ class RenderProcessHost : public IPC::Channel::Sender, static bool ShouldTryToUseExistingProcessHost(); // Get an existing RenderProcessHost associated with the given profile, if - // possible. The renderer process is chosen randomly from the - // processes associated with the given profile. - // Returns NULL if no suitable renderer process is available. - static RenderProcessHost* GetExistingProcessHost(Profile* profile); + // possible. The renderer process is chosen randomly from suitable renderers + // that share the same profile and type. + // Returns NULL if no suitable renderer process is available, in which case + // the caller is free to create a new renderer. + static RenderProcessHost* GetExistingProcessHost(Profile* profile, Type type); protected: - // Sets the process of this object, so that others access it using FromID. + // Sets the process of this object, so that others access it using FromID. void SetProcessID(int pid); + // For testing. Removes this host from the list of hosts. + void RemoveFromList(); + base::Process process_; // A proxy for our IPC::Channel that lives on the IO thread (see diff --git a/chrome/browser/tab_contents/site_instance.cc b/chrome/browser/tab_contents/site_instance.cc index 0dbe744..8ea7baf 100644 --- a/chrome/browser/tab_contents/site_instance.cc +++ b/chrome/browser/tab_contents/site_instance.cc @@ -4,6 +4,7 @@ #include "chrome/browser/tab_contents/site_instance.h" +#include "chrome/browser/dom_ui/dom_ui_factory.h" #include "chrome/browser/renderer_host/browser_render_process_host.h" #include "chrome/common/url_constants.h" #include "chrome/common/notification_service.h" @@ -35,7 +36,7 @@ RenderProcessHost* SiteInstance::GetProcess() { // See if we should reuse an old process if (RenderProcessHost::ShouldTryToUseExistingProcessHost()) process_ = RenderProcessHost::GetExistingProcessHost( - browsing_instance_->profile()); + browsing_instance_->profile(), GetRendererType()); // Otherwise (or if that fails), create a new one. if (!process_) { @@ -166,6 +167,21 @@ bool SiteInstance::IsSameWebSite(const GURL& url1, const GURL& url2) { return net::RegistryControlledDomainService::SameDomainOrHost(url1, url2); } +RenderProcessHost::Type SiteInstance::GetRendererType() { + // We may not have a site at this point, which generally means this is a + // normal navigation. + if (!has_site_ || !site_.is_valid()) + return RenderProcessHost::TYPE_NORMAL; + + if (site_.SchemeIs(chrome::kExtensionScheme)) + return RenderProcessHost::TYPE_EXTENSION; + + if (DOMUIFactory::HasDOMUIScheme(site_)) + return RenderProcessHost::TYPE_DOMUI; + + return RenderProcessHost::TYPE_NORMAL; +} + void SiteInstance::Observe(NotificationType type, const NotificationSource& source, const NotificationDetails& details) { diff --git a/chrome/browser/tab_contents/site_instance.h b/chrome/browser/tab_contents/site_instance.h index 5c14dcc..81c182d 100644 --- a/chrome/browser/tab_contents/site_instance.h +++ b/chrome/browser/tab_contents/site_instance.h @@ -138,6 +138,10 @@ class SiteInstance : public base::RefCounted, // GetRelatedSiteInstance instead. SiteInstance(BrowsingInstance* browsing_instance); + // Returns the type of renderer process this instance belongs in, for grouping + // purposes. + RenderProcessHost::Type GetRendererType(); + private: // NotificationObserver implementation. void Observe(NotificationType type, diff --git a/chrome/browser/tab_contents/site_instance_unittest.cc b/chrome/browser/tab_contents/site_instance_unittest.cc index d0068d6..12bb9f9 100644 --- a/chrome/browser/tab_contents/site_instance_unittest.cc +++ b/chrome/browser/tab_contents/site_instance_unittest.cc @@ -3,11 +3,13 @@ // found in the LICENSE file. #include "base/string16.h" +#include "chrome/browser/child_process_security_policy.h" #include "chrome/browser/renderer_host/browser_render_process_host.h" #include "chrome/browser/renderer_host/render_view_host.h" #include "chrome/browser/renderer_host/test_render_view_host.h" #include "chrome/browser/tab_contents/navigation_entry.h" #include "chrome/browser/tab_contents/tab_contents.h" +#include "chrome/common/chrome_constants.h" #include "chrome/common/render_messages.h" #include "chrome/test/testing_profile.h" #include "testing/gtest/include/gtest/gtest.h" @@ -170,12 +172,12 @@ TEST_F(SiteInstanceTest, CloneNavigationEntry) { // Test to ensure UpdateMaxPageID is working properly. TEST_F(SiteInstanceTest, UpdateMaxPageID) { scoped_refptr instance(SiteInstance::CreateSiteInstance(NULL)); - EXPECT_EQ(-1, instance.get()->max_page_id()); + EXPECT_EQ(-1, instance->max_page_id()); // Make sure max_page_id_ is monotonically increasing. - instance.get()->UpdateMaxPageID(3); - instance.get()->UpdateMaxPageID(1); - EXPECT_EQ(3, instance.get()->max_page_id()); + instance->UpdateMaxPageID(3); + instance->UpdateMaxPageID(1); + EXPECT_EQ(3, instance->max_page_id()); } // Test to ensure GetProcess returns and creates processes correctly. @@ -185,13 +187,13 @@ TEST_F(SiteInstanceTest, GetProcess) { scoped_ptr host1; scoped_refptr instance( SiteInstance::CreateSiteInstance(profile.get())); - host1.reset(instance.get()->GetProcess()); + host1.reset(instance->GetProcess()); EXPECT_TRUE(host1.get() != NULL); // Ensure that GetProcess creates a new process. scoped_refptr instance2( SiteInstance::CreateSiteInstance(profile.get())); - scoped_ptr host2(instance2.get()->GetProcess()); + scoped_ptr host2(instance2->GetProcess()); EXPECT_TRUE(host2.get() != NULL); EXPECT_NE(host1.get(), host2.get()); } @@ -200,10 +202,10 @@ TEST_F(SiteInstanceTest, GetProcess) { TEST_F(SiteInstanceTest, SetSite) { scoped_refptr instance(SiteInstance::CreateSiteInstance(NULL)); EXPECT_FALSE(instance->has_site()); - EXPECT_TRUE(instance.get()->site().is_empty()); + EXPECT_TRUE(instance->site().is_empty()); - instance.get()->SetSite(GURL("http://www.google.com/index.html")); - EXPECT_EQ(GURL("http://google.com"), instance.get()->site()); + instance->SetSite(GURL("http://www.google.com/index.html")); + EXPECT_EQ(GURL("http://google.com"), instance->site()); EXPECT_TRUE(instance->has_site()); } @@ -291,14 +293,14 @@ TEST_F(SiteInstanceTest, OneSiteInstancePerSite) { // 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.get()->GetRelatedSiteInstance(url_b1)); + 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.get()->GetRelatedSiteInstance(url_a2)); + site_instance_a1->GetRelatedSiteInstance(url_a2)); // A visit to the original site in a new BrowsingInstance (same or different // profile) should return a different SiteInstance. @@ -349,14 +351,14 @@ TEST_F(SiteInstanceTest, OneSiteInstancePerSiteInProfile) { // 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.get()->GetRelatedSiteInstance(url_b1)); + 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.get()->GetRelatedSiteInstance(url_a2)); + site_instance_a1->GetRelatedSiteInstance(url_a2)); // A visit to the original site in a new BrowsingInstance (same profile) // should also return the same SiteInstance. @@ -398,3 +400,59 @@ TEST_F(SiteInstanceTest, OneSiteInstancePerSiteInProfile) { // browsing_instances will be deleted when their SiteInstances are deleted } + +static SiteInstance* CreateSiteInstance(RenderProcessHostFactory* factory, + const GURL& url) { + SiteInstance* instance = SiteInstance::CreateSiteInstanceForURL(NULL, url); + instance->set_render_process_host_factory(factory); + return instance; +} + +// Test to ensure that pages that require certain privileges are grouped +// in processes with similar pages. +TEST_F(SiteInstanceTest, ProcessSharingByType) { + MockRenderProcessHostFactory rph_factory; + ChildProcessSecurityPolicy* policy = + ChildProcessSecurityPolicy::GetInstance(); + + // Make a bunch of mock renderers so that we hit the limit. + std::vector hosts; + for (size_t i = 0; i < chrome::kMaxRendererProcessCount; ++i) + hosts.push_back(new MockRenderProcessHost(NULL)); + + // Create some extension instances and make sure they share a process. + scoped_refptr extension1_instance( + CreateSiteInstance(&rph_factory, GURL("chrome-extension://foo/bar"))); + policy->Add(extension1_instance->GetProcess()->pid()); + policy->GrantExtensionBindings(extension1_instance->GetProcess()->pid()); + + scoped_refptr extension2_instance( + CreateSiteInstance(&rph_factory, GURL("chrome-extension://baz/bar"))); + + scoped_ptr extension_host( + extension1_instance->GetProcess()); + EXPECT_EQ(extension1_instance->GetProcess(), + extension2_instance->GetProcess()); + + // Create some DOMUI instances and make sure they share a process. + scoped_refptr dom1_instance( + CreateSiteInstance(&rph_factory, GURL("chrome://newtab"))); + policy->Add(dom1_instance->GetProcess()->pid()); + policy->GrantDOMUIBindings(dom1_instance->GetProcess()->pid()); + + scoped_refptr dom2_instance( + CreateSiteInstance(&rph_factory, GURL("chrome://history"))); + + scoped_ptr dom_host(dom1_instance->GetProcess()); + EXPECT_EQ(dom1_instance->GetProcess(), dom2_instance->GetProcess()); + + // Make sure none of differing privilege processes are mixed. + EXPECT_NE(extension1_instance->GetProcess(), dom1_instance->GetProcess()); + + for (size_t i = 0; i < chrome::kMaxRendererProcessCount; ++i) { + EXPECT_NE(extension1_instance->GetProcess(), hosts[i]); + EXPECT_NE(dom1_instance->GetProcess(), hosts[i]); + } + + STLDeleteContainerPointers(hosts.begin(), hosts.end()); +} -- cgit v1.1