Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: [extensions] load extensions on the IO thread #21811

Merged
merged 2 commits into from
Jan 21, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
23 changes: 14 additions & 9 deletions shell/browser/api/atom_api_session.cc
Original file line number Diff line number Diff line change
Expand Up @@ -615,15 +615,20 @@ v8::Local<v8::Promise> Session::LoadExtension(

auto* extension_system = static_cast<extensions::AtomExtensionSystem*>(
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<const extensions::Extension*> 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;
}
Expand Down
29 changes: 19 additions & 10 deletions shell/browser/extensions/atom_extension_loader.cc
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@

#include "shell/browser/extensions/atom_extension_loader.h"

#include <utility>

#include "base/auto_reset.h"
#include "base/bind.h"
#include "base/files/file_path.h"
Expand Down Expand Up @@ -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<const Extension> extension = LoadUnpacked(extension_dir);
if (extension)
extension_registrar_.AddExtension(extension);

return extension.get();
void AtomExtensionLoader::LoadExtension(
const base::FilePath& extension_dir,
base::OnceCallback<void(const Extension*)> 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) {
Expand All @@ -100,6 +100,15 @@ void AtomExtensionLoader::UnloadExtension(
extension_registrar_.RemoveExtension(extension_id, reason);
}

void AtomExtensionLoader::FinishExtensionLoad(
base::OnceCallback<void(const Extension*)> done,
scoped_refptr<const Extension> extension) {
if (extension) {
extension_registrar_.AddExtension(extension);
}
std::move(done).Run(extension.get());
}

void AtomExtensionLoader::FinishExtensionReload(
const ExtensionId& old_extension_id,
scoped_refptr<const Extension> extension) {
Expand Down
7 changes: 6 additions & 1 deletion shell/browser/extensions/atom_extension_loader.h
Original file line number Diff line number Diff line change
Expand Up @@ -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<void(const Extension* extension)> 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
Expand All @@ -52,6 +54,9 @@ class AtomExtensionLoader : public ExtensionRegistrar::Delegate {
void FinishExtensionReload(const ExtensionId& old_extension_id,
scoped_refptr<const Extension> extension);

void FinishExtensionLoad(base::OnceCallback<void(const Extension*)> loaded,
scoped_refptr<const Extension> extension);

// ExtensionRegistrar::Delegate:
void PreAddExtension(const Extension* extension,
const Extension* old_extension) override;
Expand Down
13 changes: 5 additions & 8 deletions shell/browser/extensions/atom_extension_system.cc
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

#include <memory>
#include <string>
#include <utility>

#include "base/bind.h"
#include "base/files/file_path.h"
Expand Down Expand Up @@ -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<void(const Extension*)> loaded) {
extension_loader_->LoadExtension(extension_dir, std::move(loaded));
}

void AtomExtensionSystem::FinishInitialization() {
Expand Down
9 changes: 2 additions & 7 deletions shell/browser/extensions/atom_extension_system.h
Original file line number Diff line number Diff line change
Expand Up @@ -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<void(const Extension*)> loaded);

// Finish initialization for the shell extension system.
void FinishInitialization();
Expand Down