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 "base/threading/thread_task_runner_handle.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,
+};
+
+// 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.

[csharp, 2017/05/24 20:49:35]

Worth mentioning it returns an invalid process and what that means it can't do?

[alito, 2017/05/24 22:20:50]

Done.

+//
+// 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)),
+        base::ThreadTaskRunnerHandle::Get());
+  }
+
+  // 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, LaunchParams) {
+  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)));

[csharp, 2017/05/24 20:49:35]

nit: I wonder if it is worth adding a helper function to convert these enums to
ints

[alito, 2017/05/24 22:20:50]

There is just this instance in this file... so probably not?

[csharp, 2017/05/25 14:44:46]

For this file yes, but I think there are 3 or 4 cases total in this cl, so it
would be a helper with the same scope as ExecutionMode (same basic idea as
channel as int)

[alito, 2017/05/25 18:39:13]

As discussed, this particular enum is cast only once in this directory, so no
need for a helper function.

+
+  // Ensure that the engine flag is always set and that it correctly reflects
+  // the value of the same flag in the SwReporterInvocation() that was passed to
+  // ChromeCleanerRunner::RunChromeCleanerAndReplyWithExitCode(). In the tests,
+  // the engine flag in the SwReporterInvocation is controlled by the value of
+  // |reporter_engine_|.
+  //
+  // If the engine switch is missing in reporter invocation, it should still be
+  // explicitly set to the value "1" for the Cleaner.
+  std::string expected_engine_switch =
+      reporter_engine_ == ReporterEngine::kNewEngine ? "2" : "1";
+  EXPECT_EQ(command_line_.GetSwitchValueASCII(chrome_cleaner::kEngineSwitch),
+            expected_engine_switch);
+
+  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)));
+
+// Enum to be used as parameter for the ChromeCleanerRunnerTest fixture below.
+enum class UwsFoundState {
+  kNoUwsFound,
+  kUwsFoundRebootRequired,
+  kUwsFoundNoRebootRequired,
+};
+
+// Test fixture for testing ChromeCleanerRunner with a mock Chrome Cleaner
+// process.
+//
+// Parameters:
+//
+// - uws_found_state (UwsFoundState): Whether the Chrome Cleaner process should
+//       find UwS on the system and if so whether reboot is required.
+// - 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<UwsFoundState,
+                     MockChromeCleanerProcess::CrashPoint,
+                     PromptAcceptance>>,
+      public ChromeCleanerRunnerTestDelegate {
+ public:
+  void SetUp() override {
+    UwsFoundState uws_found_state;
+    MockChromeCleanerProcess::CrashPoint crash_point;
+    PromptAcceptance prompt_acceptance_to_send;
+    std::tie(uws_found_state, crash_point, prompt_acceptance_to_send) =
+        GetParam();
+
+    ASSERT_FALSE(uws_found_state == UwsFoundState::kNoUwsFound &&
+                 prompt_acceptance_to_send != PromptAcceptance::DENIED);
+
+    cleaner_process_options_.SetDoFindUws(uws_found_state !=
+                                          UwsFoundState::kNoUwsFound);
+    cleaner_process_options_.set_reboot_required(
+        uws_found_state == UwsFoundState::kUwsFoundRebootRequired);
+    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)),
+        base::ThreadTaskRunnerHandle::Get());
+  }
+
+  // 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() {

[csharp, 2017/05/24 20:49:35]

I don't quite understand what this name means. This function just returns, would
the name be clearer as WaitUntilCommunicationDone?

[alito, 2017/05/24 22:20:50]

It is not really waiting... so I tried, QuitTestRunLoopIfCommunicationDone.
better?

+    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());
+
+  // Since failures in any of the above calls to EXPECT_*() do not actually fail
+  // the test, we need to ensure that we return an exit code to indicate test
+  // failure in such cases.
+  if (::testing::Test::HasFailure())
+    return MockChromeCleanerProcess::kInternalTestFailureExitCode;
+
+  MockChromeCleanerProcess mock_cleaner_process(options,
+                                                chrome_mojo_pipe_token);
+  return mock_cleaner_process.Run();
+}
+
+TEST_P(ChromeCleanerRunnerTest, MockChromeCleanerProcess) {

[csharp, 2017/05/24 20:49:35]

Maybe change MockChromeCleanerProcess so it isn't the exact same as the class
name? :)

[alito, 2017/05/24 22:20:50]

Done.

+  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(
+    All,
+    ChromeCleanerRunnerTest,
+    Combine(
+        Values(UwsFoundState::kNoUwsFound),
+        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(UwsFoundState::kUwsFoundRebootRequired,
+               UwsFoundState::kUwsFoundNoRebootRequired),
+        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