Fix app_shell shutdown crash due to BrowserContextKeyedServices

* Create ExtensionSystem and register its dependencies from the ExtensionsBrowserClient interface.
* Create and tear down BrowserContextKeyedServices using the dependency manager like you're supposed to do it.
* Don't register AppLoaderServiceFactory - we don't use it anymore.
* Remove the "custom instance" code to ExtensionSystem::Get() - it was kind of hacky.

BUG=none
TEST=Launch app_shell then close the window. No crash.

Review URL: https://codereview.chromium.org/118043003

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@244013 0039d316-1c4b-4281-b951-d872f2087c98
parent ebf2c8fc
......@@ -4,25 +4,14 @@
#include "apps/shell/shell_browser_context.h"
#include "apps/app_load_service_factory.h"
namespace {
// See ChromeBrowserMainExtraPartsProfiles for details.
void EnsureBrowserContextKeyedServiceFactoriesBuilt() {
apps::AppLoadServiceFactory::GetInstance();
}
} // namespace
namespace apps {
// TODO(jamescook): Should this be an off-the-record context?
// TODO(jamescook): Could initialize NetLog here to get logs from the networking
// stack.
// Create a normal recording browser context. If we used an incognito context
// then app_shell would also have to create a normal context and manage both.
ShellBrowserContext::ShellBrowserContext()
: content::ShellBrowserContext(false, NULL) {
EnsureBrowserContextKeyedServiceFactoriesBuilt();
// TODO(jamescook): Could initialize NetLog here to get logs from the
// networking stack.
}
ShellBrowserContext::~ShellBrowserContext() {
......
......@@ -13,9 +13,10 @@
#include "base/files/file_path.h"
#include "base/path_service.h"
#include "base/run_loop.h"
#include "chrome/browser/extensions/extension_system_factory.h"
#include "chrome/browser/extensions/extension_system.h"
#include "chrome/common/chrome_paths.h"
#include "chromeos/chromeos_paths.h"
#include "components/browser_context_keyed_service/browser_context_dependency_manager.h"
#include "content/public/common/result_codes.h"
#include "extensions/common/extension_paths.h"
#include "ui/aura/env.h"
......@@ -27,8 +28,18 @@
using content::BrowserContext;
using extensions::Extension;
using extensions::ExtensionSystem;
using extensions::ShellExtensionSystem;
namespace apps {
namespace {
void EnsureBrowserContextKeyedServiceFactoriesBuilt() {
// TODO(jamescook): Register additional BrowserContextKeyedService factories
// here. See ChromeBrowserMainExtraPartsProfiles for details.
}
} // namespace
ShellBrowserMainParts::ShellBrowserMainParts(
const content::MainFunctionParams& parameters)
......@@ -77,15 +88,16 @@ void ShellBrowserMainParts::PreMainMessageLoopRun() {
extensions::ExtensionsClient::Set(extensions_client_.get());
extensions_browser_client_.reset(
new ShellExtensionsBrowserClient(browser_context_.get()));
new extensions::ShellExtensionsBrowserClient(browser_context_.get()));
extensions::ExtensionsBrowserClient::Set(extensions_browser_client_.get());
// TODO(jamescook): Initialize policy::ProfilePolicyConnector.
// Create our custom ExtensionSystem first because other
// BrowserContextKeyedServices depend on it.
CreateExtensionSystem();
// TODO(jamescook): Create the rest of the services using
// BrowserContextDependencyManager::CreateBrowserContextServices.
EnsureBrowserContextKeyedServiceFactoriesBuilt();
BrowserContextDependencyManager::GetInstance()->CreateBrowserContextServices(
browser_context_.get());
CreateRootWindow();
......@@ -111,16 +123,13 @@ bool ShellBrowserMainParts::MainMessageLoopRun(int* result_code) {
void ShellBrowserMainParts::PostMainMessageLoopRun() {
DestroyRootWindow();
// TODO(jamescook): Destroy the rest of the services with
// BrowserContextDependencyManager::DestroyBrowserContextServices.
BrowserContextDependencyManager::GetInstance()->DestroyBrowserContextServices(
browser_context_.get());
extension_system_ = NULL;
extensions::ExtensionsBrowserClient::Set(NULL);
extensions_browser_client_.reset();
browser_context_.reset();
aura::Env::DeleteInstance();
LOG(WARNING) << "-----------------------------------";
LOG(WARNING) << "app_shell is expected to crash now.";
LOG(WARNING) << "-----------------------------------";
}
void ShellBrowserMainParts::OnWindowTreeHostCloseRequested(
......@@ -149,12 +158,8 @@ void ShellBrowserMainParts::DestroyRootWindow() {
void ShellBrowserMainParts::CreateExtensionSystem() {
DCHECK(browser_context_);
extension_system_ =
new extensions::ShellExtensionSystem(browser_context_.get());
extensions::ExtensionSystemFactory::GetInstance()->SetCustomInstance(
extension_system_);
// Must occur after setting the instance above, as it will end up calling
// ExtensionSystem::Get().
extension_system_ = static_cast<ShellExtensionSystem*>(
ExtensionSystem::GetForBrowserContext(browser_context_.get()));
extension_system_->InitForRegularProfile(true);
}
......
......@@ -21,6 +21,7 @@ struct MainFunctionParams;
}
namespace extensions {
class ShellExtensionsBrowserClient;
class ShellExtensionSystem;
}
......@@ -31,7 +32,6 @@ class WMTestHelper;
namespace apps {
class ShellBrowserContext;
class ShellExtensionsBrowserClient;
class ShellExtensionsClient;
// Handles initialization of AppShell.
......@@ -71,7 +71,8 @@ class ShellBrowserMainParts : public content::BrowserMainParts,
scoped_ptr<ShellBrowserContext> browser_context_;
scoped_ptr<ShellExtensionsClient> extensions_client_;
scoped_ptr<ShellExtensionsBrowserClient> extensions_browser_client_;
scoped_ptr<extensions::ShellExtensionsBrowserClient>
extensions_browser_client_;
// Enable a minimal set of views::corewm to be initialized.
scoped_ptr<wm::WMTestHelper> wm_test_helper_;
......
......@@ -5,26 +5,28 @@
#include "apps/shell/shell_extensions_browser_client.h"
#include "apps/shell/shell_app_sorting.h"
#include "apps/shell/shell_extension_system.h"
#include "base/prefs/pref_service.h"
#include "base/prefs/pref_service_factory.h"
#include "base/prefs/testing_pref_store.h"
#include "chrome/browser/extensions/extension_prefs.h"
#include "chrome/browser/extensions/extension_prefs_factory.h"
#include "components/user_prefs/pref_registry_syncable.h"
#include "components/user_prefs/user_prefs.h"
#include "extensions/browser/app_sorting.h"
using content::BrowserContext;
namespace extensions {
namespace {
// See chrome::RegisterProfilePrefs() in chrome/browser/prefs/browser_prefs.cc
void RegisterPrefs(user_prefs::PrefRegistrySyncable* registry) {
extensions::ExtensionPrefs::RegisterProfilePrefs(registry);
ExtensionPrefs::RegisterProfilePrefs(registry);
}
} // namespace
namespace apps {
ShellExtensionsBrowserClient::ShellExtensionsBrowserClient(
BrowserContext* context)
......@@ -101,9 +103,8 @@ bool ShellExtensionsBrowserClient::DidVersionUpdate(BrowserContext* context) {
return false;
}
scoped_ptr<extensions::AppSorting>
ShellExtensionsBrowserClient::CreateAppSorting() {
return scoped_ptr<extensions::AppSorting>(new ShellAppSorting).Pass();
scoped_ptr<AppSorting> ShellExtensionsBrowserClient::CreateAppSorting() {
return scoped_ptr<AppSorting>(new apps::ShellAppSorting).Pass();
}
bool ShellExtensionsBrowserClient::IsRunningInForcedAppMode() {
......@@ -118,4 +119,16 @@ ShellExtensionsBrowserClient::GetJavaScriptDialogManager() {
return NULL;
}
} // namespace apps
std::vector<BrowserContextKeyedServiceFactory*>
ShellExtensionsBrowserClient::GetExtensionSystemDependencies() {
std::vector<BrowserContextKeyedServiceFactory*> depends_on;
depends_on.push_back(ExtensionPrefsFactory::GetInstance());
return depends_on;
}
ExtensionSystem* ShellExtensionsBrowserClient::CreateExtensionSystem(
BrowserContext* context) {
return new ShellExtensionSystem(context);
}
} // namespace extensions
......@@ -10,18 +10,17 @@
class PrefService;
namespace apps {
namespace extensions {
// An ExtensionsBrowserClient that supports a single content::BrowserContent
// with no related incognito context.
class ShellExtensionsBrowserClient
: public extensions::ExtensionsBrowserClient {
class ShellExtensionsBrowserClient : public ExtensionsBrowserClient {
public:
// |context| is the single BrowserContext used for IsValidContext() below.
explicit ShellExtensionsBrowserClient(content::BrowserContext* context);
virtual ~ShellExtensionsBrowserClient();
// extensions::ExtensionsBrowserClient overrides:
// ExtensionsBrowserClient overrides:
virtual bool IsShuttingDown() OVERRIDE;
virtual bool AreExtensionsDisabled(const CommandLine& command_line,
content::BrowserContext* context) OVERRIDE;
......@@ -41,10 +40,14 @@ class ShellExtensionsBrowserClient
virtual bool IsBackgroundPageAllowed(content::BrowserContext* context)
const OVERRIDE;
virtual bool DidVersionUpdate(content::BrowserContext* context) OVERRIDE;
virtual scoped_ptr<extensions::AppSorting> CreateAppSorting() OVERRIDE;
virtual scoped_ptr<AppSorting> CreateAppSorting() OVERRIDE;
virtual bool IsRunningInForcedAppMode() OVERRIDE;
virtual content::JavaScriptDialogManager* GetJavaScriptDialogManager()
OVERRIDE;
virtual std::vector<BrowserContextKeyedServiceFactory*>
GetExtensionSystemDependencies() OVERRIDE;
virtual ExtensionSystem* CreateExtensionSystem(
content::BrowserContext* context) OVERRIDE;
private:
// The single BrowserContext for app_shell. Not owned.
......@@ -56,6 +59,6 @@ class ShellExtensionsBrowserClient
DISALLOW_COPY_AND_ASSIGN(ShellExtensionsBrowserClient);
};
} // namespace apps
} // namespace extensions
#endif // APPS_SHELL_SHELL_EXTENSIONS_BROWSER_CLIENT_H_
......@@ -540,6 +540,18 @@ void DriveIntegrationService::AvoidDriveAsDownloadDirecotryPreference() {
//===================== DriveIntegrationServiceFactory =======================
DriveIntegrationServiceFactory::FactoryCallback*
DriveIntegrationServiceFactory::factory_for_test_ = NULL;
DriveIntegrationServiceFactory::ScopedFactoryForTest::ScopedFactoryForTest(
FactoryCallback* factory_for_test) {
factory_for_test_ = factory_for_test;
}
DriveIntegrationServiceFactory::ScopedFactoryForTest::~ScopedFactoryForTest() {
factory_for_test_ = NULL;
}
// static
DriveIntegrationService* DriveIntegrationServiceFactory::GetForProfile(
Profile* profile) {
......@@ -573,12 +585,6 @@ DriveIntegrationServiceFactory* DriveIntegrationServiceFactory::GetInstance() {
return Singleton<DriveIntegrationServiceFactory>::get();
}
// static
void DriveIntegrationServiceFactory::SetFactoryForTest(
const FactoryCallback& factory_for_test) {
GetInstance()->factory_for_test_ = factory_for_test;
}
DriveIntegrationServiceFactory::DriveIntegrationServiceFactory()
: BrowserContextKeyedServiceFactory(
"DriveIntegrationService",
......@@ -597,7 +603,7 @@ DriveIntegrationServiceFactory::BuildServiceInstanceFor(
Profile* profile = Profile::FromBrowserContext(context);
DriveIntegrationService* service = NULL;
if (factory_for_test_.is_null()) {
if (!factory_for_test_) {
DriveIntegrationService::PreferenceWatcher* preference_watcher = NULL;
if (chromeos::IsProfileAssociatedWithGaiaAccount(profile)) {
// Drive File System can be enabled.
......@@ -608,7 +614,7 @@ DriveIntegrationServiceFactory::BuildServiceInstanceFor(
service = new DriveIntegrationService(profile, preference_watcher,
NULL, base::FilePath(), NULL);
} else {
service = factory_for_test_.Run(profile);
service = factory_for_test_->Run(profile);
}
return service;
......
......@@ -188,6 +188,14 @@ class DriveIntegrationServiceFactory
typedef base::Callback<DriveIntegrationService*(Profile* profile)>
FactoryCallback;
// Sets and resets a factory function for tests. See below for why we can't
// use BrowserContextKeyedServiceFactory::SetTestingFactory().
class ScopedFactoryForTest {
public:
explicit ScopedFactoryForTest(FactoryCallback* factory_for_test);
~ScopedFactoryForTest();
};
// Returns the DriveIntegrationService for |profile|, creating it if it is
// not yet created.
static DriveIntegrationService* GetForProfile(Profile* profile);
......@@ -207,9 +215,6 @@ class DriveIntegrationServiceFactory
// Returns the DriveIntegrationServiceFactory instance.
static DriveIntegrationServiceFactory* GetInstance();
// Sets a factory function for tests.
static void SetFactoryForTest(const FactoryCallback& factory_for_test);
private:
friend struct DefaultSingletonTraits<DriveIntegrationServiceFactory>;
......@@ -220,7 +225,12 @@ class DriveIntegrationServiceFactory
virtual BrowserContextKeyedService* BuildServiceInstanceFor(
content::BrowserContext* context) const OVERRIDE;
FactoryCallback factory_for_test_;
// This is static so it can be set without instantiating the factory. This
// allows factory creation to be delayed until it normally happens (on profile
// creation) rather than when tests are set up. DriveIntegrationServiceFactory
// transitively depends on ExtensionSystemFactory which crashes if created too
// soon (i.e. before the BrowserProcess exists).
static FactoryCallback* factory_for_test_;
};
} // namespace drive
......
......@@ -43,6 +43,7 @@
// - Doing searches on drive file system from file browser extension (using
// fileBrowserPrivate API).
using drive::DriveIntegrationServiceFactory;
using extensions::Extension;
namespace file_manager {
......@@ -298,10 +299,13 @@ class DriveFileSystemExtensionApiTest : public FileSystemExtensionApiTestBase {
// initialized by EventRouter.
ASSERT_TRUE(test_cache_root_.CreateUniqueTempDir());
drive::DriveIntegrationServiceFactory::SetFactoryForTest(
base::Bind(
// This callback will get called during Profile creation.
create_drive_integration_service_ = base::Bind(
&DriveFileSystemExtensionApiTest::CreateDriveIntegrationService,
base::Unretained(this)));
base::Unretained(this));
service_factory_for_test_.reset(
new DriveIntegrationServiceFactory::ScopedFactoryForTest(
&create_drive_integration_service_));
}
// FileSystemExtensionApiTestBase OVERRIDE.
......@@ -326,6 +330,10 @@ class DriveFileSystemExtensionApiTest : public FileSystemExtensionApiTestBase {
base::ScopedTempDir test_cache_root_;
drive::FakeDriveService* fake_drive_service_;
DriveIntegrationServiceFactory::FactoryCallback
create_drive_integration_service_;
scoped_ptr<DriveIntegrationServiceFactory::ScopedFactoryForTest>
service_factory_for_test_;
};
//
......
......@@ -42,6 +42,8 @@
#include "net/test/embedded_test_server/embedded_test_server.h"
#include "webkit/browser/fileapi/external_mount_points.h"
using drive::DriveIntegrationServiceFactory;
namespace file_manager {
namespace {
......@@ -270,14 +272,17 @@ class DriveTestVolume {
}
// Sends request to add this volume to the file system as Google drive.
// This method must be calld at SetUp method of FileManagerBrowserTestBase.
// This method must be called at SetUp method of FileManagerBrowserTestBase.
// Returns true on success.
bool SetUp() {
if (!test_cache_root_.CreateUniqueTempDir())
return false;
drive::DriveIntegrationServiceFactory::SetFactoryForTest(
create_drive_integration_service_ =
base::Bind(&DriveTestVolume::CreateDriveIntegrationService,
base::Unretained(this)));
base::Unretained(this));
service_factory_for_test_.reset(
new DriveIntegrationServiceFactory::ScopedFactoryForTest(
&create_drive_integration_service_));
return true;
}
......@@ -414,6 +419,10 @@ class DriveTestVolume {
base::ScopedTempDir test_cache_root_;
drive::FakeDriveService* fake_drive_service_;
drive::DriveIntegrationService* integration_service_;
DriveIntegrationServiceFactory::FactoryCallback
create_drive_integration_service_;
scoped_ptr<DriveIntegrationServiceFactory::ScopedFactoryForTest>
service_factory_for_test_;
};
// Listener to obtain the test relative messages synchronously.
......
......@@ -37,13 +37,10 @@ namespace {
class SyncFileSystemApiTest : public ExtensionApiTest {
public:
SyncFileSystemApiTest() {}
SyncFileSystemApiTest()
: mock_remote_service_(NULL), real_default_quota_(0) {}
virtual void SetUpInProcessBrowserTestFixture() OVERRIDE {
mock_remote_service_ = new ::testing::NiceMock<MockRemoteFileSyncService>;
SyncFileSystemServiceFactory::GetInstance()->set_mock_remote_file_service(
scoped_ptr<RemoteFileSyncService>(mock_remote_service_));
ExtensionApiTest::SetUpInProcessBrowserTestFixture();
// TODO(calvinlo): Update test code after default quota is made const
// (http://crbug.com/155488).
......@@ -56,6 +53,16 @@ class SyncFileSystemApiTest : public ExtensionApiTest {
ExtensionApiTest::TearDownInProcessBrowserTestFixture();
}
virtual void SetUpOnMainThread() OVERRIDE {
// Must happen after the browser process is created because instantiating
// the factory will instantiate ExtensionSystemFactory which depends on
// ExtensionsBrowserClient setup in BrowserProcessImpl.
mock_remote_service_ = new ::testing::NiceMock<MockRemoteFileSyncService>;
SyncFileSystemServiceFactory::GetInstance()->set_mock_remote_file_service(
scoped_ptr<RemoteFileSyncService>(mock_remote_service_));
ExtensionApiTest::SetUpOnMainThread();
}
::testing::NiceMock<MockRemoteFileSyncService>* mock_remote_service() {
return mock_remote_service_;
}
......
......@@ -10,6 +10,8 @@
#include "chrome/browser/browser_process.h"
#include "chrome/browser/extensions/chrome_app_sorting.h"
#include "chrome/browser/extensions/extension_prefs.h"
#include "chrome/browser/extensions/extension_system.h"
#include "chrome/browser/extensions/extension_system_factory.h"
#include "chrome/browser/profiles/profile.h"
#include "chrome/browser/profiles/profile_manager.h"
#include "chrome/browser/ui/app_modal_dialogs/javascript_dialog_manager.h"
......@@ -154,4 +156,16 @@ ChromeExtensionsBrowserClient::GetJavaScriptDialogManager() {
return GetJavaScriptDialogManagerInstance();
}
std::vector<BrowserContextKeyedServiceFactory*>
ChromeExtensionsBrowserClient::GetExtensionSystemDependencies() {
std::vector<BrowserContextKeyedServiceFactory*> dependencies;
dependencies.push_back(ExtensionSystemSharedFactory::GetInstance());
return dependencies;
}
ExtensionSystem* ChromeExtensionsBrowserClient::CreateExtensionSystem(
content::BrowserContext* context) {
return new ExtensionSystemImpl(static_cast<Profile*>(context));
}
} // namespace extensions
......@@ -55,6 +55,10 @@ class ChromeExtensionsBrowserClient : public ExtensionsBrowserClient {
virtual bool IsRunningInForcedAppMode() OVERRIDE;
virtual content::JavaScriptDialogManager* GetJavaScriptDialogManager()
OVERRIDE;
virtual std::vector<BrowserContextKeyedServiceFactory*>
GetExtensionSystemDependencies() OVERRIDE;
virtual ExtensionSystem* CreateExtensionSystem(
content::BrowserContext* context) OVERRIDE;
private:
friend struct base::DefaultLazyInstanceTraits<ChromeExtensionsBrowserClient>;
......
......@@ -52,8 +52,7 @@ ExtensionSystemSharedFactory::BuildServiceInstanceFor(
content::BrowserContext* ExtensionSystemSharedFactory::GetBrowserContextToUse(
content::BrowserContext* context) const {
// Redirected in incognito.
return extensions::ExtensionsBrowserClient::Get()->
GetOriginalContext(context);
return ExtensionsBrowserClient::Get()->GetOriginalContext(context);
}
// ExtensionSystemFactory
......@@ -72,24 +71,21 @@ ExtensionSystemFactory* ExtensionSystemFactory::GetInstance() {
ExtensionSystemFactory::ExtensionSystemFactory()
: BrowserContextKeyedServiceFactory(
"ExtensionSystem",
BrowserContextDependencyManager::GetInstance()),
custom_instance_(NULL) {
DependsOn(ExtensionSystemSharedFactory::GetInstance());
BrowserContextDependencyManager::GetInstance()) {
DCHECK(ExtensionsBrowserClient::Get())
<< "ExtensionSystemFactory must be initialized after BrowserProcess";
std::vector<BrowserContextKeyedServiceFactory*> dependencies =
ExtensionsBrowserClient::Get()->GetExtensionSystemDependencies();
for (size_t i = 0; i < dependencies.size(); ++i)
DependsOn(dependencies[i]);
}
ExtensionSystemFactory::~ExtensionSystemFactory() {
}
void ExtensionSystemFactory::SetCustomInstance(
ExtensionSystem* extension_system) {
custom_instance_ = extension_system;
}
BrowserContextKeyedService* ExtensionSystemFactory::BuildServiceInstanceFor(
content::BrowserContext* profile) const {
if (custom_instance_)
return custom_instance_;
return new ExtensionSystemImpl(static_cast<Profile*>(profile));
content::BrowserContext* context) const {
return ExtensionsBrowserClient::Get()->CreateExtensionSystem(context);
}
content::BrowserContext* ExtensionSystemFactory::GetBrowserContextToUse(
......
......@@ -44,10 +44,6 @@ class ExtensionSystemFactory : public BrowserContextKeyedServiceFactory {
static ExtensionSystemFactory* GetInstance();
// Provides a custom ExtensionSystem to use instead of building a new
// ExtensionSystemImpl. The BrowserContextKeyedService system takes ownership.
void SetCustomInstance(ExtensionSystem* extension_system);
private:
friend struct DefaultSingletonTraits<ExtensionSystemFactory>;
......@@ -59,8 +55,6 @@ class ExtensionSystemFactory : public BrowserContextKeyedServiceFactory {
virtual content::BrowserContext* GetBrowserContextToUse(
content::BrowserContext* context) const OVERRIDE;
virtual bool ServiceIsCreatedWithBrowserContext() const OVERRIDE;
ExtensionSystem* custom_instance_; // Not owned.
};
} // namespace extensions
......
......@@ -5,8 +5,11 @@
#ifndef EXTENSIONS_BROWSER_EXTENSIONS_BROWSER_CLIENT_H_
#define EXTENSIONS_BROWSER_EXTENSIONS_BROWSER_CLIENT_H_
#include <vector>
#include "base/memory/scoped_ptr.h"
class BrowserContextKeyedServiceFactory;
class CommandLine;
class PrefService;
......@@ -18,6 +21,7 @@ class JavaScriptDialogManager;
namespace extensions {
class AppSorting;
class ExtensionSystem;
// Interface to allow the extensions module to make browser-process-specific
// queries of the embedder. Should be Set() once in the browser process.
......@@ -87,6 +91,14 @@ class ExtensionsBrowserClient {
// does not support JavaScript dialogs.
virtual content::JavaScriptDialogManager* GetJavaScriptDialogManager() = 0;
// Returns the dependencies of ExtensionSystem. May return an empty list.
virtual std::vector<BrowserContextKeyedServiceFactory*>
GetExtensionSystemDependencies() = 0;
// Creates a new ExtensionSystem for |context|.
virtual ExtensionSystem* CreateExtensionSystem(
content::BrowserContext* context) = 0;
// Returns the single instance of |this|.
static ExtensionsBrowserClient* Get();
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment