Chrome Cleaner UI: reporter no longer uses mojo.

chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win_unittest.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 <tuple>
+#include <utility>
+#include <vector>
+
+#include "base/logging.h"
+#include "base/process/launch.h"
+#include "base/process/process.h"
+#include "base/run_loop.h"
+#include "base/single_thread_task_runner.h"
+#include "base/strings/string_number_conversions.h"
+#include "base/test/multiprocess_test.h"
+#include "base/test/scoped_feature_list.h"
+#include "base/threading/thread.h"
+#include "chrome/browser/safe_browsing/chrome_cleaner/mock_chrome_cleaner_process_win.h"
+#include "chrome/browser/safe_browsing/chrome_cleaner/srt_field_trial_win.h"
+#include "components/chrome_cleaner/public/constants/constants.h"
+#include "components/chrome_cleaner/public/interfaces/chrome_prompt.mojom.h"
+#include "content/public/browser/browser_thread.h"
+#include "content/public/test/test_browser_thread_bundle.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "testing/multiprocess_func_list.h"
+
+namespace safe_browsing {
+namespace {
+
+using ::chrome_cleaner::mojom::ChromePrompt;
+using ::chrome_cleaner::mojom::PromptAcceptance;
+using ::content::BrowserThread;
+using ::testing::Bool;
+using ::testing::Combine;
+using ::testing::Values;
+using ChromeMetricsStatus = ChromeCleanerRunner::ChromeMetricsStatus;
+using CleanerLogsStatus = ChromeCleanerRunner::CleanerLogsStatus;
+
+enum class ReporterEngine {
+  kUnspecified,
+  kOldEngine,
+  kNewEngine,
+};
+
+std::string ReporterEngineToString(ReporterEngine reporter_engine) {
+  switch (reporter_engine) {
+    case ReporterEngine::kUnspecified:  // Fallthrough.
+    case ReporterEngine::kOldEngine:
+      return "1";
+    case ReporterEngine::kNewEngine:
+      return "2";
+  }
+  NOTREACHED();
+  return std::string();
+}
+
+// Simple test fixture that intercepts the launching of the Chrome Cleaner
+// process. Intended for testing simple things like command line flags that
+// Chrome sends to the Chrome Cleaner process.
+//
+// Parameters:
+// - metrics_status (ChromeMetricsStatus): whether Chrome metrics reporting is
+//       enabled
+// - logs_upload_status (CleanerLogsStatus): whether logs upload in the Cleaner
+//       process should be enabled.
+// - reporter_engine (ReporterEngine): the type of Cleaner engine specified in
+//       the SwReporterInvocation.
+class ChromeCleanerRunnerSimpleTest
+    : public testing::TestWithParam<
+          std::tuple<ChromeCleanerRunner::ChromeMetricsStatus,
+                     ChromeCleanerRunner::CleanerLogsStatus,
+                     ReporterEngine>>,
+      public ChromeCleanerRunnerTestDelegate {
+ public:
+  ChromeCleanerRunnerSimpleTest()
+      : command_line_(base::CommandLine::NO_PROGRAM) {}
+
+  void SetUp() override {
+    std::tie(metrics_status_, logs_upload_status_, reporter_engine_) =
+        GetParam();
+
+    SetChromeCleanerRunnerTestDelegateForTesting(this);
+    scoped_feature_list_.InitAndEnableFeature(kInBrowserCleanerUIFeature);
+  }
+
+  void CallRunChromeCleaner() {
+    SwReporterInvocation reporter_invocation;
+    switch (reporter_engine_) {
+      case ReporterEngine::kUnspecified:
+        // No engine switch.
+        break;
+      case ReporterEngine::kOldEngine:
+        reporter_invocation.command_line.AppendSwitchASCII(
+            chrome_cleaner::kEngineSwitch, "1");
+        break;
+      case ReporterEngine::kNewEngine:
+        reporter_invocation.command_line.AppendSwitchASCII(
+            chrome_cleaner::kEngineSwitch, "2");
+        break;
+    }
+
+    ChromeCleanerRunner::RunChromeCleanerAndReplyWithExitCode(
+        base::FilePath(FILE_PATH_LITERAL("cleaner.exe")), reporter_invocation,
+        metrics_status_, logs_upload_status_,
+        base::BindOnce(&ChromeCleanerRunnerSimpleTest::OnPromptUser,
+                       base::Unretained(this)),
+        base::BindOnce(&ChromeCleanerRunnerSimpleTest::OnConnectionClosed,
+                       base::Unretained(this)),
+        base::BindOnce(&ChromeCleanerRunnerSimpleTest::OnProcessDone,
+                       base::Unretained(this)));
+  }
+
+  // ChromeCleanerRunnerTestDelegate overrides.
+
+  base::Process LaunchTestProcess(
+      const base::CommandLine& command_line,
+      const base::LaunchOptions& launch_options) override {
+    command_line_ = command_line;
+    // Return an invalid process.
+    return base::Process();
+  }
+
+  base::CommandLine GetTestBaseCommandLine() override {
+    return base::CommandLine(base::CommandLine::NO_PROGRAM);
+  }
+
+  // IPC callbacks.
+
+  void OnPromptUser(std::unique_ptr<std::set<base::FilePath>> files_to_delete,
+                    ChromePrompt::PromptUserCallback response) {}
+
+  void OnConnectionClosed() {}
+
+  void OnProcessDone(ChromeCleanerRunner::LaunchStatus launch_status) {
+    on_process_done_called_ = true;
+    launch_status_ = launch_status;
+    run_loop_.QuitWhenIdle();
+  }
+
+ protected:
+  content::TestBrowserThreadBundle test_browser_thread_bundle_;
+  base::test::ScopedFeatureList scoped_feature_list_;
+
+  // Test fixture parameters.
+  ChromeCleanerRunner::ChromeMetricsStatus metrics_status_;
+  ChromeCleanerRunner::CleanerLogsStatus logs_upload_status_;
+  ReporterEngine reporter_engine_;
+
+  // Set by LaunchTestProcess.
+  base::CommandLine command_line_;
+
+  // Variables set by OnProcessDone().
+  bool on_process_done_called_ = false;
+  ChromeCleanerRunner::LaunchStatus launch_status_;
+
+  base::RunLoop run_loop_;
+};
+
+TEST_P(ChromeCleanerRunnerSimpleTest, InterceptLaunchParams) {

[Joe Mason, 2017/05/23 15:39:36]

Nit: I would just name this test "LaunchParams". Normally tests are named after
what they're testing, not the action they take.

[alito, 2017/05/23 18:58:57]

Done.

+  CallRunChromeCleaner();
+  run_loop_.Run();
+
+  EXPECT_TRUE(on_process_done_called_);
+
+  EXPECT_EQ(
+      command_line_.GetSwitchValueASCII(chrome_cleaner::kExecutionModeSwitch),
+      base::IntToString(
+          static_cast<int>(chrome_cleaner::ExecutionMode::kScanning)));
+
+  EXPECT_EQ(command_line_.GetSwitchValueASCII(chrome_cleaner::kEngineSwitch),
+            ReporterEngineToString(reporter_engine_));

[Joe Mason, 2017/05/23 15:39:36]

It took me a while to realize this wasn't a useless check, because
CallRunChromeCleaner (which is not the code being tested) sets kEngineSwitch on
"command_line". This could use a comment saying exactly what's being tested.

Nit: Also I think it would be clearer in this case to put the conversion from
kEngineSwitch to string in this function instead of having to jump up to a
helper function to see exactly what's being tested.

[alito, 2017/05/23 18:58:57]

Done.

+
+  EXPECT_EQ(metrics_status_ == ChromeMetricsStatus::kEnabled,
+            command_line_.HasSwitch(chrome_cleaner::kUmaUserSwitch));
+  EXPECT_EQ(
+      metrics_status_ == ChromeMetricsStatus::kEnabled,
+      command_line_.HasSwitch(chrome_cleaner::kEnableCrashReportingSwitch));
+
+  EXPECT_EQ(
+      logs_upload_status_ == CleanerLogsStatus::kUploadEnabled,
+      command_line_.HasSwitch(chrome_cleaner::kEnableCleanerLoggingSwitch));
+}
+
+INSTANTIATE_TEST_CASE_P(
+    All,
+    ChromeCleanerRunnerSimpleTest,
+    Combine(Values(ChromeCleanerRunner::ChromeMetricsStatus::kEnabled,
+                   ChromeCleanerRunner::ChromeMetricsStatus::kDisabled),
+            Values(ChromeCleanerRunner::CleanerLogsStatus::kUploadEnabled,
+                   ChromeCleanerRunner::CleanerLogsStatus::kUploadDisabled),
+            Values(ReporterEngine::kUnspecified,
+                   ReporterEngine::kOldEngine,
+                   ReporterEngine::kNewEngine)));
+
+// Test fixture for testing ChromeCleanerRunner with a mock Chrome Cleaner
+// process.
+//
+// Parameters:
+//
+// - found_uws (bool): Whether the Chrome Cleaner process should find UwS on the
+//       system.
+// - reboot_required (bool) : Whether the Chrome Cleaner process should exit
+//       with an exit code indicating that that a reboot is required. Must not
+//       be set to true when the |found_uws| parameter is false.
+// - crash_point (CrashPoint): a single crash point where the Chrome Cleaner
+//       process will crash. See the MockChromeCleanerProcess documentation for
+//       possible values.
+// - prompt_acceptance_to_send (PromptAcceptance): the prompt acceptance value
+//       that Chrome should send to the Chrome Cleaner process. Must be DENIED
+//       if |found_uws| is false.
+class ChromeCleanerRunnerTest
+    : public testing::TestWithParam<
+          std::tuple<bool,
+                     bool,
+                     MockChromeCleanerProcess::CrashPoint,
+                     PromptAcceptance>>,
+      public ChromeCleanerRunnerTestDelegate {
+ public:
+  void SetUp() override {
+    bool found_uws;
+    bool reboot_required;
+    MockChromeCleanerProcess::CrashPoint crash_point;
+    PromptAcceptance prompt_acceptance_to_send;
+    std::tie(found_uws, reboot_required, crash_point,
+             prompt_acceptance_to_send) = GetParam();
+
+    ASSERT_FALSE(!found_uws && reboot_required);
+    ASSERT_TRUE(found_uws ||
+                prompt_acceptance_to_send == PromptAcceptance::DENIED);
+
+    cleaner_process_options_.SetDoFindUws(found_uws);
+    cleaner_process_options_.set_reboot_required(reboot_required);
+    cleaner_process_options_.set_crash_point(crash_point);
+    prompt_acceptance_to_send_ = prompt_acceptance_to_send;
+
+    SetChromeCleanerRunnerTestDelegateForTesting(this);
+    scoped_feature_list_.InitAndEnableFeature(kInBrowserCleanerUIFeature);
+  }
+
+  void CallRunChromeCleaner() {
+    ChromeCleanerRunner::RunChromeCleanerAndReplyWithExitCode(
+        base::FilePath(FILE_PATH_LITERAL("cleaner.exe")),
+        SwReporterInvocation(), ChromeMetricsStatus::kDisabled,
+        CleanerLogsStatus::kUploadDisabled,
+        base::BindOnce(&ChromeCleanerRunnerTest::OnPromptUser,
+                       base::Unretained(this)),
+        base::BindOnce(&ChromeCleanerRunnerTest::OnConnectionClosed,
+                       base::Unretained(this)),
+        base::BindOnce(&ChromeCleanerRunnerTest::OnProcessDone,
+                       base::Unretained(this)));
+  }
+
+  // ChromeCleanerRunnerTestDelegate overrides.
+
+  base::Process LaunchTestProcess(
+      const base::CommandLine& command_line,
+      const base::LaunchOptions& launch_options) override {
+    base::SpawnChildResult result = base::SpawnMultiProcessTestChild(
+        "MockChromeCleanerProcessMain", command_line, launch_options);
+
+    EXPECT_TRUE(result.process.IsValid());
+    return std::move(result.process);
+  }
+
+  base::CommandLine GetTestBaseCommandLine() override {
+    base::CommandLine command_line =
+        base::GetMultiProcessTestChildBaseCommandLine();
+    cleaner_process_options_.AddSwitchesToCommandLine(&command_line);
+    return command_line;
+  }
+
+  // IPC callbacks.
+
+  // Will receive the main Mojo message from the Mock Chrome Cleaner process.
+  void OnPromptUser(std::unique_ptr<std::set<base::FilePath>> files_to_delete,
+                    ChromePrompt::PromptUserCallback response) {
+    on_prompt_user_called_ = true;
+    received_files_to_delete_ = std::move(files_to_delete);
+    BrowserThread::GetTaskRunnerForThread(BrowserThread::IO)
+        ->PostTask(FROM_HERE, base::BindOnce(std::move(response),
+                                             prompt_acceptance_to_send_));
+  }
+
+  void ContinueTestIfCommunicationDone() {
+    if (on_process_done_called_ && on_connection_closed_called_)
+      run_loop_.QuitWhenIdle();
+  }
+
+  void OnConnectionClosed() {
+    on_connection_closed_called_ = true;
+    ContinueTestIfCommunicationDone();
+  }
+
+  void OnProcessDone(ChromeCleanerRunner::LaunchStatus launch_status) {
+    on_process_done_called_ = true;
+    launch_status_ = launch_status;
+    ContinueTestIfCommunicationDone();
+  }
+
+ protected:
+  content::TestBrowserThreadBundle test_browser_thread_bundle_;
+  base::test::ScopedFeatureList scoped_feature_list_;
+
+  base::RunLoop run_loop_;
+
+  MockChromeCleanerProcess::Options cleaner_process_options_;
+  PromptAcceptance prompt_acceptance_to_send_ = PromptAcceptance::UNSPECIFIED;
+
+  // Set by OnProcessDone().
+  ChromeCleanerRunner::LaunchStatus launch_status_ = {false, -1};
+  // Set by OnPromptUser().
+  std::unique_ptr<std::set<base::FilePath>> received_files_to_delete_;
+
+  bool on_prompt_user_called_ = false;
+  bool on_connection_closed_called_ = false;
+  bool on_process_done_called_ = false;
+};
+
+MULTIPROCESS_TEST_MAIN(MockChromeCleanerProcessMain) {
+  base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();
+  MockChromeCleanerProcess::Options options;
+  EXPECT_TRUE(MockChromeCleanerProcess::Options::FromCommandLine(*command_line,
+                                                                 &options));
+
+  std::string chrome_mojo_pipe_token = command_line->GetSwitchValueASCII(
+      chrome_cleaner::kChromeMojoPipeTokenSwitch);
+  EXPECT_FALSE(chrome_mojo_pipe_token.empty());
+
+  if (::testing::Test::HasFailure())

[Joe Mason, 2017/05/23 15:39:36]

When can this happen?

[alito, 2017/05/23 18:58:57]

Added a comment explaining this here as well.

+    return MockChromeCleanerProcess::kInternalTestFailureExitCode;
+
+  MockChromeCleanerProcess mock_cleaner_process(options,
+                                                chrome_mojo_pipe_token);
+  return mock_cleaner_process.Run();
+}
+
+TEST_P(ChromeCleanerRunnerTest, MockChromeCleanerProcess) {
+  CallRunChromeCleaner();
+  run_loop_.Run();
+
+  EXPECT_TRUE(on_process_done_called_);
+  EXPECT_TRUE(on_connection_closed_called_);
+  EXPECT_EQ(on_prompt_user_called_,
+            (cleaner_process_options_.crash_point() ==
+                 MockChromeCleanerProcess::CrashPoint::kNone ||
+             cleaner_process_options_.crash_point() ==
+                 MockChromeCleanerProcess::CrashPoint::kAfterResponseReceived));
+
+  if (on_prompt_user_called_ && !cleaner_process_options_.found_uws().empty()) {
+    EXPECT_EQ(*received_files_to_delete_,
+              cleaner_process_options_.files_to_delete());
+  }
+
+  EXPECT_TRUE(launch_status_.process_ok);
+  EXPECT_EQ(launch_status_.exit_code, cleaner_process_options_.ExpectedExitCode(
+                                          prompt_acceptance_to_send_));
+}
+
+INSTANTIATE_TEST_CASE_P(
+    NoUwsFound,
+    ChromeCleanerRunnerTest,
+    Combine(
+        Values(false),

[Joe Mason, 2017/05/23 15:39:36]

It would be easier to read if this were an enum instead of bool. (I suggest
FoundNothing, FoundUwSWithoutReboot, FoundUwSWithReboot instead of separate
enums.) 

[alito, 2017/05/23 18:58:57]

Done.

+        Values(false),
+        Values(MockChromeCleanerProcess::CrashPoint::kNone,
+               MockChromeCleanerProcess::CrashPoint::kOnStartup,
+               MockChromeCleanerProcess::CrashPoint::kAfterConnection,
+               MockChromeCleanerProcess::CrashPoint::kAfterRequestSent,
+               MockChromeCleanerProcess::CrashPoint::kAfterResponseReceived),
+        Values(PromptAcceptance::DENIED)));
+
+INSTANTIATE_TEST_CASE_P(
+    UwsFound,
+    ChromeCleanerRunnerTest,
+    Combine(
+        Values(true),
+        Bool(),
+        Values(MockChromeCleanerProcess::CrashPoint::kNone,
+               MockChromeCleanerProcess::CrashPoint::kOnStartup,
+               MockChromeCleanerProcess::CrashPoint::kAfterConnection,
+               MockChromeCleanerProcess::CrashPoint::kAfterRequestSent,
+               MockChromeCleanerProcess::CrashPoint::kAfterResponseReceived),
+        Values(PromptAcceptance::DENIED,
+               PromptAcceptance::ACCEPTED_WITH_LOGS,
+               PromptAcceptance::ACCEPTED_WITHOUT_LOGS)));
+
+}  // namespace
+}  // namespace safe_browsing