Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(102)

Unified Diff: chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.cc

Issue 2890023005: Chrome Cleaner UI: reporter no longer uses mojo. (Closed)
Patch Set: Move main run function to inside the ChromeCleanerRunner class. Created 3 years, 7 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
Index: chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.cc
diff --git a/chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.cc b/chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.cc
new file mode 100644
index 0000000000000000000000000000000000000000..d2dada1e173da9b48e19610992e5566a55cf67b8
--- /dev/null
+++ b/chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.cc
@@ -0,0 +1,240 @@
+// 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 "chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.h"
+
+#include <utility>
+
+#include "base/base_paths.h"
+#include "base/bind.h"
+#include "base/bind_helpers.h"
+#include "base/feature_list.h"
+#include "base/location.h"
+#include "base/memory/ptr_util.h"
+#include "base/path_service.h"
+#include "base/process/launch.h"
+#include "base/process/process.h"
+#include "base/strings/string_number_conversions.h"
+#include "base/task_scheduler/post_task.h"
+#include "chrome/browser/safe_browsing/chrome_cleaner/srt_client_info_win.h"
+#include "chrome/browser/safe_browsing/chrome_cleaner/srt_field_trial_win.h"
+#include "chrome/installer/util/install_util.h"
+#include "components/chrome_cleaner/public/constants/constants.h"
+#include "components/chrome_cleaner/public/interfaces/chrome_prompt.mojom.h"
+#include "components/version_info/version_info.h"
+#include "content/public/browser/browser_thread.h"
+#include "mojo/edk/embedder/connection_params.h"
+#include "mojo/edk/embedder/embedder.h"
+#include "mojo/edk/embedder/outgoing_broker_client_invitation.h"
+#include "mojo/edk/embedder/platform_channel_pair.h"
+#include "mojo/edk/embedder/transport_protocol.h"
+#include "mojo/public/cpp/system/message_pipe.h"
+
+using chrome_cleaner::mojom::ChromePrompt;
+using chrome_cleaner::mojom::ChromePromptRequest;
+using content::BrowserThread;
+
+namespace safe_browsing {
+
+// static
+void ChromeCleanerRunner::RunChromeCleanerAndReplyWithExitCode(
+ const base::FilePath& executable_path,
+ const SwReporterInvocation& reporter_invocation,
+ bool metrics_enabled,
+ bool cleaner_logs_enabled,
+ ChromePromptImpl::OnPromptUser on_prompt_user,
+ base::OnceClosure on_connection_closed,
+ base::OnceClosure on_connection_error,
+ ChromeCleanerRunner::ProcessDoneCallback on_process_done_) {
Joe Mason 2017/05/19 22:02:25 on_process_done_ shouldn't have a trailing _
alito 2017/05/22 23:26:30 Done.
+ DCHECK_CURRENTLY_ON(BrowserThread::UI);
Joe Mason 2017/05/19 22:02:25 I don't think this is necessary. We don't really c
alito 2017/05/22 23:26:30 Done.
+
+ auto cleaner_runner = make_scoped_refptr(new ChromeCleanerRunner(
csharp 2017/05/19 20:44:17 I wonder if it'd be cleaner to move the creation o
alito 2017/05/22 23:26:31 One benefit of having the static function create t
+ executable_path, reporter_invocation, metrics_enabled,
+ cleaner_logs_enabled, std::move(on_prompt_user),
+ std::move(on_connection_closed), std::move(on_connection_error),
+ std::move(on_process_done_)));
+ auto launch_and_wait = base::BindOnce(
+ &ChromeCleanerRunner::LaunchAndWaitForExitOnBackgroundThread,
+ cleaner_runner);
+ auto process_done =
+ base::BindOnce(&ChromeCleanerRunner::OnProcessDone, cleaner_runner);
+ base::PostTaskWithTraitsAndReplyWithResult(
+ FROM_HERE,
+ // LaunchAndWaitForExitOnBackgroundThread creates (MayBlock()) and joins
+ // (WithBaseSyncPrimitives()) a process.
+ {base::MayBlock(), base::WithBaseSyncPrimitives(),
+ base::TaskPriority::BACKGROUND,
+ base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN},
+ std::move(launch_and_wait), std::move(process_done));
+}
+
+ChromeCleanerRunner::ChromeCleanerRunner(
+ const base::FilePath& executable_path,
+ const SwReporterInvocation& reporter_invocation,
+ bool metrics_enabled,
+ bool cleaner_logs_enabled,
+ ChromePromptImpl::OnPromptUser on_prompt_user,
+ base::OnceClosure on_connection_closed,
+ base::OnceClosure on_connection_error,
+ ProcessDoneCallback on_process_done)
+ : command_line_(executable_path),
+ on_prompt_user_(std::move(on_prompt_user)),
+ on_connection_closed_(std::move(on_connection_closed)),
+ on_connection_error_(std::move(on_connection_error)),
+ on_process_done_(std::move(on_process_done)) {
+ DCHECK(base::FeatureList::IsEnabled(kInBrowserCleanerUIFeature));
+ DCHECK_CURRENTLY_ON(BrowserThread::UI);
Joe Mason 2017/05/19 22:02:25 base::SequenceChecker can replace this. You'd als
alito 2017/05/22 23:26:30 Now using ThreadTaskRunnerHandle::Get() to get a S
+ DCHECK(on_prompt_user_);
+ DCHECK(on_connection_closed_);
+ DCHECK(on_connection_error_);
+ DCHECK(!executable_path.empty());
+
+ // Add the non-IPC switches that should be passed to the Cleaner process.
+
+ // Add switches that pass information about this Chrome installation.
+ command_line_.AppendSwitchASCII(chrome_cleaner::kChromeVersionSwitch,
+ version_info::GetVersionNumber());
+ command_line_.AppendSwitchASCII(chrome_cleaner::kChromeChannelSwitch,
+ base::IntToString(ChannelAsInt()));
+ base::FilePath chrome_exe_path;
+ PathService::Get(base::FILE_EXE, &chrome_exe_path);
+ command_line_.AppendSwitchPath(chrome_cleaner::kChromeExePathSwitch,
+ chrome_exe_path);
+ if (!InstallUtil::IsPerUserInstall())
+ command_line_.AppendSwitch(chrome_cleaner::kChromeSystemInstallSwitch);
+
+ // Start the cleaner process in scanning mode.
+ command_line_.AppendSwitchASCII(
+ chrome_cleaner::kExecutionModeSwitch,
+ base::IntToString(
+ static_cast<int>(chrome_cleaner::ExecutionMode::kScanning)));
+
+ // If set, forward the engine flag from the reporter. Otherwise, set the
+ // engine flag explicitly to 1.
+ command_line_.AppendSwitchASCII(
+ chrome_cleaner::kEngineSwitch,
+ reporter_invocation.command_line.HasSwitch(chrome_cleaner::kEngineSwitch)
+ ? reporter_invocation.command_line.GetSwitchValueASCII(
+ chrome_cleaner::kEngineSwitch)
+ : base::IntToString(1));
+
+ // If metrics is enabled, we can enable crash reporting in the Chrome Cleaner
+ // process.
+ if (metrics_enabled) {
+ command_line_.AppendSwitch(chrome_cleaner::kUmaUserSwitch);
+ command_line_.AppendSwitch(chrome_cleaner::kEnableCrashReportingSwitch);
+ }
+
+ // Enable logs upload for users who have opted into safe browsing extended
+ // reporting v2.
+ if (cleaner_logs_enabled)
+ command_line_.AppendSwitch(chrome_cleaner::kEnableCleanerLoggingSwitch);
+}
+
+int ChromeCleanerRunner::LaunchAndWaitForExitOnBackgroundThread() {
+ mojo::edk::OutgoingBrokerClientInvitation invitation;
+ std::string mojo_pipe_token = mojo::edk::GenerateRandomToken();
+ mojo::ScopedMessagePipeHandle mojo_pipe =
+ invitation.AttachMessagePipe(mojo_pipe_token);
+ command_line_.AppendSwitchASCII(chrome_cleaner::kChromeMojoPipeTokenSwitch,
+ mojo_pipe_token);
+
+ mojo::edk::PlatformChannelPair channel;
+ base::HandlesToInheritVector handles_to_inherit;
+ channel.PrepareToPassClientHandleToChildProcess(&command_line_,
+ &handles_to_inherit);
+ base::LaunchOptions launch_options;
+ launch_options.handles_to_inherit = &handles_to_inherit;
+
+ base::Process cleaner_process =
+ base::LaunchProcess(command_line_, launch_options);
+
+ if (!cleaner_process.IsValid())
+ return kNotLaunchedExitCode;
csharp 2017/05/19 20:44:17 I wonder if it might be cleaner to have bool in a
alito 2017/05/22 23:26:30 Done.
+
+ // ChromePromptImpl tasks will need to run on the IO thread. There is no
+ // need to synchronize its creation, since the client end will wait for this
+ // initialization to be done before sending requests.
+ BrowserThread::GetTaskRunnerForThread(BrowserThread::IO)
+ ->PostTask(FROM_HERE,
+ base::BindOnce(&ChromeCleanerRunner::CreateChromePromptImpl,
+ base::RetainedRef(this),
+ chrome_cleaner::mojom::ChromePromptRequest(
+ std::move(mojo_pipe))));
+
+ invitation.Send(
+ cleaner_process.Handle(),
+ mojo::edk::ConnectionParams(mojo::edk::TransportProtocol::kLegacy,
+ channel.PassServerHandle()),
+ base::Bind(&ChromeCleanerRunner::OnConnectionError,
+ base::RetainedRef(this)));
+
+ int exit_code = kNotLaunchedExitCode;
+ bool success = cleaner_process.WaitForExit(&exit_code);
+ DCHECK(success);
+ BrowserThread::GetTaskRunnerForThread(BrowserThread::IO)
+ ->PostTask(FROM_HERE,
+ base::Bind(&ChromeCleanerRunner::ReleaseChromePromptImpl,
+ base::RetainedRef(this)));
+ return exit_code;
+}
+
+ChromeCleanerRunner::~ChromeCleanerRunner() {
+ DCHECK_CURRENTLY_ON(BrowserThread::UI);
Joe Mason 2017/05/19 22:02:25 Can you also DCHECK that chrome_prompt_impl_ is nu
alito 2017/05/22 23:26:31 Done.
+}
+
+void ChromeCleanerRunner::CreateChromePromptImpl(
+ ChromePromptRequest chrome_prompt_request) {
+ DCHECK_CURRENTLY_ON(BrowserThread::IO);
+ DCHECK(!chrome_prompt_impl_);
+
+ chrome_prompt_impl_ = base::MakeUnique<ChromePromptImpl>(
+ std::move(chrome_prompt_request),
+ base::Bind(&ChromeCleanerRunner::OnConnectionClosed,
+ base::RetainedRef(this)),
+ base::Bind(&ChromeCleanerRunner::OnPromptUser, base::RetainedRef(this)));
+}
+
+void ChromeCleanerRunner::ReleaseChromePromptImpl() {
+ DCHECK_CURRENTLY_ON(BrowserThread::IO);
+ DCHECK(chrome_prompt_impl_);
+
+ chrome_prompt_impl_.release();
+}
+
+void ChromeCleanerRunner::OnPromptUser(
+ std::unique_ptr<std::set<base::FilePath>> files_to_delete,
+ ChromePrompt::PromptUserCallback prompt_user_callback) {
+ if (on_prompt_user_) {
+ BrowserThread::GetTaskRunnerForThread(BrowserThread::UI)
+ ->PostTask(FROM_HERE,
+ base::BindOnce(std::move(on_prompt_user_),
+ base::Passed(&files_to_delete),
+ base::Passed(&prompt_user_callback)));
+ }
+}
+
+void ChromeCleanerRunner::OnConnectionClosed() {
+ if (on_connection_closed_) {
+ BrowserThread::GetTaskRunnerForThread(BrowserThread::UI)
+ ->PostTask(FROM_HERE, std::move(on_connection_closed_));
+ }
+}
+
+void ChromeCleanerRunner::OnConnectionError(const std::string& error) {
+ if (on_connection_error_) {
+ BrowserThread::GetTaskRunnerForThread(BrowserThread::UI)
+ ->PostTask(FROM_HERE, std::move(on_connection_error_));
+ }
+}
+
+void ChromeCleanerRunner::OnProcessDone(int exit_code) {
+ DCHECK_CURRENTLY_ON(BrowserThread::UI);
+
+ if (!on_process_done_)
+ return;
+ std::move(on_process_done_).Run(exit_code);
+}
+
+} // namespace safe_browsing

Powered by Google App Engine
This is Rietveld 408576698