From 03721598639ea7c7246c5b964495bc619fabe309 Mon Sep 17 00:00:00 2001 From: Istiaque Ahmed Date: Tue, 25 Jul 2017 21:15:28 +0000 Subject: [PATCH] [TaskScheduler] Update terminal_private_api to use TaskScheduler. FILE BrowserThread is deprecrated. Make terminal_private_api post tasks to TaskScheduler. Bug: 689520 Change-Id: Idd71fde7c5f3744816016d82e8d73535f0ceeb6c Reviewed-on: https://chromium-review.googlesource.com/578712 Commit-Queue: Istiaque Ahmed Reviewed-by: Gabriel Charette Reviewed-by: Toni Barzic Cr-Commit-Position: refs/heads/master@{#489433} --- .../api/terminal/terminal_private_api.cc | 66 ++++++++++--------- .../api/terminal/terminal_private_api.h | 12 ++-- chromeos/process_proxy/process_proxy.cc | 3 +- chromeos/process_proxy/process_proxy.h | 1 + .../process_proxy/process_proxy_registry.cc | 13 +++- .../process_proxy/process_proxy_registry.h | 4 ++ .../process_proxy/process_proxy_unittest.cc | 62 ++++++++++++----- 7 files changed, 104 insertions(+), 57 deletions(-) diff --git a/chrome/browser/extensions/api/terminal/terminal_private_api.cc b/chrome/browser/extensions/api/terminal/terminal_private_api.cc index c2207fe849f5a0..582571ef429f19 100644 --- a/chrome/browser/extensions/api/terminal/terminal_private_api.cc +++ b/chrome/browser/extensions/api/terminal/terminal_private_api.cc @@ -11,6 +11,7 @@ #include "base/json/json_writer.h" #include "base/memory/ptr_util.h" #include "base/sys_info.h" +#include "base/task_scheduler/post_task.h" #include "base/values.h" #include "chrome/browser/extensions/api/terminal/terminal_extension_helper.h" #include "chrome/browser/extensions/extension_service.h" @@ -140,11 +141,12 @@ TerminalPrivateOpenTerminalProcessFunction::Run() { if (tab_id < 0) return RespondNow(Error("Not called from a tab or app window")); - // Registry lives on FILE thread. - content::BrowserThread::PostTask( - content::BrowserThread::FILE, FROM_HERE, + // Registry lives on its own task runner. + chromeos::ProcessProxyRegistry::GetTaskRunner()->PostTask( + FROM_HERE, base::BindOnce( - &TerminalPrivateOpenTerminalProcessFunction::OpenOnFileThread, this, + &TerminalPrivateOpenTerminalProcessFunction::OpenOnRegistryTaskRunner, + this, base::Bind(&NotifyProcessOutput, browser_context(), extension_id(), tab_id), base::Bind( @@ -153,7 +155,7 @@ TerminalPrivateOpenTerminalProcessFunction::Run() { return RespondLater(); } -void TerminalPrivateOpenTerminalProcessFunction::OpenOnFileThread( +void TerminalPrivateOpenTerminalProcessFunction::OpenOnRegistryTaskRunner( const ProcessOutputCallback& output_callback, const OpenProcessCallback& callback) { DCHECK(!command_.empty()); @@ -182,15 +184,16 @@ ExtensionFunction::ResponseAction TerminalPrivateSendInputFunction::Run() { std::unique_ptr params(SendInput::Params::Create(*args_)); EXTENSION_FUNCTION_VALIDATE(params.get()); - // Registry lives on the FILE thread. - content::BrowserThread::PostTask( - content::BrowserThread::FILE, FROM_HERE, - base::BindOnce(&TerminalPrivateSendInputFunction::SendInputOnFileThread, - this, params->pid, params->input)); + // Registry lives on its own task runner. + chromeos::ProcessProxyRegistry::GetTaskRunner()->PostTask( + FROM_HERE, + base::BindOnce( + &TerminalPrivateSendInputFunction::SendInputOnRegistryTaskRunner, + this, params->pid, params->input)); return RespondLater(); } -void TerminalPrivateSendInputFunction::SendInputOnFileThread( +void TerminalPrivateSendInputFunction::SendInputOnRegistryTaskRunner( int terminal_id, const std::string& text) { bool success = @@ -215,17 +218,16 @@ TerminalPrivateCloseTerminalProcessFunction::Run() { CloseTerminalProcess::Params::Create(*args_)); EXTENSION_FUNCTION_VALIDATE(params.get()); - // Registry lives on the FILE thread. - content::BrowserThread::PostTask( - content::BrowserThread::FILE, FROM_HERE, - base::BindOnce( - &TerminalPrivateCloseTerminalProcessFunction::CloseOnFileThread, this, - params->pid)); + // Registry lives on its own task runner. + chromeos::ProcessProxyRegistry::GetTaskRunner()->PostTask( + FROM_HERE, base::BindOnce(&TerminalPrivateCloseTerminalProcessFunction:: + CloseOnRegistryTaskRunner, + this, params->pid)); return RespondLater(); } -void TerminalPrivateCloseTerminalProcessFunction::CloseOnFileThread( +void TerminalPrivateCloseTerminalProcessFunction::CloseOnRegistryTaskRunner( int terminal_id) { bool success = chromeos::ProcessProxyRegistry::Get()->CloseProcess(terminal_id); @@ -251,17 +253,17 @@ TerminalPrivateOnTerminalResizeFunction::Run() { OnTerminalResize::Params::Create(*args_)); EXTENSION_FUNCTION_VALIDATE(params.get()); - // Registry lives on the FILE thread. - content::BrowserThread::PostTask( - content::BrowserThread::FILE, FROM_HERE, - base::BindOnce( - &TerminalPrivateOnTerminalResizeFunction::OnResizeOnFileThread, this, - params->pid, params->width, params->height)); + // Registry lives on its own task runner. + chromeos::ProcessProxyRegistry::GetTaskRunner()->PostTask( + FROM_HERE, + base::BindOnce(&TerminalPrivateOnTerminalResizeFunction:: + OnResizeOnRegistryTaskRunner, + this, params->pid, params->width, params->height)); return RespondLater(); } -void TerminalPrivateOnTerminalResizeFunction::OnResizeOnFileThread( +void TerminalPrivateOnTerminalResizeFunction::OnResizeOnRegistryTaskRunner( int terminal_id, int width, int height) { @@ -296,16 +298,18 @@ ExtensionFunction::ResponseAction TerminalPrivateAckOutputFunction::Run() { if (tab_id != params->tab_id) return RespondNow(NoArguments()); - // Registry lives on the FILE thread. - content::BrowserThread::PostTask( - content::BrowserThread::FILE, FROM_HERE, - base::BindOnce(&TerminalPrivateAckOutputFunction::AckOutputOnFileThread, - this, params->pid)); + // Registry lives on its own task runner. + chromeos::ProcessProxyRegistry::GetTaskRunner()->PostTask( + FROM_HERE, + base::BindOnce( + &TerminalPrivateAckOutputFunction::AckOutputOnRegistryTaskRunner, + this, params->pid)); return RespondNow(NoArguments()); } -void TerminalPrivateAckOutputFunction::AckOutputOnFileThread(int terminal_id) { +void TerminalPrivateAckOutputFunction::AckOutputOnRegistryTaskRunner( + int terminal_id) { chromeos::ProcessProxyRegistry::Get()->AckOutput(terminal_id); } diff --git a/chrome/browser/extensions/api/terminal/terminal_private_api.h b/chrome/browser/extensions/api/terminal/terminal_private_api.h index 09982a748054b2..c645578405902a 100644 --- a/chrome/browser/extensions/api/terminal/terminal_private_api.h +++ b/chrome/browser/extensions/api/terminal/terminal_private_api.h @@ -32,8 +32,8 @@ class TerminalPrivateOpenTerminalProcessFunction const std::string& output)>; using OpenProcessCallback = base::Callback; - void OpenOnFileThread(const ProcessOutputCallback& output_callback, - const OpenProcessCallback& callback); + void OpenOnRegistryTaskRunner(const ProcessOutputCallback& output_callback, + const OpenProcessCallback& callback); void RespondOnUIThread(int terminal_id); std::string command_; @@ -52,7 +52,7 @@ class TerminalPrivateSendInputFunction : public UIThreadExtensionFunction { ExtensionFunction::ResponseAction Run() override; private: - void SendInputOnFileThread(int terminal_id, const std::string& input); + void SendInputOnRegistryTaskRunner(int terminal_id, const std::string& input); void RespondOnUIThread(bool success); }; @@ -69,7 +69,7 @@ class TerminalPrivateCloseTerminalProcessFunction ExtensionFunction::ResponseAction Run() override; private: - void CloseOnFileThread(int terminal_id); + void CloseOnRegistryTaskRunner(int terminal_id); void RespondOnUIThread(bool success); }; @@ -86,7 +86,7 @@ class TerminalPrivateOnTerminalResizeFunction ExtensionFunction::ResponseAction Run() override; private: - void OnResizeOnFileThread(int terminal_id, int width, int height); + void OnResizeOnRegistryTaskRunner(int terminal_id, int width, int height); void RespondOnUIThread(bool success); }; @@ -101,7 +101,7 @@ class TerminalPrivateAckOutputFunction : public UIThreadExtensionFunction { ExtensionFunction::ResponseAction Run() override; private: - void AckOutputOnFileThread(int terminal_id); + void AckOutputOnRegistryTaskRunner(int terminal_id); }; } // namespace extensions diff --git a/chromeos/process_proxy/process_proxy.cc b/chromeos/process_proxy/process_proxy.cc index 331ba5f1076cdb..03b7390e79ad12 100644 --- a/chromeos/process_proxy/process_proxy.cc +++ b/chromeos/process_proxy/process_proxy.cc @@ -65,6 +65,7 @@ int ProcessProxy::Open(const std::string& command) { bool ProcessProxy::StartWatchingOutput( const scoped_refptr& watcher_runner, + const scoped_refptr& callback_runner, const OutputCallback& callback) { DCHECK(process_launched_); CHECK(!output_watcher_.get()); @@ -78,7 +79,7 @@ bool ProcessProxy::StartWatchingOutput( callback_set_ = true; callback_ = callback; - callback_runner_ = base::ThreadTaskRunnerHandle::Get(); + callback_runner_ = callback_runner; watcher_runner_ = watcher_runner; // This object will delete itself once watching is stopped. diff --git a/chromeos/process_proxy/process_proxy.h b/chromeos/process_proxy/process_proxy.h index 19335bef349cad..8e843171d632d3 100644 --- a/chromeos/process_proxy/process_proxy.h +++ b/chromeos/process_proxy/process_proxy.h @@ -45,6 +45,7 @@ class ProcessProxy : public base::RefCountedThreadSafe { bool StartWatchingOutput( const scoped_refptr& watcher_runner, + const scoped_refptr& callback_runner, const OutputCallback& callback); // Sends some data to the process. diff --git a/chromeos/process_proxy/process_proxy_registry.cc b/chromeos/process_proxy/process_proxy_registry.cc index 7ba2e452fa710c..5b9a5b3f1d27ae 100644 --- a/chromeos/process_proxy/process_proxy_registry.cc +++ b/chromeos/process_proxy/process_proxy_registry.cc @@ -6,6 +6,8 @@ #include "base/bind.h" #include "base/message_loop/message_loop.h" +#include "base/sequenced_task_runner.h" +#include "base/task_scheduler/lazy_task_runner.h" namespace chromeos { @@ -68,9 +70,18 @@ void ProcessProxyRegistry::ShutDown() { // static ProcessProxyRegistry* ProcessProxyRegistry::Get() { + DCHECK(ProcessProxyRegistry::GetTaskRunner()->RunsTasksInCurrentSequence()); return g_process_proxy_registry.Pointer(); } +// static +scoped_refptr ProcessProxyRegistry::GetTaskRunner() { + static base::LazySequencedTaskRunner task_runner = + LAZY_SEQUENCED_TASK_RUNNER_INITIALIZER( + base::TaskTraits({base::MayBlock(), base::TaskPriority::BACKGROUND})); + return task_runner.Get(); +} + int ProcessProxyRegistry::OpenProcess(const std::string& command, const OutputCallback& output_callback) { DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); @@ -89,7 +100,7 @@ int ProcessProxyRegistry::OpenProcess(const std::string& command, // We can use Unretained because proxy will stop calling callback after it is // closed, which is done before this object goes away. if (!proxy->StartWatchingOutput( - watcher_thread_->task_runner(), + watcher_thread_->task_runner(), GetTaskRunner(), base::Bind(&ProcessProxyRegistry::OnProcessOutput, base::Unretained(this), terminal_id))) { proxy->Close(); diff --git a/chromeos/process_proxy/process_proxy_registry.h b/chromeos/process_proxy/process_proxy_registry.h index 16ca7b535a3bbb..5bb34dbfc922c3 100644 --- a/chromeos/process_proxy/process_proxy_registry.h +++ b/chromeos/process_proxy/process_proxy_registry.h @@ -41,6 +41,10 @@ class CHROMEOS_EXPORT ProcessProxyRegistry { static ProcessProxyRegistry* Get(); + // Returns a SequencedTaskRunner where the singleton instance of + // ProcessProxyRegistry lives. + static scoped_refptr GetTaskRunner(); + // Starts new ProcessProxy (which starts new process). // Returns ID used for the created process. Returns -1 on failure. int OpenProcess(const std::string& command, const OutputCallback& callback); diff --git a/chromeos/process_proxy/process_proxy_unittest.cc b/chromeos/process_proxy/process_proxy_unittest.cc index 45cae00ea40319..194d810aed1e57 100644 --- a/chromeos/process_proxy/process_proxy_unittest.cc +++ b/chromeos/process_proxy/process_proxy_unittest.cc @@ -8,13 +8,17 @@ #include #include +#include "base/at_exit.h" #include "base/bind.h" +#include "base/callback_forward.h" #include "base/location.h" #include "base/message_loop/message_loop.h" #include "base/process/kill.h" #include "base/process/process.h" #include "base/run_loop.h" #include "base/single_thread_task_runner.h" +#include "base/test/scoped_task_environment.h" +#include "base/threading/sequenced_task_runner_handle.h" #include "base/threading/thread.h" #include "base/threading/thread_task_runner_handle.h" #include "chromeos/process_proxy/process_proxy_registry.h" @@ -31,8 +35,15 @@ const char kCatCommand[] = "cat"; const char kStdoutType[] = "stdout"; const int kTestLineNum = 100; +void RunOnTaskRunner( + base::OnceClosure closure, + const scoped_refptr& task_runner) { + task_runner->PostTask(FROM_HERE, std::move(closure)); +} + class TestRunner { public: + TestRunner() {} virtual ~TestRunner() {} virtual void SetupExpectations(int terminal_id) = 0; virtual void OnSomeRead(int terminal_id, @@ -40,8 +51,14 @@ class TestRunner { const std::string& output) = 0; virtual void StartRegistryTest(ProcessProxyRegistry* registry) = 0; + void set_done_read_closure(base::OnceClosure done_closure) { + done_read_closure_ = std::move(done_closure); + } + protected: int terminal_id_; + + base::OnceClosure done_read_closure_; }; class RegistryTestRunner : public TestRunner { @@ -82,8 +99,8 @@ class RegistryTestRunner : public TestRunner { } if (!valid || TestSucceeded()) { - base::ThreadTaskRunnerHandle::Get()->PostTask( - FROM_HERE, base::MessageLoop::QuitWhenIdleClosure()); + ASSERT_FALSE(done_read_closure_.is_null()); + std::move(done_read_closure_).Run(); } } @@ -145,8 +162,8 @@ class RegistryNotifiedOnProcessExitTestRunner : public TestRunner { return; } EXPECT_EQ("exit", type); - base::ThreadTaskRunnerHandle::Get()->PostTask( - FROM_HERE, base::MessageLoop::QuitWhenIdleClosure()); + ASSERT_FALSE(done_read_closure_.is_null()); + std::move(done_read_closure_).Run(); } void StartRegistryTest(ProcessProxyRegistry* registry) override { @@ -165,7 +182,7 @@ class ProcessProxyTest : public testing::Test { ~ProcessProxyTest() override {} protected: - void InitRegistryTest() { + void InitRegistryTest(base::OnceClosure done_closure) { registry_ = ProcessProxyRegistry::Get(); terminal_id_ = registry_->OpenProcess( @@ -173,6 +190,7 @@ class ProcessProxyTest : public testing::Test { base::Bind(&ProcessProxyTest::HandleRead, base::Unretained(this))); EXPECT_GE(terminal_id_, 0); + test_runner_->set_done_read_closure(std::move(done_closure)); test_runner_->SetupExpectations(terminal_id_); test_runner_->StartRegistryTest(registry_); } @@ -184,7 +202,7 @@ class ProcessProxyTest : public testing::Test { registry_->AckOutput(terminal_id); } - void EndRegistryTest() { + void EndRegistryTest(base::OnceClosure done_closure) { registry_->CloseProcess(terminal_id_); base::TerminationStatus status = @@ -198,34 +216,42 @@ class ProcessProxyTest : public testing::Test { registry_->ShutDown(); - base::ThreadTaskRunnerHandle::Get()->PostTask( - FROM_HERE, base::MessageLoop::QuitWhenIdleClosure()); + std::move(done_closure).Run(); } void RunTest() { - base::ThreadTaskRunnerHandle::Get()->PostTask( - FROM_HERE, base::Bind(&ProcessProxyTest::InitRegistryTest, - base::Unretained(this))); - + base::RunLoop init_registry_waiter; + ProcessProxyRegistry::GetTaskRunner()->PostTask( + FROM_HERE, + base::BindOnce( + &ProcessProxyTest::InitRegistryTest, base::Unretained(this), + base::BindOnce(&RunOnTaskRunner, init_registry_waiter.QuitClosure(), + base::SequencedTaskRunnerHandle::Get()))); // Wait until all data from output watcher is received (QuitTask will be // fired on watcher thread). - base::RunLoop().Run(); + init_registry_waiter.Run(); - base::ThreadTaskRunnerHandle::Get()->PostTask( + base::RunLoop end_registry_waiter; + ProcessProxyRegistry::GetTaskRunner()->PostTask( FROM_HERE, - base::Bind(&ProcessProxyTest::EndRegistryTest, base::Unretained(this))); - + base::BindOnce( + &ProcessProxyTest::EndRegistryTest, base::Unretained(this), + base::BindOnce(&RunOnTaskRunner, end_registry_waiter.QuitClosure(), + base::SequencedTaskRunnerHandle::Get()))); // Wait until we clean up the process proxy. - base::RunLoop().Run(); + end_registry_waiter.Run(); } std::unique_ptr test_runner_; private: + // Destroys ProcessProxyRegistry LazyInstance after each test. + base::ShadowingAtExitManager shadowing_at_exit_manager_; + ProcessProxyRegistry* registry_; int terminal_id_; - base::MessageLoop message_loop_; + base::test::ScopedTaskEnvironment scoped_task_environment_; }; // Test will open new process that will run cat command, and verify data we