Chrome Cleanup: Do not use SBER/Scout for Cleaner logs

Now that Chrome Cleanup has its own opt-in, the SBER/Scout state should
not be passed to the Cleaner's command-line to make sure it isn't
accidentally used.

Bug: 690020
Change-Id: I6f1ecb60f2a9d0bdb5c3f4bbab728621082a2879
Reviewed-on: https://chromium-review.googlesource.com/570349Reviewed-by: Chris Sharp <csharp@chromium.org>
Commit-Queue: proberge <proberge@chromium.org>
Cr-Commit-Position: refs/heads/master@{#486784}

chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_controller_win.cc

@@ -157,11 +157,6 @@ void ChromeCleanerControllerDelegate::FetchAndVerifyChromeCleaner(
       base::BindOnce(&OnChromeCleanerFetched, base::Passed(&fetched_callback)));
 }
 
-bool ChromeCleanerControllerDelegate::
-    SafeBrowsingExtendedReportingScoutEnabled() {
-  return safe_browsing::SafeBrowsingExtendedReportingScoutEnabled();
-}
-
 bool ChromeCleanerControllerDelegate::IsMetricsAndCrashReportingEnabled() {
   return ChromeMetricsServiceAccessor::IsMetricsAndCrashReportingEnabled();
 }
@@ -404,14 +399,8 @@ void ChromeCleanerController::OnChromeCleanerFetchedAndVerified(
           ? ChromeCleanerRunner::ChromeMetricsStatus::kEnabled
           : ChromeCleanerRunner::ChromeMetricsStatus::kDisabled;
 
-  ChromeCleanerRunner::CleanerLogsStatus cleaner_logs_status =
-      delegate_->SafeBrowsingExtendedReportingScoutEnabled()
-          ? ChromeCleanerRunner::CleanerLogsStatus::kUploadEnabled
-          : ChromeCleanerRunner::CleanerLogsStatus::kUploadDisabled;
-
   ChromeCleanerRunner::RunChromeCleanerAndReplyWithExitCode(
       executable_path, *reporter_invocation_, metrics_status,
-      cleaner_logs_status,
       base::Bind(&ChromeCleanerController::WeakOnPromptUser,
                  weak_factory_.GetWeakPtr()),
       base::Bind(&ChromeCleanerController::OnConnectionClosed,

chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_controller_win.h

@@ -50,7 +50,6 @@ class ChromeCleanerControllerDelegate {
   // extension. If the operation fails, the file name passed to
   // |fecthed_callback| will be empty.
   virtual void FetchAndVerifyChromeCleaner(FetchedCallback fetched_callback);
-  virtual bool SafeBrowsingExtendedReportingScoutEnabled();
   virtual bool IsMetricsAndCrashReportingEnabled();
 
   // Auxiliary methods for tagging and resetting open profiles.

chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_controller_win_unittest.cc

@@ -84,11 +84,6 @@ enum class MetricsStatus {
   kDisabled,
 };
 
-enum class ScoutStatus {
-  kEnabled,
-  kDisabled,
-};
-
 // Simple test fixture that passes an invalid process handle back to the
 // ChromeCleanerRunner class and is intended for testing simple things like
 // command line flags that Chrome sends to the Chrome Cleaner process.
@@ -96,10 +91,8 @@ enum class ScoutStatus {
 // Parameters:
 // - metrics_status (MetricsStatus): whether Chrome metrics reporting is
 //   enabled.
-// - scout_status (ScoutStatus): whether logs upload in the Cleaner process
-//   should be enabled.
 class ChromeCleanerControllerSimpleTest
-    : public testing::TestWithParam<std::tuple<MetricsStatus, ScoutStatus>>,
+    : public testing::TestWithParam<MetricsStatus>,
       public ChromeCleanerRunnerTestDelegate,
       public ChromeCleanerControllerDelegate {
  public:
@@ -109,12 +102,9 @@ class ChromeCleanerControllerSimpleTest
   ~ChromeCleanerControllerSimpleTest() override {}
 
   void SetUp() override {
-    MetricsStatus metrics_status;
-    ScoutStatus scout_status;
-    std::tie(metrics_status, scout_status) = GetParam();
+    MetricsStatus metrics_status = GetParam();
 
     metrics_enabled_ = metrics_status == MetricsStatus::kEnabled;
-    scout_enabled_ = scout_status == ScoutStatus::kEnabled;
 
     SetChromeCleanerRunnerTestDelegateForTesting(this);
     ChromeCleanerController::ResetInstanceForTesting();
@@ -136,10 +126,6 @@ class ChromeCleanerControllerSimpleTest
         .Run(base::FilePath(FILE_PATH_LITERAL("chrome_cleaner.exe")));
   }
 
-  bool SafeBrowsingExtendedReportingScoutEnabled() override {
-    return scout_enabled_;
-  }
-
   bool IsMetricsAndCrashReportingEnabled() override { return metrics_enabled_; }
 
   void TagForResetting(Profile* profile) override {
@@ -174,7 +160,6 @@ class ChromeCleanerControllerSimpleTest
   base::test::ScopedFeatureList scoped_feature_list_;
 
   bool metrics_enabled_;
-  bool scout_enabled_;
   base::CommandLine command_line_;
 
   StrictMock<MockChromeCleanerControllerObserver> mock_observer_;
@@ -212,17 +197,14 @@ TEST_P(ChromeCleanerControllerSimpleTest, FlagsPassedToCleanerProcess) {
             command_line_.HasSwitch(chrome_cleaner::kUmaUserSwitch));
   EXPECT_EQ(metrics_enabled_, command_line_.HasSwitch(
                                   chrome_cleaner::kEnableCrashReportingSwitch));
-  EXPECT_EQ(scout_enabled_, command_line_.HasSwitch(
-                                chrome_cleaner::kEnableCleanerLoggingSwitch));
 
   controller->RemoveObserver(&mock_observer_);
 }
 
-INSTANTIATE_TEST_CASE_P(
-    All,
-    ChromeCleanerControllerSimpleTest,
-    Combine(Values(MetricsStatus::kDisabled, MetricsStatus::kEnabled),
-            Values(ScoutStatus::kDisabled, ScoutStatus::kEnabled)));
+INSTANTIATE_TEST_CASE_P(All,
+                        ChromeCleanerControllerSimpleTest,
+                        Values(MetricsStatus::kDisabled,
+                               MetricsStatus::kEnabled));
 
 enum class CleanerProcessStatus {
   kFetchFailure,
@@ -290,12 +272,6 @@ class ChromeCleanerControllerTest
                 : base::FilePath()));
   }
 
-  bool SafeBrowsingExtendedReportingScoutEnabled() override {
-    // Returning an arbitrary value since this is not being tested in this
-    // fixture.
-    return false;
-  }
-
   bool IsMetricsAndCrashReportingEnabled() override {
     // Returning an arbitrary value since this is not being tested in this
     // fixture.

chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.cc

@@ -51,16 +51,14 @@ void ChromeCleanerRunner::RunChromeCleanerAndReplyWithExitCode(
     const base::FilePath& cleaner_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(
       cleaner_executable_path, reporter_invocation, metrics_status,
-      cleaner_logs_status, std::move(on_prompt_user),
-      std::move(on_connection_closed), std::move(on_process_done),
-      std::move(task_runner)));
+      std::move(on_prompt_user), std::move(on_connection_closed),
+      std::move(on_process_done), std::move(task_runner)));
   auto launch_and_wait = base::BindOnce(
       &ChromeCleanerRunner::LaunchAndWaitForExitOnBackgroundThread,
       cleaner_runner);
@@ -80,7 +78,6 @@ ChromeCleanerRunner::ChromeCleanerRunner(
     const base::FilePath& cleaner_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,
@@ -133,12 +130,6 @@ ChromeCleanerRunner::ChromeCleanerRunner(
     cleaner_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)
-    cleaner_command_line_.AppendSwitch(
-        chrome_cleaner::kEnableCleanerLoggingSwitch);
 }
 
 ChromeCleanerRunner::ProcessStatus

chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win.h

@@ -54,11 +54,6 @@ class ChromeCleanerRunner
     kDisabled,
   };
 
-  enum class CleanerLogsStatus {
-    kUploadEnabled,
-    kUploadDisabled,
-  };
-
   enum class LaunchStatus {
     // Got an invalid process when attempting to launch the Chrome Cleaner
     // process. As a result, the Mojo pipe was never set up and the
@@ -94,11 +89,11 @@ class ChromeCleanerRunner
   //
   // This function will pass command line flags to the Chrome Cleaner executable
   // as appropriate based on the flags in |reporter_invocation| and the
-  // |metrics_status| and |cleaner_logs_status| parameters. The Cleaner process
-  // will communicate with Chrome via a Mojo IPC interface and any IPC requests
-  // or notifications are passed to the caller via the |on_prompt_user| and
-  // |on_connection_closed| callbacks. Finally, when the Chrome Cleaner process
-  // terminates, a ProcessStatus is passed along to |on_process_done|.
+  // |metrics_status| parameters The Cleaner process will communicate with
+  // Chrome via a Mojo IPC interface and any IPC requests or notifications are
+  // passed to the caller via the |on_prompt_user| and |on_connection_closed|
+  // callbacks. Finally, when the Chrome Cleaner process terminates, a
+  // ProcessStatus is passed along to |on_process_done|.
   //
   // The details of the mojo interface are documented in
   // "components/chrome_cleaner/public/interfaces/chrome_prompt.mojom.h".
@@ -106,7 +101,6 @@ class ChromeCleanerRunner
       const base::FilePath& cleaner_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,
@@ -120,7 +114,6 @@ class ChromeCleanerRunner
   ChromeCleanerRunner(const base::FilePath& cleaner_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,

chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win_unittest.cc

@@ -38,7 +38,6 @@ using ::testing::Bool;
 using ::testing::Combine;
 using ::testing::Values;
 using ChromeMetricsStatus = ChromeCleanerRunner::ChromeMetricsStatus;
-using CleanerLogsStatus = ChromeCleanerRunner::CleanerLogsStatus;
 
 enum class ReporterEngine {
   kUnspecified,
@@ -55,14 +54,11 @@ enum class ReporterEngine {
 // 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:
@@ -70,8 +66,7 @@ class ChromeCleanerRunnerSimpleTest
       : command_line_(base::CommandLine::NO_PROGRAM) {}
 
   void SetUp() override {
-    std::tie(metrics_status_, logs_upload_status_, reporter_engine_) =
-        GetParam();
+    std::tie(metrics_status_, reporter_engine_) = GetParam();
 
     SetChromeCleanerRunnerTestDelegateForTesting(this);
     scoped_feature_list_.InitAndEnableFeature(kInBrowserCleanerUIFeature);
@@ -95,7 +90,7 @@ class ChromeCleanerRunnerSimpleTest
 
     ChromeCleanerRunner::RunChromeCleanerAndReplyWithExitCode(
         base::FilePath(FILE_PATH_LITERAL("cleaner.exe")), reporter_invocation,
-        metrics_status_, logs_upload_status_,
+        metrics_status_,
         base::BindOnce(&ChromeCleanerRunnerSimpleTest::OnPromptUser,
                        base::Unretained(this)),
         base::BindOnce(&ChromeCleanerRunnerSimpleTest::OnConnectionClosed,
@@ -137,7 +132,6 @@ class ChromeCleanerRunnerSimpleTest
 
   // Test fixture parameters.
   ChromeCleanerRunner::ChromeMetricsStatus metrics_status_;
-  ChromeCleanerRunner::CleanerLogsStatus logs_upload_status_;
   ReporterEngine reporter_engine_;
 
   // Set by LaunchTestProcess.
@@ -179,10 +173,6 @@ TEST_P(ChromeCleanerRunnerSimpleTest, LaunchParams) {
   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(
@@ -190,8 +180,6 @@ INSTANTIATE_TEST_CASE_P(
     ChromeCleanerRunnerSimpleTest,
     Combine(Values(ChromeCleanerRunner::ChromeMetricsStatus::kEnabled,
                    ChromeCleanerRunner::ChromeMetricsStatus::kDisabled),
-            Values(ChromeCleanerRunner::CleanerLogsStatus::kUploadEnabled,
-                   ChromeCleanerRunner::CleanerLogsStatus::kUploadDisabled),
             Values(ReporterEngine::kUnspecified,
                    ReporterEngine::kOldEngine,
                    ReporterEngine::kNewEngine)));
@@ -250,7 +238,6 @@ class ChromeCleanerRunnerTest
     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,

components/chrome_cleaner/public/constants/constants.cc

@@ -13,7 +13,6 @@ const char kChromeMojoPipeTokenSwitch[] = "chrome-mojo-pipe-token";
 const char kChromePromptSwitch[] = "chrome-prompt";
 const char kChromeSystemInstallSwitch[] = "chrome-system-install";
 const char kChromeVersionSwitch[] = "chrome-version";
-const char kEnableCleanerLoggingSwitch[] = "enable-cleaner-logging";
 const char kEnableCrashReportingSwitch[] = "enable-crash-reporting";
 const char kEngineExperimentGroupSwitch[] = "engine-experiment-group";
 const char kEngineSwitch[] = "engine";

components/chrome_cleaner/public/constants/constants.h

@@ -33,10 +33,6 @@ extern const char kChromeSystemInstallSwitch[];
 // The Chrome version string.
 extern const char kChromeVersionSwitch[];
 
-// Indicates whether logs upload is enabled in the cleaner process.
-// Takes effect only if execution mode is ExecutionMode::kCleanup.
-extern const char kEnableCleanerLoggingSwitch[];
-
 // Indicates that crash reporting is enabled for the current user.
 extern const char kEnableCrashReportingSwitch[];
 
@@ -116,9 +112,12 @@ enum class ExecutionMode {
   // The cleaner will run in scanning mode. No UI will be shown to the user
   // (UI handled by Chrome) and logs will not be uploaded.
   kScanning = 1,
-  // The cleaner will run in cleanup mode only. No UI will be shown to the
-  // user (UI handled by Chrome) and logs should only be uploaded if
-  // |kEnableCleanerLoggingSwitch| is set.
+  // The cleaner will run in cleaning mode. No UI will be shown to the user
+  // (UI handled by Chrome) and logs will be uploaded if the user did not opt
+  // out of logs collection when it was offered by the Chrome UI.
+  // Chrome should not try to launch the Chrome Cleanup Tool with |kCleanup|.
+  // It should instead communicate through IPC with the cleaner launched with
+  // |kScanning| to ask it to start cleanup.
   kCleanup = 2,
 
   // Auxiliary enumerator for range checking.