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/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 care what sequence this is on,
as long as all the callbacks get called on the same sequence.

[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 of the cleaner_runner outside
of this function (instead of passing in that list of variables, you'd just need
on_process_done and a pointer to the ChromeCleanerRunner)

[alito, 2017/05/22 23:26:31]

One benefit of having the static function create the instance is that users of
ChromeCleanerRunner won't have to manage the lifetime of ChromeCleanerRunner
instances. At least, this way, passing these arguments to the constructor are an
implementation detail that are hidden away from clients.

+      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 also want to save "task_runner_ = SequencedTaskRunnerHandle::Get()" and
replace all the "GetTaskRunnerForThread(BrowserThread::UI)->PostTask" with
task_runner_->PostTask

[alito, 2017/05/22 23:26:30]

Now using ThreadTaskRunnerHandle::Get() to get a SingleThreadTaskRunner. See my
response in the .h file for justification.

+  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 addition to the exit code int
to make it clearer (instead of overloading the exit code value)

[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 null here? Otherwise it'll be
released on the wrong thread, if ReleaseChromePromptImpl wasn't called.

[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