Commit 40b4cbc3 authored by Devlin Cronin's avatar Devlin Cronin Committed by Commit Bot

[Task Migration][Extensions] Add and use a devoted file task runner

With the new task scheduling system, there are no guarantees that
different operations will not collide - each caller has to ensure that
no conflicting tasks will be scheduled and executed synchronously. This
means that for file tasks, we need to have a common task runner for any
tasks that may touch the same files.

Introduce a common SequencedTaskRunner for the extension system, and use
it in the UserScriptLoader.  Additionally, use it in ExtensionService,
in lieu of the prior ExtensionService-owned task runner that's used for
file tasks in installation.  Finally, also make the GetFileTaskRunner()
method in ExtensionService non-virtual, since it's not needed on any of
the interfaces.

Bug: 689520
Bug: 750122
Change-Id: I9d0b993689821e7aca9b2553283609cce796a162
Reviewed-on: https://chromium-review.googlesource.com/593854Reviewed-by: default avatarIstiaque Ahmed <lazyboy@chromium.org>
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#491209}
parent 05c7a3f2
......@@ -74,6 +74,7 @@
#include "content/public/browser/storage_partition.h"
#include "extensions/browser/app_sorting.h"
#include "extensions/browser/event_router.h"
#include "extensions/browser/extension_file_task_runner.h"
#include "extensions/browser/extension_host.h"
#include "extensions/browser/extension_registry.h"
#include "extensions/browser/extension_system.h"
......@@ -329,13 +330,6 @@ ExtensionService::ExtensionService(Profile* profile,
install_directory_(install_directory),
extensions_enabled_(extensions_enabled),
ready_(ready),
// We should be able to interrupt any part of extension install process
// during shutdown. SKIP_ON_SHUTDOWN ensures that not extension install
// task will be stopped while it is running but that tasks that have not
// started running will be skipped on shutdown.
file_task_runner_(base::CreateSequencedTaskRunnerWithTraits(
{base::MayBlock(), base::TaskPriority::BACKGROUND,
base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN})),
shared_module_service_(new extensions::SharedModuleService(profile_)),
renderer_helper_(
extensions::RendererStartupHelperFactory::GetForBrowserContext(
......@@ -1208,10 +1202,6 @@ bool ExtensionService::is_ready() {
return ready_->is_signaled();
}
base::SequencedTaskRunner* ExtensionService::GetFileTaskRunner() {
return file_task_runner_.get();
}
void ExtensionService::CheckManagementPolicy() {
std::vector<std::string> to_unload;
std::map<std::string, Extension::DisableReason> to_disable;
......@@ -2518,6 +2508,12 @@ void ExtensionService::UnregisterInstallGate(
}
}
base::SequencedTaskRunner* ExtensionService::GetFileTaskRunner() {
// TODO(devlin): Update callers to use GetExtensionFileTaskRunner()
// directly.
return extensions::GetExtensionFileTaskRunner().get();
}
// Used only by test code.
void ExtensionService::UnloadAllExtensionsInternal() {
profile_->GetExtensionSpecialStoragePolicy()->RevokeRightsForAllExtensions();
......
......@@ -170,8 +170,6 @@ class ExtensionServiceInterface
// Whether the extension service is ready.
virtual bool is_ready() = 0;
// Returns task runner for crx installation file I/O operations.
virtual base::SequencedTaskRunner* GetFileTaskRunner() = 0;
};
// Manages installed and running Chromium extensions. An instance is shared
......@@ -227,7 +225,6 @@ class ExtensionService
void CheckManagementPolicy() override;
void CheckForUpdatesSoon() override;
bool is_ready() override;
base::SequencedTaskRunner* GetFileTaskRunner() override;
// ExternalProvider::VisitorInterface implementation.
// Exposed for testing.
......@@ -367,6 +364,9 @@ class ExtensionService
extensions::InstallGate* install_delayer);
void UnregisterInstallGate(extensions::InstallGate* install_delayer);
// Returns task runner for crx installation file I/O operations.
base::SequencedTaskRunner* GetFileTaskRunner();
//////////////////////////////////////////////////////////////////////////////
// Simple Accessors
......@@ -716,9 +716,6 @@ class ExtensionService
// responsible for prompting the user about suspicious extensions.
std::unique_ptr<extensions::ExternalInstallManager> external_install_manager_;
// Sequenced task runner for extension related file operations.
const scoped_refptr<base::SequencedTaskRunner> file_task_runner_;
std::unique_ptr<extensions::ExtensionActionStorageManager>
extension_action_storage_manager_;
......
......@@ -15,7 +15,6 @@
#include "base/location.h"
#include "base/macros.h"
#include "base/memory/ptr_util.h"
#include "base/run_loop.h"
#include "base/strings/string_util.h"
#include "chrome/browser/chrome_notification_types.h"
#include "chrome/test/base/testing_profile.h"
......@@ -23,6 +22,7 @@
#include "content/public/browser/notification_registrar.h"
#include "content/public/browser/notification_service.h"
#include "content/public/test/test_browser_thread_bundle.h"
#include "content/public/test/test_utils.h"
#include "extensions/common/host_id.h"
#include "testing/gtest/include/gtest/gtest.h"
......@@ -83,7 +83,7 @@ TEST_F(ExtensionUserScriptLoaderTest, NoScripts) {
HostID(),
true /* listen_for_extension_system_loaded */);
loader.StartLoad();
base::RunLoop().RunUntilIdle();
content::RunAllBlockingPoolTasksUntilIdle();
ASSERT_TRUE(shared_memory_ != NULL);
}
......
......@@ -66,11 +66,6 @@ bool TestExtensionService::is_ready() {
return false;
}
base::SequencedTaskRunner* TestExtensionService::GetFileTaskRunner() {
ADD_FAILURE();
return NULL;
}
void TestExtensionService::AddExtension(const Extension* extension) {
ADD_FAILURE();
}
......
......@@ -43,8 +43,6 @@ class TestExtensionService : public ExtensionServiceInterface {
bool is_ready() override;
base::SequencedTaskRunner* GetFileTaskRunner() override;
void AddExtension(const extensions::Extension* extension) override;
void AddComponentExtension(const extensions::Extension* extension) override;
......
......@@ -112,6 +112,8 @@ source_set("browser_sources") {
"extension_dialog_auto_confirm.h",
"extension_error.cc",
"extension_error.h",
"extension_file_task_runner.cc",
"extension_file_task_runner.h",
"extension_function.cc",
"extension_function.h",
"extension_function_dispatcher.cc",
......
// Copyright 2017 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.
#include "extensions/browser/extension_file_task_runner.h"
#include "base/sequenced_task_runner.h"
#include "base/task_scheduler/lazy_task_runner.h"
#include "base/task_scheduler/task_traits.h"
namespace extensions {
namespace {
// Note: All tasks posted to a single task runner have the same priority. This
// is unfortunate, since some file-related tasks are high priority (like serving
// a file from the extension protocols or loading an extension in response to a
// user action), and others are low priority (like garbage collection). Split
// the difference and use USER_VISIBLE, which is the default priority and what a
// task posted to a named thread (like the FILE thread) would receive.
base::LazySequencedTaskRunner g_task_runner =
LAZY_SEQUENCED_TASK_RUNNER_INITIALIZER(
base::TaskTraits(base::MayBlock(),
base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN,
base::TaskPriority::USER_VISIBLE));
} // namespace
scoped_refptr<base::SequencedTaskRunner> GetExtensionFileTaskRunner() {
return g_task_runner.Get();
}
} // namespace extensions
// Copyright 2017 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_EXTENSION_FILE_TASK_RUNNER_H_
#define EXTENSIONS_BROWSER_EXTENSION_FILE_TASK_RUNNER_H_
#include "base/memory/ref_counted.h"
namespace base {
class SequencedTaskRunner;
}
namespace extensions {
// Returns the singleton instance of the task runner to be used for
// extension-related tasks that read, modify, or delete files. All these tasks
// must be posted to this task runner, even if it is only reading the file,
// since other tasks may be modifying it.
// TODO(devlin): We need to pull in all extension-related file tasks to use this
// task runner.
scoped_refptr<base::SequencedTaskRunner> GetExtensionFileTaskRunner();
} // namespace extensions
#endif // EXTENSIONS_BROWSER_EXTENSION_FILE_TASK_RUNNER_H_
......@@ -24,6 +24,7 @@
#include "content/public/browser/render_process_host.h"
#include "extensions/browser/component_extension_resource_manager.h"
#include "extensions/browser/content_verifier.h"
#include "extensions/browser/extension_file_task_runner.h"
#include "extensions/browser/extension_registry.h"
#include "extensions/browser/extension_system.h"
#include "extensions/browser/extensions_browser_client.h"
......@@ -154,12 +155,13 @@ void LoadUserScripts(UserScriptList* user_scripts,
}
}
void LoadScriptsOnFileThread(
void LoadScriptsOnFileTaskRunner(
std::unique_ptr<UserScriptList> user_scripts,
const ExtensionUserScriptLoader::HostsInfo& hosts_info,
const std::set<int>& added_script_ids,
const scoped_refptr<ContentVerifier>& verifier,
UserScriptLoader::LoadScriptsCallback callback) {
DCHECK(GetExtensionFileTaskRunner()->RunsTasksInCurrentSequence());
DCHECK(user_scripts.get());
LoadUserScripts(user_scripts.get(), hosts_info, added_script_ids, verifier);
std::unique_ptr<base::SharedMemory> memory =
......@@ -214,9 +216,9 @@ void ExtensionUserScriptLoader::LoadScripts(
LoadScriptsCallback callback) {
UpdateHostsInfo(changed_hosts);
content::BrowserThread::PostTask(
content::BrowserThread::FILE, FROM_HERE,
base::BindOnce(&LoadScriptsOnFileThread, std::move(user_scripts),
GetExtensionFileTaskRunner()->PostTask(
FROM_HERE,
base::BindOnce(&LoadScriptsOnFileTaskRunner, std::move(user_scripts),
hosts_info_, added_script_ids, content_verifier_,
std::move(callback)));
}
......
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