From 3059caa70c826f0edbcf50d6f61e6e9a9c34008d Mon Sep 17 00:00:00 2001 From: "jyasskin@chromium.org" Date: Thu, 6 Jun 2013 03:48:41 +0000 Subject: Store the RenderProcessHostFactory in a global variable instead of a member variable within each SiteInstance. The local variable in one SiteInstance was documented to be passed on to other SiteInstances spawned from it, but that never actually worked. Even fixing that doesn't serve the use case of allowing different RenderProcessHostFactories for different trees of SiteInstances because of the many calls to SiteInstance::CreateForURL inside RenderViewHostManager. Review URL: https://chromiumcodereview.appspot.com/15799009 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@204409 0039d316-1c4b-4281-b951-d872f2087c98 --- .../web_contents_video_capture_device_unittest.cc | 5 +++-- content/browser/site_instance_impl.cc | 12 ++++++++--- content/browser/site_instance_impl.h | 17 ++++++++-------- content/browser/site_instance_impl_unittest.cc | 23 ++++++++++------------ content/test/test_render_view_host_factory.cc | 10 ++++------ content/test/test_render_view_host_factory.h | 9 --------- 6 files changed, 35 insertions(+), 41 deletions(-) diff --git a/content/browser/renderer_host/media/web_contents_video_capture_device_unittest.cc b/content/browser/renderer_host/media/web_contents_video_capture_device_unittest.cc index 58f2914..52bca58 100644 --- a/content/browser/renderer_host/media/web_contents_video_capture_device_unittest.cc +++ b/content/browser/renderer_host/media/web_contents_video_capture_device_unittest.cc @@ -447,8 +447,8 @@ class WebContentsVideoCaptureDeviceTest : public testing::Test { scoped_refptr site_instance = SiteInstance::Create(browser_context_.get()); - static_cast(site_instance.get())-> - set_render_process_host_factory(render_process_host_factory_.get()); + SiteInstanceImpl::set_render_process_host_factory( + render_process_host_factory_.get()); web_contents_.reset( TestWebContents::Create(browser_context_.get(), site_instance.get())); @@ -485,6 +485,7 @@ class WebContentsVideoCaptureDeviceTest : public testing::Test { content::RunAllPendingInMessageLoop(); + SiteInstanceImpl::set_render_process_host_factory(NULL); render_view_host_factory_.reset(); render_process_host_factory_.reset(); } diff --git a/content/browser/site_instance_impl.cc b/content/browser/site_instance_impl.cc index 56ee6df..889d9d2 100644 --- a/content/browser/site_instance_impl.cc +++ b/content/browser/site_instance_impl.cc @@ -35,12 +35,13 @@ static bool IsURLSameAsAnySiteInstance(const GURL& url) { url == GURL(kChromeUIShorthangURL); } +const RenderProcessHostFactory* + SiteInstanceImpl::g_render_process_host_factory_ = NULL; int32 SiteInstanceImpl::next_site_instance_id_ = 1; SiteInstanceImpl::SiteInstanceImpl(BrowsingInstance* browsing_instance) : id_(next_site_instance_id_++), browsing_instance_(browsing_instance), - render_process_host_factory_(NULL), process_(NULL), has_site_(false) { DCHECK(browsing_instance); @@ -111,8 +112,8 @@ RenderProcessHost* SiteInstanceImpl::GetProcess() { // Otherwise (or if that fails), create a new one. if (!process_) { - if (render_process_host_factory_) { - process_ = render_process_host_factory_->CreateRenderProcessHost( + if (g_render_process_host_factory_) { + process_ = g_render_process_host_factory_->CreateRenderProcessHost( browser_context); } else { StoragePartitionImpl* partition = @@ -220,6 +221,11 @@ bool SiteInstanceImpl::HasWrongProcessForURL(const GURL& url) { GetProcess(), browsing_instance_->browser_context(), site_url); } +void SiteInstanceImpl::set_render_process_host_factory( + const RenderProcessHostFactory* rph_factory) { + g_render_process_host_factory_ = rph_factory; +} + BrowserContext* SiteInstanceImpl::GetBrowserContext() const { return browsing_instance_->browser_context(); } diff --git a/content/browser/site_instance_impl.h b/content/browser/site_instance_impl.h index fda0d37..bd5f9c8 100644 --- a/content/browser/site_instance_impl.h +++ b/content/browser/site_instance_impl.h @@ -44,14 +44,12 @@ class CONTENT_EXPORT SiteInstanceImpl : public SiteInstance, // navigating to the URL. bool HasWrongProcessForURL(const GURL& url); - // Sets the factory used to create new RenderProcessHosts. This will also be - // passed on to SiteInstances spawned by this one. - // The factory must outlive the SiteInstance; ownership is not transferred. It - // may be NULL, in which case the default BrowserRenderProcessHost will be - // created (this is the behavior if you don't call this function). - void set_render_process_host_factory(RenderProcessHostFactory* rph_factory) { - render_process_host_factory_ = rph_factory; - } + // Sets the global factory used to create new RenderProcessHosts. It may be + // NULL, in which case the default BrowserRenderProcessHost will be created + // (this is the behavior if you don't call this function). The factory must + // be set back to NULL before it's destroyed; ownership is not transferred. + static void set_render_process_host_factory( + const RenderProcessHostFactory* rph_factory); protected: friend class BrowsingInstance; @@ -78,6 +76,9 @@ class CONTENT_EXPORT SiteInstanceImpl : public SiteInstance, // Used to restrict a process' origin access rights. void LockToOrigin(); + // An object used to construct RenderProcessHosts. + static const RenderProcessHostFactory* g_render_process_host_factory_; + // The next available SiteInstance ID. static int32 next_site_instance_id_; diff --git a/content/browser/site_instance_impl_unittest.cc b/content/browser/site_instance_impl_unittest.cc index 7e807bd..3c3a09c 100644 --- a/content/browser/site_instance_impl_unittest.cc +++ b/content/browser/site_instance_impl_unittest.cc @@ -103,6 +103,7 @@ class SiteInstanceTest : public testing::Test { EXPECT_TRUE(RenderProcessHost::AllHostsIterator().IsAtEnd()); SetBrowserClientForTesting(old_browser_client_); + SiteInstanceImpl::set_render_process_host_factory(NULL); // http://crbug.com/143565 found SiteInstanceTest leaking an // AppCacheDatabase. This happens because some part of the test indirectly @@ -543,21 +544,17 @@ TEST_F(SiteInstanceTest, OneSiteInstancePerSiteInBrowserContext) { DrainMessageLoops(); } -static SiteInstanceImpl* CreateSiteInstance( - BrowserContext* browser_context, - RenderProcessHostFactory* factory, - const GURL& url) { - SiteInstanceImpl* instance = - reinterpret_cast( - SiteInstance::CreateForURL(browser_context, url)); - instance->set_render_process_host_factory(factory); - return instance; +static SiteInstanceImpl* CreateSiteInstance(BrowserContext* browser_context, + const GURL& url) { + return static_cast( + SiteInstance::CreateForURL(browser_context, url)); } // Test to ensure that pages that require certain privileges are grouped // in processes with similar pages. TEST_F(SiteInstanceTest, ProcessSharingByType) { MockRenderProcessHostFactory rph_factory; + SiteInstanceImpl::set_render_process_host_factory(&rph_factory); ChildProcessSecurityPolicyImpl* policy = ChildProcessSecurityPolicyImpl::GetInstance(); @@ -569,12 +566,12 @@ TEST_F(SiteInstanceTest, ProcessSharingByType) { // Create some extension instances and make sure they share a process. scoped_refptr extension1_instance( - CreateSiteInstance(browser_context.get(), &rph_factory, + CreateSiteInstance(browser_context.get(), GURL(kPrivilegedScheme + std::string("://foo/bar")))); set_privileged_process_id(extension1_instance->GetProcess()->GetID()); scoped_refptr extension2_instance( - CreateSiteInstance(browser_context.get(), &rph_factory, + CreateSiteInstance(browser_context.get(), GURL(kPrivilegedScheme + std::string("://baz/bar")))); scoped_ptr extension_host( @@ -584,12 +581,12 @@ TEST_F(SiteInstanceTest, ProcessSharingByType) { // Create some WebUI instances and make sure they share a process. scoped_refptr webui1_instance(CreateSiteInstance( - browser_context.get(), &rph_factory, + browser_context.get(), GURL(chrome::kChromeUIScheme + std::string("://newtab")))); policy->GrantWebUIBindings(webui1_instance->GetProcess()->GetID()); scoped_refptr webui2_instance(CreateSiteInstance( - browser_context.get(), &rph_factory, + browser_context.get(), GURL(chrome::kChromeUIScheme + std::string("://history")))); scoped_ptr dom_host(webui1_instance->GetProcess()); diff --git a/content/test/test_render_view_host_factory.cc b/content/test/test_render_view_host_factory.cc index 3211dd0..47213aa 100644 --- a/content/test/test_render_view_host_factory.cc +++ b/content/test/test_render_view_host_factory.cc @@ -11,18 +11,19 @@ namespace content { TestRenderViewHostFactory::TestRenderViewHostFactory( - RenderProcessHostFactory* rph_factory) - : render_process_host_factory_(rph_factory) { + RenderProcessHostFactory* rph_factory) { + SiteInstanceImpl::set_render_process_host_factory(rph_factory); RenderViewHostFactory::RegisterFactory(this); } TestRenderViewHostFactory::~TestRenderViewHostFactory() { RenderViewHostFactory::UnregisterFactory(); + SiteInstanceImpl::set_render_process_host_factory(NULL); } void TestRenderViewHostFactory::set_render_process_host_factory( RenderProcessHostFactory* rph_factory) { - render_process_host_factory_ = rph_factory; + SiteInstanceImpl::set_render_process_host_factory(rph_factory); } RenderViewHost* TestRenderViewHostFactory::CreateRenderViewHost( @@ -33,9 +34,6 @@ RenderViewHost* TestRenderViewHostFactory::CreateRenderViewHost( int main_frame_routing_id, bool swapped_out, SessionStorageNamespace* session_storage) { - // See declaration of render_process_host_factory_ below. - static_cast(instance)-> - set_render_process_host_factory(render_process_host_factory_); return new TestRenderViewHost( instance, delegate, widget_delegate, routing_id, main_frame_routing_id, swapped_out); diff --git a/content/test/test_render_view_host_factory.h b/content/test/test_render_view_host_factory.h index 6b9b45e..d194b2c 100644 --- a/content/test/test_render_view_host_factory.h +++ b/content/test/test_render_view_host_factory.h @@ -37,15 +37,6 @@ class TestRenderViewHostFactory : public RenderViewHostFactory { SessionStorageNamespace* session_storage) OVERRIDE; private: - // This is a bit of a hack. With the current design of the site instances / - // browsing instances, it's difficult to pass a RenderProcessHostFactory - // around properly. - // - // Instead, we set it right before we create a new RenderViewHost, which - // happens before the RenderProcessHost is created. This way, the instance - // has the correct factory and creates our special RenderProcessHosts. - RenderProcessHostFactory* render_process_host_factory_; - DISALLOW_COPY_AND_ASSIGN(TestRenderViewHostFactory); }; -- cgit v1.1