diff options
author | jamescook@chromium.org <jamescook@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2014-07-22 02:13:47 +0000 |
---|---|---|
committer | jamescook@chromium.org <jamescook@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2014-07-22 02:13:47 +0000 |
commit | 6b54fda654446046577e4b9c10c48eb61b022ebb (patch) | |
tree | 4707735b1e04dc6669d4baebc6e28dfa31f19c55 /extensions | |
parent | e25bbc6831ccc2d0a924c3b0d8ce79469beff32c (diff) | |
download | chromium_src-6b54fda654446046577e4b9c10c48eb61b022ebb.zip chromium_src-6b54fda654446046577e4b9c10c48eb61b022ebb.tar.gz chromium_src-6b54fda654446046577e4b9c10c48eb61b022ebb.tar.bz2 |
Reland: Refactor code that defers extension background page loading
src/extensions depends on chrome::NOTIFICATION_PROFILE_CREATED to support
deferred loading of extension background pages when the profile isn't ready
yet. This is a layering violation.
* Remove Chrome concepts like "browser window ready" and "profile created"
from ProcessManager. Introduce ProcessManagerDelegate with a general concept
of deferring background page loading.
* Consolidate all the tricky Chrome-specific background page loading rules into
ChromeProcessManagerDelegate. This keeps all the rules in one place. Annotate
each block of special case code with the bug that inspired it.
* Extend unit test coverage for ProcessManager.
This will make it easier to eliminate chrome::NOTIFICATION_PROFILE_DESTROYED
in ProcessManager in a later CL.
(Original CL https://codereview.chromium.org/381283002 broke valgrind bots
because it was initializing left-over BrowserContextKeyedServices from tests
running earlier in the same process.)
BUG=392658
TEST=unit_tests ProcessManagerTest, browser_tests ProcessManagerBrowserTest, manual
Review URL: https://codereview.chromium.org/408523005
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@284593 0039d316-1c4b-4281-b951-d872f2087c98
Diffstat (limited to 'extensions')
-rw-r--r-- | extensions/DEPS | 3 | ||||
-rw-r--r-- | extensions/browser/extension_registry.h | 2 | ||||
-rw-r--r-- | extensions/browser/extensions_browser_client.h | 10 | ||||
-rw-r--r-- | extensions/browser/lazy_background_task_queue_unittest.cc | 5 | ||||
-rw-r--r-- | extensions/browser/process_manager.cc | 159 | ||||
-rw-r--r-- | extensions/browser/process_manager.h | 43 | ||||
-rw-r--r-- | extensions/browser/process_manager_delegate.h | 34 | ||||
-rw-r--r-- | extensions/browser/process_manager_unittest.cc | 131 | ||||
-rw-r--r-- | extensions/browser/test_extensions_browser_client.cc | 15 | ||||
-rw-r--r-- | extensions/browser/test_extensions_browser_client.h | 14 | ||||
-rw-r--r-- | extensions/extensions.gyp | 1 |
11 files changed, 291 insertions, 126 deletions
diff --git a/extensions/DEPS b/extensions/DEPS index 0505b02..4077a55 100644 --- a/extensions/DEPS +++ b/extensions/DEPS @@ -1,4 +1,5 @@ include_rules = [ + # Do not add Chrome dependencies. Much work went into removing them. "+components/url_matcher", "+content/public/common", "+content/public/test", @@ -13,7 +14,7 @@ include_rules = [ # NOTE: Please do not add includes without talking to the app shell team; # see OWNERS for this directory. # - # TODO(jamescook): Remove these. http://crbug.com/162530 + # TODO(jamescook): Remove this. http://crbug.com/392622 "!chrome/browser/chrome_notification_types.h", ] diff --git a/extensions/browser/extension_registry.h b/extensions/browser/extension_registry.h index 75724cd..d11111c 100644 --- a/extensions/browser/extension_registry.h +++ b/extensions/browser/extension_registry.h @@ -43,6 +43,8 @@ class ExtensionRegistry : public KeyedService { // Returns the instance for the given |browser_context|. static ExtensionRegistry* Get(content::BrowserContext* browser_context); + content::BrowserContext* browser_context() const { return browser_context_; } + // NOTE: These sets are *eventually* mututally exclusive, but an extension can // appear in two sets for short periods of time. const ExtensionSet& enabled_extensions() const { diff --git a/extensions/browser/extensions_browser_client.h b/extensions/browser/extensions_browser_client.h index 293b2af..cb810b6 100644 --- a/extensions/browser/extensions_browser_client.h +++ b/extensions/browser/extensions_browser_client.h @@ -41,6 +41,7 @@ class ExtensionPrefsObserver; class ExtensionSystem; class ExtensionSystemProvider; class InfoMap; +class ProcessManagerDelegate; class RuntimeAPIDelegate; // Interface to allow the extensions module to make browser-process-specific @@ -133,12 +134,9 @@ class ExtensionsBrowserClient { content::BrowserContext* context, std::vector<ExtensionPrefsObserver*>* observers) const = 0; - // Returns true if loading background pages should be deferred. - virtual bool DeferLoadingBackgroundHosts( - content::BrowserContext* context) const = 0; - - virtual bool IsBackgroundPageAllowed( - content::BrowserContext* context) const = 0; + // Returns the ProcessManagerDelegate shared across all BrowserContexts. May + // return NULL in tests or for simple embedders. + virtual ProcessManagerDelegate* GetProcessManagerDelegate() const = 0; // Creates a new ExtensionHostDelegate instance. virtual scoped_ptr<ExtensionHostDelegate> CreateExtensionHostDelegate() = 0; diff --git a/extensions/browser/lazy_background_task_queue_unittest.cc b/extensions/browser/lazy_background_task_queue_unittest.cc index 0a991b4..bda663c 100644 --- a/extensions/browser/lazy_background_task_queue_unittest.cc +++ b/extensions/browser/lazy_background_task_queue_unittest.cc @@ -11,6 +11,7 @@ #include "chrome/browser/extensions/test_extension_system.h" #include "chrome/test/base/testing_profile.h" #include "content/public/test/test_browser_thread_bundle.h" +#include "extensions/browser/extension_registry.h" #include "extensions/browser/process_manager.h" #include "extensions/common/extension.h" #include "extensions/common/extension_builder.h" @@ -22,7 +23,9 @@ namespace extensions { class TestProcessManager : public ProcessManager { public: explicit TestProcessManager(Profile* profile) - : ProcessManager(profile, profile->GetOriginalProfile()), + : ProcessManager(profile, + profile->GetOriginalProfile(), + ExtensionRegistry::Get(profile)), create_count_(0) {} virtual ~TestProcessManager() {} diff --git a/extensions/browser/process_manager.cc b/extensions/browser/process_manager.cc index e6beddb..72fbd2c 100644 --- a/extensions/browser/process_manager.cc +++ b/extensions/browser/process_manager.cc @@ -33,6 +33,7 @@ #include "extensions/browser/extension_registry.h" #include "extensions/browser/extension_system.h" #include "extensions/browser/extensions_browser_client.h" +#include "extensions/browser/process_manager_delegate.h" #include "extensions/browser/process_manager_observer.h" #include "extensions/browser/view_type_utils.h" #include "extensions/common/constants.h" @@ -105,7 +106,8 @@ class IncognitoProcessManager : public ProcessManager { public: IncognitoProcessManager(BrowserContext* incognito_context, BrowserContext* original_context, - ProcessManager* original_manager); + ProcessManager* original_manager, + ExtensionRegistry* extension_registry); virtual ~IncognitoProcessManager() {} virtual bool CreateBackgroundHost(const Extension* extension, const GURL& url) OVERRIDE; @@ -190,6 +192,7 @@ struct ProcessManager::BackgroundPageData { // static ProcessManager* ProcessManager::Create(BrowserContext* context) { + ExtensionRegistry* extension_registry = ExtensionRegistry::Get(context); ExtensionsBrowserClient* client = ExtensionsBrowserClient::Get(); if (client->IsGuestSession(context)) { // In the guest session, there is a single off-the-record context. Unlike @@ -197,7 +200,7 @@ ProcessManager* ProcessManager::Create(BrowserContext* context) { // created regardless of whether extensions use "spanning" or "split" // incognito behavior. BrowserContext* original_context = client->GetOriginalContext(context); - return new ProcessManager(context, original_context); + return new ProcessManager(context, original_context, extension_registry); } if (context->IsOffTheRecord()) { @@ -205,31 +208,46 @@ ProcessManager* ProcessManager::Create(BrowserContext* context) { ProcessManager* original_manager = ExtensionSystem::Get(original_context)->process_manager(); return new IncognitoProcessManager( - context, original_context, original_manager); + context, original_context, original_manager, extension_registry); } - return new ProcessManager(context, context); + return new ProcessManager(context, context, extension_registry); +} + +// static +ProcessManager* ProcessManager::CreateForTesting( + BrowserContext* context, + ExtensionRegistry* extension_registry) { + DCHECK(!context->IsOffTheRecord()); + return new ProcessManager(context, context, extension_registry); } // static ProcessManager* ProcessManager::CreateIncognitoForTesting( BrowserContext* incognito_context, BrowserContext* original_context, - ProcessManager* original_manager) { + ProcessManager* original_manager, + ExtensionRegistry* extension_registry) { DCHECK(incognito_context->IsOffTheRecord()); DCHECK(!original_context->IsOffTheRecord()); - return new IncognitoProcessManager( - incognito_context, original_context, original_manager); + return new IncognitoProcessManager(incognito_context, + original_context, + original_manager, + extension_registry); } ProcessManager::ProcessManager(BrowserContext* context, - BrowserContext* original_context) + BrowserContext* original_context, + ExtensionRegistry* extension_registry) : site_instance_(SiteInstance::Create(context)), + extension_registry_(extension_registry), startup_background_hosts_created_(false), devtools_callback_(base::Bind(&ProcessManager::OnDevToolsStateChanged, base::Unretained(this))), last_background_close_sequence_id_(0), weak_ptr_factory_(this) { + // ExtensionRegistry is shared between incognito and regular contexts. + DCHECK_EQ(original_context, extension_registry_->browser_context()); registrar_.Add(this, chrome::NOTIFICATION_EXTENSIONS_READY, content::Source<BrowserContext>(original_context)); registrar_.Add(this, @@ -245,8 +263,6 @@ ProcessManager::ProcessManager(BrowserContext* context, content::NotificationService::AllSources()); registrar_.Add(this, content::NOTIFICATION_WEB_CONTENTS_CONNECTED, content::NotificationService::AllSources()); - registrar_.Add(this, chrome::NOTIFICATION_PROFILE_CREATED, - content::Source<BrowserContext>(original_context)); registrar_.Add(this, chrome::NOTIFICATION_PROFILE_DESTROYED, content::Source<BrowserContext>(context)); if (context->IsOffTheRecord()) { @@ -307,11 +323,14 @@ bool ProcessManager::CreateBackgroundHost(const Extension* extension, const GURL& url) { // Hosted apps are taken care of from BackgroundContentsService. Ignore them // here. - if (extension->is_hosted_app() || - !ExtensionsBrowserClient::Get()-> - IsBackgroundPageAllowed(GetBrowserContext())) { + if (extension->is_hosted_app()) + return false; + + // Don't create hosts if the embedder doesn't allow it. + ProcessManagerDelegate* delegate = + ExtensionsBrowserClient::Get()->GetProcessManagerDelegate(); + if (delegate && !delegate->IsBackgroundPageAllowed(GetBrowserContext())) return false; - } // Don't create multiple background hosts for an extension. if (GetBackgroundHostForExtension(extension->id())) @@ -360,11 +379,7 @@ const Extension* ProcessManager::GetExtensionForRenderViewHost( if (!render_view_host->GetSiteInstance()) return NULL; - ExtensionRegistry* registry = ExtensionRegistry::Get(GetBrowserContext()); - if (!registry) - return NULL; - - return registry->enabled_extensions().GetByID( + return extension_registry_->enabled_extensions().GetByID( GetExtensionID(render_view_host)); } @@ -440,9 +455,7 @@ void ProcessManager::DecrementLazyKeepaliveCount( const std::string& extension_id) { int& count = background_page_data_[extension_id].lazy_keepalive_count; DCHECK(count > 0 || - !ExtensionRegistry::Get(GetBrowserContext()) - ->enabled_extensions() - .Contains(extension_id)); + !extension_registry_->enabled_extensions().Contains(extension_id)); // If we reach a zero keepalive count when the lazy background page is about // to be closed, incrementing close_sequence_id will cancel the close @@ -620,16 +633,6 @@ void ProcessManager::CancelSuspend(const Extension* extension) { } } -void ProcessManager::OnBrowserWindowReady() { - // If the extension system isn't ready yet the background hosts will be - // created via NOTIFICATION_EXTENSIONS_READY below. - ExtensionSystem* system = ExtensionSystem::Get(GetBrowserContext()); - if (!system->ready().is_signaled()) - return; - - CreateBackgroundHostsForProfileStartup(); -} - content::BrowserContext* ProcessManager::GetBrowserContext() const { return site_instance_->GetBrowserContext(); } @@ -648,15 +651,10 @@ void ProcessManager::Observe(int type, const content::NotificationSource& source, const content::NotificationDetails& details) { switch (type) { - case chrome::NOTIFICATION_EXTENSIONS_READY: - case chrome::NOTIFICATION_PROFILE_CREATED: { - // Don't load background hosts now if the loading should be deferred. - // Instead they will be loaded when a browser window for this profile - // (or an incognito profile from this profile) is ready. - if (DeferLoadingBackgroundHosts()) - break; - - CreateBackgroundHostsForProfileStartup(); + case chrome::NOTIFICATION_EXTENSIONS_READY: { + // TODO(jamescook): Convert this to use ExtensionSystem::ready() instead + // of a notification. + MaybeCreateStartupBackgroundHosts(); break; } @@ -784,24 +782,24 @@ void ProcessManager::OnDevToolsStateChanged( } } -void ProcessManager::CreateBackgroundHostsForProfileStartup() { - if (startup_background_hosts_created_ || - !ExtensionsBrowserClient::Get()-> - IsBackgroundPageAllowed(GetBrowserContext())) { +void ProcessManager::MaybeCreateStartupBackgroundHosts() { + if (startup_background_hosts_created_) return; - } - const ExtensionSet& enabled_extensions = - ExtensionRegistry::Get(GetBrowserContext())->enabled_extensions(); - for (ExtensionSet::const_iterator extension = enabled_extensions.begin(); - extension != enabled_extensions.end(); - ++extension) { - CreateBackgroundHostForExtensionLoad(this, extension->get()); + // The embedder might disallow background pages entirely. + ProcessManagerDelegate* delegate = + ExtensionsBrowserClient::Get()->GetProcessManagerDelegate(); + if (delegate && !delegate->IsBackgroundPageAllowed(GetBrowserContext())) + return; - FOR_EACH_OBSERVER(ProcessManagerObserver, - observer_list_, - OnBackgroundHostStartup(*extension)); - } + // The embedder might want to defer background page loading. For example, + // Chrome defers background page loading when it is launched to show the app + // list, then triggers a load later when a browser window opens. + if (delegate && + delegate->DeferCreatingStartupBackgroundHosts(GetBrowserContext())) + return; + + CreateStartupBackgroundHosts(); startup_background_hosts_created_ = true; // Background pages should only be loaded once. To prevent any further loads @@ -810,14 +808,6 @@ void ProcessManager::CreateBackgroundHostsForProfileStartup() { ExtensionsBrowserClient::Get()->GetOriginalContext(GetBrowserContext()); if (registrar_.IsRegistered( this, - chrome::NOTIFICATION_PROFILE_CREATED, - content::Source<BrowserContext>(original_context))) { - registrar_.Remove(this, - chrome::NOTIFICATION_PROFILE_CREATED, - content::Source<BrowserContext>(original_context)); - } - if (registrar_.IsRegistered( - this, chrome::NOTIFICATION_EXTENSIONS_READY, content::Source<BrowserContext>(original_context))) { registrar_.Remove(this, @@ -826,6 +816,21 @@ void ProcessManager::CreateBackgroundHostsForProfileStartup() { } } +void ProcessManager::CreateStartupBackgroundHosts() { + DCHECK(!startup_background_hosts_created_); + const ExtensionSet& enabled_extensions = + extension_registry_->enabled_extensions(); + for (ExtensionSet::const_iterator extension = enabled_extensions.begin(); + extension != enabled_extensions.end(); + ++extension) { + CreateBackgroundHostForExtensionLoad(this, extension->get()); + + FOR_EACH_OBSERVER(ProcessManagerObserver, + observer_list_, + OnBackgroundHostStartup(*extension)); + } +} + void ProcessManager::OnBackgroundHostCreated(ExtensionHost* host) { DCHECK_EQ(GetBrowserContext(), host->browser_context()); background_hosts_.insert(host); @@ -890,12 +895,6 @@ void ProcessManager::ClearBackgroundPageData(const std::string& extension_id) { } } -bool ProcessManager::DeferLoadingBackgroundHosts() const { - // The extensions embedder may have special rules about background hosts. - return ExtensionsBrowserClient::Get()->DeferLoadingBackgroundHosts( - GetBrowserContext()); -} - // // IncognitoProcessManager // @@ -903,8 +902,9 @@ bool ProcessManager::DeferLoadingBackgroundHosts() const { IncognitoProcessManager::IncognitoProcessManager( BrowserContext* incognito_context, BrowserContext* original_context, - ProcessManager* original_manager) - : ProcessManager(incognito_context, original_context), + ProcessManager* original_manager, + ExtensionRegistry* extension_registry) + : ProcessManager(incognito_context, original_context, extension_registry), original_manager_(original_manager) { DCHECK(incognito_context->IsOffTheRecord()); @@ -914,8 +914,6 @@ IncognitoProcessManager::IncognitoProcessManager( // in the NOTIFICATION_BROWSER_WINDOW_READY notification handler. registrar_.Remove(this, chrome::NOTIFICATION_EXTENSIONS_READY, content::Source<BrowserContext>(original_context)); - registrar_.Remove(this, chrome::NOTIFICATION_PROFILE_CREATED, - content::Source<BrowserContext>(original_context)); } bool IncognitoProcessManager::CreateBackgroundHost(const Extension* extension, @@ -932,14 +930,11 @@ bool IncognitoProcessManager::CreateBackgroundHost(const Extension* extension, } SiteInstance* IncognitoProcessManager::GetSiteInstanceForURL(const GURL& url) { - ExtensionRegistry* registry = ExtensionRegistry::Get(GetBrowserContext()); - if (registry) { - const Extension* extension = - registry->enabled_extensions().GetExtensionOrAppByURL(url); - if (extension && !IncognitoInfo::IsSplitMode(extension)) { - return original_manager_->GetSiteInstanceForURL(url); - } - } + const Extension* extension = + extension_registry_->enabled_extensions().GetExtensionOrAppByURL(url); + if (extension && !IncognitoInfo::IsSplitMode(extension)) + return original_manager_->GetSiteInstanceForURL(url); + return ProcessManager::GetSiteInstanceForURL(url); } diff --git a/extensions/browser/process_manager.h b/extensions/browser/process_manager.h index e580bbd..e3c6bcf 100644 --- a/extensions/browser/process_manager.h +++ b/extensions/browser/process_manager.h @@ -33,6 +33,8 @@ namespace extensions { class Extension; class ExtensionHost; +class ExtensionRegistry; +class ProcessManagerDelegate; class ProcessManagerObserver; // Manages dynamic state of running Chromium extensions. There is one instance @@ -122,8 +124,9 @@ class ProcessManager : public content::NotificationObserver { // onSuspendCanceled() event to it. void CancelSuspend(const Extension* extension); - // Ensures background hosts are loaded for a new browser window. - void OnBrowserWindowReady(); + // Creates background hosts if the embedder is ready and they are not already + // loaded. + void MaybeCreateStartupBackgroundHosts(); // Gets the BrowserContext associated with site_instance_ and all other // related SiteInstances. @@ -137,18 +140,30 @@ class ProcessManager : public content::NotificationObserver { void SetKeepaliveImpulseDecrementCallbackForTesting( const ImpulseCallbackForTesting& callback); - // Creates an incognito-context instance for tests. Tests for non-incognito - // contexts can just use Create() above. + // Creates a non-incognito instance for tests. |registry| allows unit tests + // to inject an ExtensionRegistry that is not managed by the usual + // BrowserContextKeyedServiceFactory system. + static ProcessManager* CreateForTesting(content::BrowserContext* context, + ExtensionRegistry* registry); + + // Creates an incognito-context instance for tests. static ProcessManager* CreateIncognitoForTesting( content::BrowserContext* incognito_context, content::BrowserContext* original_context, - ProcessManager* original_manager); + ProcessManager* original_manager, + ExtensionRegistry* registry); + + bool startup_background_hosts_created_for_test() const { + return startup_background_hosts_created_; + } protected: // If |context| is incognito pass the master context as |original_context|. - // Otherwise pass the same context for both. + // Otherwise pass the same context for both. Pass the ExtensionRegistry for + // |context| as |registry|, or override it for testing. ProcessManager(content::BrowserContext* context, - content::BrowserContext* original_context); + content::BrowserContext* original_context, + ExtensionRegistry* registry); // Called on browser shutdown to close our extension hosts. void CloseBackgroundHosts(); @@ -158,10 +173,6 @@ class ProcessManager : public content::NotificationObserver { const content::NotificationSource& source, const content::NotificationDetails& details) OVERRIDE; - // Load all background pages once the profile data is ready and the pages - // should be loaded. - void CreateBackgroundHostsForProfileStartup(); - content::NotificationRegistrar registrar_; // The set of ExtensionHosts running viewless background extensions. @@ -172,6 +183,9 @@ class ProcessManager : public content::NotificationObserver { // browsing instance is created. This controls process grouping. scoped_refptr<content::SiteInstance> site_instance_; + // Not owned. Also used by IncognitoProcessManager. + ExtensionRegistry* extension_registry_; + private: friend class ProcessManagerTest; @@ -182,6 +196,10 @@ class ProcessManager : public content::NotificationObserver { typedef std::map<content::RenderViewHost*, extensions::ViewType> ExtensionRenderViews; + // Load all background pages once the profile data is ready and the pages + // should be loaded. + void CreateStartupBackgroundHosts(); + // Called just after |host| is created so it can be registered in our lists. void OnBackgroundHostCreated(ExtensionHost* host); @@ -216,9 +234,6 @@ class ProcessManager : public content::NotificationObserver { // Clears background page data for this extension. void ClearBackgroundPageData(const std::string& extension_id); - // Returns true if loading background pages should be deferred. - bool DeferLoadingBackgroundHosts() const; - void OnDevToolsStateChanged(content::DevToolsAgentHost*, bool attached); // Contains all active extension-related RenderViewHost instances for all diff --git a/extensions/browser/process_manager_delegate.h b/extensions/browser/process_manager_delegate.h new file mode 100644 index 0000000..7cc48c0 --- /dev/null +++ b/extensions/browser/process_manager_delegate.h @@ -0,0 +1,34 @@ +// Copyright 2014 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. + +#ifndef EXTENSIONS_BROWSER_PROCESS_MANAGER_DELEGATE_H_ +#define EXTENSIONS_BROWSER_PROCESS_MANAGER_DELEGATE_H_ + +namespace content { +class BrowserContext; +}; + +namespace extensions { + +// Customization of ProcessManager for the extension system embedder. +class ProcessManagerDelegate { + public: + virtual ~ProcessManagerDelegate() {} + + // Returns true if the embedder allows background pages for the given + // |context|. + virtual bool IsBackgroundPageAllowed( + content::BrowserContext* context) const = 0; + + // Returns true if the embedder wishes to defer starting up the renderers for + // extension background pages. If the embedder returns true it must call + // ProcessManager::MaybeCreateStartupBackgroundHosts() when it is ready. See + // ChromeProcessManagerDelegate for examples of how this is useful. + virtual bool DeferCreatingStartupBackgroundHosts( + content::BrowserContext* context) const = 0; +}; + +} // namespace extensions + +#endif // EXTENSIONS_BROWSER_PROCESS_MANAGER_DELEGATE_H_ diff --git a/extensions/browser/process_manager_unittest.cc b/extensions/browser/process_manager_unittest.cc index b72b112..e5fc807 100644 --- a/extensions/browser/process_manager_unittest.cc +++ b/extensions/browser/process_manager_unittest.cc @@ -9,6 +9,8 @@ #include "content/public/browser/notification_service.h" #include "content/public/browser/site_instance.h" #include "content/public/test/test_browser_context.h" +#include "extensions/browser/extension_registry.h" +#include "extensions/browser/process_manager_delegate.h" #include "extensions/browser/test_extensions_browser_client.h" #include "testing/gtest/include/gtest/gtest.h" @@ -33,12 +35,37 @@ class TestBrowserContextIncognito : public TestBrowserContext { DISALLOW_COPY_AND_ASSIGN(TestBrowserContextIncognito); }; +// A trivial ProcessManagerDelegate. +class TestProcessManagerDelegate : public ProcessManagerDelegate { + public: + TestProcessManagerDelegate() + : is_background_page_allowed_(true), + defer_creating_startup_background_hosts_(false) {} + virtual ~TestProcessManagerDelegate() {} + + // ProcessManagerDelegate implementation. + virtual bool IsBackgroundPageAllowed(BrowserContext* context) const OVERRIDE { + return is_background_page_allowed_; + } + virtual bool DeferCreatingStartupBackgroundHosts( + BrowserContext* context) const OVERRIDE { + return defer_creating_startup_background_hosts_; + } + + bool is_background_page_allowed_; + bool defer_creating_startup_background_hosts_; +}; + } // namespace class ProcessManagerTest : public testing::Test { public: - ProcessManagerTest() : extensions_browser_client_(&original_context_) { + ProcessManagerTest() + : extension_registry_(&original_context_), + extensions_browser_client_(&original_context_) { extensions_browser_client_.SetIncognitoContext(&incognito_context_); + extensions_browser_client_.set_process_manager_delegate( + &process_manager_delegate_); ExtensionsBrowserClient::Set(&extensions_browser_client_); } @@ -48,6 +75,10 @@ class ProcessManagerTest : public testing::Test { BrowserContext* original_context() { return &original_context_; } BrowserContext* incognito_context() { return &incognito_context_; } + ExtensionRegistry* extension_registry() { return &extension_registry_; } + TestProcessManagerDelegate* process_manager_delegate() { + return &process_manager_delegate_; + } // Returns true if the notification |type| is registered for |manager| with // source |context|. Pass NULL for |context| for all sources. @@ -61,6 +92,8 @@ class ProcessManagerTest : public testing::Test { private: TestBrowserContext original_context_; TestBrowserContextIncognito incognito_context_; + ExtensionRegistry extension_registry_; // Shared between BrowserContexts. + TestProcessManagerDelegate process_manager_delegate_; TestExtensionsBrowserClient extensions_browser_client_; DISALLOW_COPY_AND_ASSIGN(ProcessManagerTest); @@ -69,8 +102,8 @@ class ProcessManagerTest : public testing::Test { // Test that notification registration works properly. TEST_F(ProcessManagerTest, ExtensionNotificationRegistration) { // Test for a normal context ProcessManager. - scoped_ptr<ProcessManager> manager1( - ProcessManager::Create(original_context())); + scoped_ptr<ProcessManager> manager1(ProcessManager::CreateForTesting( + original_context(), extension_registry())); EXPECT_EQ(original_context(), manager1->GetBrowserContext()); EXPECT_EQ(0u, manager1->background_hosts().size()); @@ -90,8 +123,11 @@ TEST_F(ProcessManagerTest, ExtensionNotificationRegistration) { original_context())); // Test for an incognito context ProcessManager. - scoped_ptr<ProcessManager> manager2(ProcessManager::CreateIncognitoForTesting( - incognito_context(), original_context(), manager1.get())); + scoped_ptr<ProcessManager> manager2( + ProcessManager::CreateIncognitoForTesting(incognito_context(), + original_context(), + manager1.get(), + extension_registry())); EXPECT_EQ(incognito_context(), manager2->GetBrowserContext()); EXPECT_EQ(0u, manager2->background_hosts().size()); @@ -125,6 +161,83 @@ TEST_F(ProcessManagerTest, ExtensionNotificationRegistration) { incognito_context())); } +// Test that startup background hosts are created when the extension system +// becomes ready. +// +// NOTE: This test and those that follow do not try to create ExtensionsHosts +// because ExtensionHost is tightly coupled to WebContents and can't be +// constructed in unit tests. +TEST_F(ProcessManagerTest, CreateBackgroundHostsOnExtensionsReady) { + scoped_ptr<ProcessManager> manager(ProcessManager::CreateForTesting( + original_context(), extension_registry())); + ASSERT_FALSE(manager->startup_background_hosts_created_for_test()); + + // Simulate the extension system becoming ready. + content::NotificationService::current()->Notify( + chrome::NOTIFICATION_EXTENSIONS_READY, + content::Source<BrowserContext>(original_context()), + content::NotificationService::NoDetails()); + EXPECT_TRUE(manager->startup_background_hosts_created_for_test()); +} + +// Test that startup background hosts can be created explicitly before the +// extension system is ready (this is the normal pattern in Chrome). +TEST_F(ProcessManagerTest, CreateBackgroundHostsExplicitly) { + scoped_ptr<ProcessManager> manager(ProcessManager::CreateForTesting( + original_context(), extension_registry())); + ASSERT_FALSE(manager->startup_background_hosts_created_for_test()); + + // Embedder explicitly asks for hosts to be created. Chrome does this on + // normal startup. + manager->MaybeCreateStartupBackgroundHosts(); + EXPECT_TRUE(manager->startup_background_hosts_created_for_test()); +} + +// Test that the embedder can defer background host creation. Chrome does this +// when the profile is created asynchronously, which may take a while. +TEST_F(ProcessManagerTest, CreateBackgroundHostsDeferred) { + scoped_ptr<ProcessManager> manager(ProcessManager::CreateForTesting( + original_context(), extension_registry())); + ASSERT_FALSE(manager->startup_background_hosts_created_for_test()); + + // Don't create background hosts if the delegate says to defer them. + process_manager_delegate()->defer_creating_startup_background_hosts_ = true; + manager->MaybeCreateStartupBackgroundHosts(); + EXPECT_FALSE(manager->startup_background_hosts_created_for_test()); + + // The extension system becoming ready still doesn't create the hosts. + content::NotificationService::current()->Notify( + chrome::NOTIFICATION_EXTENSIONS_READY, + content::Source<BrowserContext>(original_context()), + content::NotificationService::NoDetails()); + EXPECT_FALSE(manager->startup_background_hosts_created_for_test()); + + // Once the embedder is ready the background hosts can be created. + process_manager_delegate()->defer_creating_startup_background_hosts_ = false; + manager->MaybeCreateStartupBackgroundHosts(); + EXPECT_TRUE(manager->startup_background_hosts_created_for_test()); +} + +// Test that the embedder can disallow background host creation. +// Chrome OS does this in guest mode. +TEST_F(ProcessManagerTest, IsBackgroundHostAllowed) { + scoped_ptr<ProcessManager> manager(ProcessManager::CreateForTesting( + original_context(), extension_registry())); + ASSERT_FALSE(manager->startup_background_hosts_created_for_test()); + + // Don't create background hosts if the delegate disallows them. + process_manager_delegate()->is_background_page_allowed_ = false; + manager->MaybeCreateStartupBackgroundHosts(); + EXPECT_FALSE(manager->startup_background_hosts_created_for_test()); + + // The extension system becoming ready still doesn't create the hosts. + content::NotificationService::current()->Notify( + chrome::NOTIFICATION_EXTENSIONS_READY, + content::Source<BrowserContext>(original_context()), + content::NotificationService::NoDetails()); + EXPECT_FALSE(manager->startup_background_hosts_created_for_test()); +} + // Test that extensions get grouped in the right SiteInstance (and therefore // process) based on their URLs. TEST_F(ProcessManagerTest, ProcessGrouping) { @@ -133,12 +246,14 @@ TEST_F(ProcessManagerTest, ProcessGrouping) { // Extensions in different browser contexts should always be different // SiteInstances. - scoped_ptr<ProcessManager> manager1( - ProcessManager::Create(original_context())); + scoped_ptr<ProcessManager> manager1(ProcessManager::CreateForTesting( + original_context(), extension_registry())); // NOTE: This context is not associated with the TestExtensionsBrowserClient. // That's OK because we're not testing regular vs. incognito behavior. TestBrowserContext another_context; - scoped_ptr<ProcessManager> manager2(ProcessManager::Create(&another_context)); + ExtensionRegistry another_registry(&another_context); + scoped_ptr<ProcessManager> manager2( + ProcessManager::CreateForTesting(&another_context, &another_registry)); // Extensions with common origins ("scheme://id/") should be grouped in the // same SiteInstance. diff --git a/extensions/browser/test_extensions_browser_client.cc b/extensions/browser/test_extensions_browser_client.cc index dcd467f..a01b128 100644 --- a/extensions/browser/test_extensions_browser_client.cc +++ b/extensions/browser/test_extensions_browser_client.cc @@ -15,7 +15,9 @@ namespace extensions { TestExtensionsBrowserClient::TestExtensionsBrowserClient( BrowserContext* main_context) - : main_context_(main_context), incognito_context_(NULL) { + : main_context_(main_context), + incognito_context_(NULL), + process_manager_delegate_(NULL) { DCHECK(main_context_); DCHECK(!main_context_->IsOffTheRecord()); } @@ -116,14 +118,9 @@ void TestExtensionsBrowserClient::GetEarlyExtensionPrefsObservers( content::BrowserContext* context, std::vector<ExtensionPrefsObserver*>* observers) const {} -bool TestExtensionsBrowserClient::DeferLoadingBackgroundHosts( - BrowserContext* context) const { - return false; -} - -bool TestExtensionsBrowserClient::IsBackgroundPageAllowed( - BrowserContext* context) const { - return true; +ProcessManagerDelegate* TestExtensionsBrowserClient::GetProcessManagerDelegate() + const { + return process_manager_delegate_; } scoped_ptr<ExtensionHostDelegate> diff --git a/extensions/browser/test_extensions_browser_client.h b/extensions/browser/test_extensions_browser_client.h index b793259..1cfffee 100644 --- a/extensions/browser/test_extensions_browser_client.h +++ b/extensions/browser/test_extensions_browser_client.h @@ -15,10 +15,14 @@ namespace extensions { // this class should call ExtensionsBrowserClient::Set() with its instance. class TestExtensionsBrowserClient : public ExtensionsBrowserClient { public: - // |context| is required and must not be an incognito context. + // |main_context| is required and must not be an incognito context. explicit TestExtensionsBrowserClient(content::BrowserContext* main_context); virtual ~TestExtensionsBrowserClient(); + void set_process_manager_delegate(ProcessManagerDelegate* delegate) { + process_manager_delegate_ = delegate; + } + // Associates an incognito context with |main_context_|. void SetIncognitoContext(content::BrowserContext* incognito_context); @@ -59,10 +63,7 @@ class TestExtensionsBrowserClient : public ExtensionsBrowserClient { virtual void GetEarlyExtensionPrefsObservers( content::BrowserContext* context, std::vector<ExtensionPrefsObserver*>* observers) const OVERRIDE; - virtual bool DeferLoadingBackgroundHosts( - content::BrowserContext* context) const OVERRIDE; - virtual bool IsBackgroundPageAllowed(content::BrowserContext* context) const - OVERRIDE; + virtual ProcessManagerDelegate* GetProcessManagerDelegate() const OVERRIDE; virtual scoped_ptr<ExtensionHostDelegate> CreateExtensionHostDelegate() OVERRIDE; virtual bool DidVersionUpdate(content::BrowserContext* context) OVERRIDE; @@ -82,6 +83,9 @@ class TestExtensionsBrowserClient : public ExtensionsBrowserClient { content::BrowserContext* main_context_; // Not owned. content::BrowserContext* incognito_context_; // Not owned, defaults to NULL. + // Not owned, defaults to NULL. + ProcessManagerDelegate* process_manager_delegate_; + DISALLOW_COPY_AND_ASSIGN(TestExtensionsBrowserClient); }; diff --git a/extensions/extensions.gyp b/extensions/extensions.gyp index 447dc32..35e8f38 100644 --- a/extensions/extensions.gyp +++ b/extensions/extensions.gyp @@ -433,6 +433,7 @@ 'browser/pref_names.h', 'browser/process_manager.cc', 'browser/process_manager.h', + 'browser/process_manager_delegate.h', 'browser/process_manager_observer.h', 'browser/process_map.cc', 'browser/process_map.h', |