Chrome Cleaner UI: reporter no longer uses mojo.

chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.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 "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/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 {
+
+namespace {
+
+// Global delegate used to override the launching of the Cleaner process during
+// tests.
+ChromeCleanerRunnerTestDelegate* g_test_delegate = nullptr;
+
+}  // namespace
+
+// static
+void ChromeCleanerRunner::RunChromeCleanerAndReplyWithExitCode(
+    const base::FilePath& executable_path,
+    const SwReporterInvocation& reporter_invocation,
+    ChromeMetricsStatus metrics_status,
+    CleanerLogsStatus cleaner_logs_status,
+    ChromePromptImpl::OnPromptUser on_prompt_user,
+    base::OnceClosure on_connection_closed,
+    ChromeCleanerRunner::ProcessDoneCallback on_process_done,
+    scoped_refptr<base::SequencedTaskRunner> task_runner) {
+  auto cleaner_runner = make_scoped_refptr(new ChromeCleanerRunner(
+      executable_path, reporter_invocation, metrics_status, cleaner_logs_status,
+      std::move(on_prompt_user), std::move(on_connection_closed),
+      std::move(on_process_done), task_runner));

[ftirelo, 2017/05/24 01:45:20]

Maybe std::move(task_runner) as well?

[alito, 2017/05/24 03:20:23]

Done.

+  auto launch_and_wait = base::BindOnce(
+      &ChromeCleanerRunner::LaunchAndWaitForExitOnBackgroundThread,
+      cleaner_runner);

[ftirelo, 2017/05/24 01:45:20]

Should we use base::RetainedRef(cleaner_runner) here?

[alito, 2017/05/24 03:20:23]

Actually, I think the way it is written here is the default intended use of
Bind. See the docs here:
https://chromium.googlesource.com/chromium/src/+/master/docs/callback.md#Bind....

+  auto process_done =
+      base::BindOnce(&ChromeCleanerRunner::OnProcessDone, cleaner_runner);

[ftirelo, 2017/05/24 01:45:20]

Should we use base::RetainedRef(cleaner_runner) here?

[alito, 2017/05/24 03:20:23]

See previous comment.

+  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,
+    ChromeMetricsStatus metrics_status,
+    CleanerLogsStatus cleaner_logs_status,
+    ChromePromptImpl::OnPromptUser on_prompt_user,
+    base::OnceClosure on_connection_closed,
+    ProcessDoneCallback on_process_done,
+    scoped_refptr<base::SequencedTaskRunner> task_runner)
+    : task_runner_(task_runner),

[ftirelo, 2017/05/24 01:45:20]

Ditto.

[alito, 2017/05/24 03:20:23]

Done.

+      command_line_(executable_path),
+      on_prompt_user_(std::move(on_prompt_user)),
+      on_connection_closed_(std::move(on_connection_closed)),
+      on_process_done_(std::move(on_process_done)) {
+  DCHECK(base::FeatureList::IsEnabled(kInBrowserCleanerUIFeature));
+  DCHECK(on_prompt_user_);
+  DCHECK(on_connection_closed_);
+  DCHECK(on_process_done_);
+  DCHECK(!executable_path.empty());
+
+  if (g_test_delegate)
+    command_line_ = g_test_delegate->GetTestBaseCommandLine();
+
+  // 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));

[ftirelo, 2017/05/24 01:45:20]

I think it'd be easier to read this if you saved the switch value to an
auxiliary variable and check if it's empty. Something like this:

const std::string& engine =
reporter_invocation.command_line.GetSwitchValueASCII(
    chrome_cleaner::kEngineSwitch);
command_line_.AppendSwitchASCII(chrome_cleaner::kEngineSwitch, engine.empty() ?
"1" : engine);

[alito, 2017/05/24 03:20:23]

Done.

+
+  // If metrics is enabled, we can enable crash reporting in the Chrome Cleaner
+  // process.
+  if (metrics_status == ChromeMetricsStatus::kEnabled) {
+    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_status == CleanerLogsStatus::kUploadEnabled)
+    command_line_.AppendSwitch(chrome_cleaner::kEnableCleanerLoggingSwitch);
+}
+
+ChromeCleanerRunner::LaunchStatus
+ChromeCleanerRunner::LaunchAndWaitForExitOnBackgroundThread() {
+  constexpr int kBadProcessExitCode = std::numeric_limits<int>::max();
+
+  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 =
+      g_test_delegate
+          ? g_test_delegate->LaunchTestProcess(command_line_, launch_options)
+          : base::LaunchProcess(command_line_, launch_options);
+
+  if (!cleaner_process.IsValid())
+    return {false, kBadProcessExitCode};
+
+  // 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()));
+
+  int exit_code = kBadProcessExitCode;
+  if (cleaner_process.WaitForExit(&exit_code))
+    return {true, exit_code};
+  return {false, kBadProcessExitCode};
+}
+
+ChromeCleanerRunner::~ChromeCleanerRunner() {
+  DCHECK(!chrome_prompt_impl_);
+}
+
+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)));
+  // Ensure that this object is not destroyed before chrome_prompt_impl_ has
+  // been released in ReleaseChromePromptImpl(). This is a bit paranoid, but
+  // better safe than sorry.
+  AddRef();
+}
+
+void ChromeCleanerRunner::ReleaseChromePromptImpl() {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  DCHECK(chrome_prompt_impl_);
+  chrome_prompt_impl_.release();
+  // Balanced by the call to AddRef() in CreateChromePromptImpl().
+  Release();
+}
+
+void ChromeCleanerRunner::OnPromptUser(
+    std::unique_ptr<std::set<base::FilePath>> files_to_delete,
+    ChromePrompt::PromptUserCallback prompt_user_callback) {
+  if (on_prompt_user_) {
+    task_runner_->PostTask(FROM_HERE,
+                           base::BindOnce(std::move(on_prompt_user_),
+                                          base::Passed(&files_to_delete),
+                                          base::Passed(&prompt_user_callback)));
+  }
+}
+
+void ChromeCleanerRunner::OnConnectionClosed() {
+  BrowserThread::GetTaskRunnerForThread(BrowserThread::IO)
+      ->PostTask(FROM_HERE,
+                 base::Bind(&ChromeCleanerRunner::ReleaseChromePromptImpl,
+                            base::RetainedRef(this)));
+
+  if (on_connection_closed_)
+    task_runner_->PostTask(FROM_HERE, std::move(on_connection_closed_));
+}
+
+void ChromeCleanerRunner::OnProcessDone(LaunchStatus launch_status) {
+  if (on_process_done_) {
+    task_runner_->PostTask(
+        FROM_HERE, base::BindOnce(std::move(on_process_done_), launch_status));
+  }
+}
+
+void SetChromeCleanerRunnerTestDelegateForTesting(
+    ChromeCleanerRunnerTestDelegate* test_delegate) {
+  g_test_delegate = test_delegate;
+}
+
+}  // namespace safe_browsing