From 100a85f93ae11420ef3e7a6d3853d9c0503e3806 Mon Sep 17 00:00:00 2001 From: Jeremy Apthorp Date: Tue, 21 Jan 2020 09:43:18 -0800 Subject: [PATCH] fix: [extensions] load extensions on the IO thread (#21811) --- shell/browser/api/atom_api_session.cc | 23 +++++++++------ .../extensions/atom_extension_loader.cc | 29 ++++++++++++------- .../extensions/atom_extension_loader.h | 7 ++++- .../extensions/atom_extension_system.cc | 13 ++++----- .../extensions/atom_extension_system.h | 9 ++---- 5 files changed, 46 insertions(+), 35 deletions(-) diff --git a/shell/browser/api/atom_api_session.cc b/shell/browser/api/atom_api_session.cc index cff55b4e47f3..b1a1bc3e22b9 100644 --- a/shell/browser/api/atom_api_session.cc +++ b/shell/browser/api/atom_api_session.cc @@ -615,15 +615,20 @@ v8::Local Session::LoadExtension( auto* extension_system = static_cast( extensions::ExtensionSystem::Get(browser_context())); - // TODO(nornagon): make LoadExtension() asynchronous. - auto* extension = extension_system->LoadExtension(extension_path); - - if (extension) { - promise.Resolve(extension); - } else { - // TODO(nornagon): plumb through error message from extension loader. - promise.RejectWithErrorMessage("Failed to load extension"); - } + extension_system->LoadExtension( + extension_path, + base::BindOnce( + [](gin_helper::Promise promise, + const extensions::Extension* extension) { + if (extension) { + promise.Resolve(extension); + } else { + // TODO(nornagon): plumb through error message from extension + // loader. + promise.RejectWithErrorMessage("Failed to load extension"); + } + }, + std::move(promise))); return handle; } diff --git a/shell/browser/extensions/atom_extension_loader.cc b/shell/browser/extensions/atom_extension_loader.cc index f2021886df11..a195bca50a24 100644 --- a/shell/browser/extensions/atom_extension_loader.cc +++ b/shell/browser/extensions/atom_extension_loader.cc @@ -4,6 +4,8 @@ #include "shell/browser/extensions/atom_extension_loader.h" +#include + #include "base/auto_reset.h" #include "base/bind.h" #include "base/files/file_path.h" @@ -66,16 +68,14 @@ AtomExtensionLoader::AtomExtensionLoader( AtomExtensionLoader::~AtomExtensionLoader() = default; -const Extension* AtomExtensionLoader::LoadExtension( - const base::FilePath& extension_dir) { - // TODO(nornagon): load extensions asynchronously on - // GetExtensionFileTaskRunner() - base::ScopedAllowBlockingForTesting allow_blocking; - scoped_refptr extension = LoadUnpacked(extension_dir); - if (extension) - extension_registrar_.AddExtension(extension); - - return extension.get(); +void AtomExtensionLoader::LoadExtension( + const base::FilePath& extension_dir, + base::OnceCallback loaded) { + base::PostTaskAndReplyWithResult( + GetExtensionFileTaskRunner().get(), FROM_HERE, + base::BindOnce(&LoadUnpacked, extension_dir), + base::BindOnce(&AtomExtensionLoader::FinishExtensionLoad, + weak_factory_.GetWeakPtr(), std::move(loaded))); } void AtomExtensionLoader::ReloadExtension(const ExtensionId& extension_id) { @@ -100,6 +100,15 @@ void AtomExtensionLoader::UnloadExtension( extension_registrar_.RemoveExtension(extension_id, reason); } +void AtomExtensionLoader::FinishExtensionLoad( + base::OnceCallback done, + scoped_refptr extension) { + if (extension) { + extension_registrar_.AddExtension(extension); + } + std::move(done).Run(extension.get()); +} + void AtomExtensionLoader::FinishExtensionReload( const ExtensionId& old_extension_id, scoped_refptr extension) { diff --git a/shell/browser/extensions/atom_extension_loader.h b/shell/browser/extensions/atom_extension_loader.h index 545b7c285e5a..2cca8e109492 100644 --- a/shell/browser/extensions/atom_extension_loader.h +++ b/shell/browser/extensions/atom_extension_loader.h @@ -34,7 +34,9 @@ class AtomExtensionLoader : public ExtensionRegistrar::Delegate { // Loads an unpacked extension from a directory synchronously. Returns the // extension on success, or nullptr otherwise. - const Extension* LoadExtension(const base::FilePath& extension_dir); + void LoadExtension( + const base::FilePath& extension_dir, + base::OnceCallback loaded); // Starts reloading the extension. A keep-alive is maintained until the // reload succeeds/fails. If the extension is an app, it will be launched upon @@ -52,6 +54,9 @@ class AtomExtensionLoader : public ExtensionRegistrar::Delegate { void FinishExtensionReload(const ExtensionId& old_extension_id, scoped_refptr extension); + void FinishExtensionLoad(base::OnceCallback loaded, + scoped_refptr extension); + // ExtensionRegistrar::Delegate: void PreAddExtension(const Extension* extension, const Extension* old_extension) override; diff --git a/shell/browser/extensions/atom_extension_system.cc b/shell/browser/extensions/atom_extension_system.cc index 7c315abdbd33..7136cbf63feb 100644 --- a/shell/browser/extensions/atom_extension_system.cc +++ b/shell/browser/extensions/atom_extension_system.cc @@ -6,6 +6,7 @@ #include #include +#include #include "base/bind.h" #include "base/files/file_path.h" @@ -43,14 +44,10 @@ AtomExtensionSystem::AtomExtensionSystem(BrowserContext* browser_context) AtomExtensionSystem::~AtomExtensionSystem() = default; -const Extension* AtomExtensionSystem::LoadExtension( - const base::FilePath& extension_dir) { - return extension_loader_->LoadExtension(extension_dir); -} - -const Extension* AtomExtensionSystem::LoadApp(const base::FilePath& app_dir) { - NOTIMPLEMENTED() << "Attempted to load platform app in Electron"; - return nullptr; +void AtomExtensionSystem::LoadExtension( + const base::FilePath& extension_dir, + base::OnceCallback loaded) { + extension_loader_->LoadExtension(extension_dir, std::move(loaded)); } void AtomExtensionSystem::FinishInitialization() { diff --git a/shell/browser/extensions/atom_extension_system.h b/shell/browser/extensions/atom_extension_system.h index 06a6d954e053..5c3a0fd7e925 100644 --- a/shell/browser/extensions/atom_extension_system.h +++ b/shell/browser/extensions/atom_extension_system.h @@ -39,13 +39,8 @@ class AtomExtensionSystem : public ExtensionSystem { // Loads an unpacked extension from a directory. Returns the extension on // success, or nullptr otherwise. - const Extension* LoadExtension(const base::FilePath& extension_dir); - - // Loads an unpacked platform app from a directory. Returns the extension on - // success, or nullptr otherwise. - // Currently this just calls LoadExtension, as apps are not loaded differently - // than other extensions. Use LaunchApp() to actually launch the loaded app. - const Extension* LoadApp(const base::FilePath& app_dir); + void LoadExtension(const base::FilePath& extension_dir, + base::OnceCallback loaded); // Finish initialization for the shell extension system. void FinishInitialization();