Add ExtensionCleaningFeatureStatus param to ChromeCleanerControllerTest

Now there are an extreme number of permutations of test params, so this
also splits off all the tests where the cleaner process does not start
to cut down on redundant tests.

R=csharp@chromium.org

Bug: 907443
Change-Id: I015c02dd9ed72e3c5951acbdfc8163c89b1bc3ea
Reviewed-on: https://chromium-review.googlesource.com/c/1347437Reviewed-by: Chris Sharp <csharp@chromium.org>
Commit-Queue: Joe Mason <joenotcharles@google.com>
Cr-Commit-Position: refs/heads/master@{#610245}

chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_controller_impl_win_unittest.cc

@@ -14,6 +14,7 @@
 #include "base/run_loop.h"
 #include "base/task/post_task.h"
 #include "base/test/multiprocess_test.h"
+#include "base/test/scoped_feature_list.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "build/build_config.h"
 #include "chrome/browser/extensions/extension_service_test_base.h"
@@ -38,6 +39,7 @@ namespace safe_browsing {
 namespace {
 
 using ::chrome_cleaner::mojom::PromptAcceptance;
+using ::testing::_;
 using ::testing::Bool;
 using ::testing::Combine;
 using ::testing::DoAll;
@@ -46,7 +48,7 @@ using ::testing::SaveArg;
 using ::testing::StrictMock;
 using ::testing::UnorderedElementsAreArray;
 using ::testing::Values;
-using ::testing::_;
+using ::testing::ValuesIn;
 using CrashPoint = MockChromeCleanerProcess::CrashPoint;
 using IdleReason = ChromeCleanerController::IdleReason;
 using ItemsReporting = MockChromeCleanerProcess::ItemsReporting;
@@ -225,9 +227,15 @@ enum class UwsFoundStatus {
   kUwsFoundNoRebootRequired,
 };
 
+enum class ExtensionCleaningFeatureStatus {
+  kEnabled,
+  kDisabled,
+};
+
 typedef std::tuple<CleanerProcessStatus,
                    CrashPoint,
                    UwsFoundStatus,
+                   ExtensionCleaningFeatureStatus,
                    ItemsReporting,
                    ItemsReporting,
                    UserResponse>
@@ -245,10 +253,17 @@ class ChromeCleanerControllerTest
   ~ChromeCleanerControllerTest() override {}
 
   void SetUp() override {
-    InitializeEmptyExtensionService();
     std::tie(process_status_, crash_point_, uws_found_status_,
-             registry_keys_reporting_, extensions_reporting_, user_response_) =
-        GetParam();
+             extension_cleaning_feature_status_, registry_keys_reporting_,
+             extensions_reporting_, user_response_) = GetParam();
+
+    if (extension_cleaning_feature_status_ ==
+        ExtensionCleaningFeatureStatus::kEnabled)
+      features_.InitAndEnableFeature(kChromeCleanupExtensionsFeature);
+    else
+      features_.InitAndDisableFeature(kChromeCleanupExtensionsFeature);
+
+    InitializeEmptyExtensionService();
 
     cleaner_process_options_.SetReportedResults(
         uws_found_status_ != UwsFoundStatus::kNoUwsFound,
@@ -376,6 +391,12 @@ class ChromeCleanerControllerTest
   }
 
   bool ExpectedExtensionsReported() {
+    if (extension_cleaning_feature_status_ ==
+        ExtensionCleaningFeatureStatus::kDisabled) {
+      // Extensions should not be shown to the user even if the cleaner process
+      // reports them, because they cannot be cleaned.
+      return false;
+    }
     return ExpectedOnInfectedCalled() &&
            extensions_reporting_ == ItemsReporting::kReported;
   }
@@ -442,6 +463,7 @@ class ChromeCleanerControllerTest
   CleanerProcessStatus process_status_;
   MockChromeCleanerProcess::CrashPoint crash_point_;
   UwsFoundStatus uws_found_status_;
+  ExtensionCleaningFeatureStatus extension_cleaning_feature_status_;
   ItemsReporting registry_keys_reporting_;
   ItemsReporting extensions_reporting_;
   ChromeCleanerController::UserResponse user_response_;
@@ -456,6 +478,8 @@ class ChromeCleanerControllerTest
   std::vector<Profile*> profiles_to_reset_if_tagged_;
 
   bool reboot_flow_started_ = false;
+
+  base::test::ScopedFeatureList features_;
 };
 
 MULTIPROCESS_TEST_MAIN(MockChromeCleanerProcessMain) {
@@ -668,6 +692,19 @@ std::ostream& operator<<(std::ostream& out, UwsFoundStatus status) {
   }
 }
 
+std::ostream& operator<<(std::ostream& out,
+                         ExtensionCleaningFeatureStatus status) {
+  switch (status) {
+    case ExtensionCleaningFeatureStatus::kEnabled:
+      return out << "ExtensionCleaningEnabled";
+    case ExtensionCleaningFeatureStatus::kDisabled:
+      return out << "ExtensionCleaningDisabled";
+    default:
+      NOTREACHED();
+      return out << "UnknownExtensionCleaningStatus" << status;
+  }
+}
+
 std::ostream& operator<<(std::ostream& out, ItemsReporting items_reporting) {
   switch (items_reporting) {
     case ItemsReporting::kUnsupported:
@@ -711,27 +748,31 @@ struct ChromeCleanerControllerTestParamsToString {
     param_name << std::get<2>(info.param) << "_";
     param_name << std::get<3>(info.param) << "_";
     param_name << std::get<4>(info.param) << "_";
-    param_name << std::get<5>(info.param);
+    param_name << std::get<5>(info.param) << "_";
+    param_name << std::get<6>(info.param);
     return param_name.str();
   }
 };
 
+// This includes all crash points after kOnStartup, except kAfterRequestSent.
+// That one's not used because if we induce a crash after the mock cleaner
+// sends a request, there would be a race condition between the request being
+// received and the connection being lost. So at that point, invoking the error
+// handler and not invoking it are both expected behaviour.
+constexpr CrashPoint kCrashPointsAfterStartup[] = {
+    CrashPoint::kNone, CrashPoint::kAfterConnection,
+    CrashPoint::kAfterResponseReceived};
+
+// Tests where the process gets past the startup phase and finds UwS to clean.
 INSTANTIATE_TEST_CASE_P(
-    All,
+    CleanerFindsUwS,
     ChromeCleanerControllerTest,
-    Combine(Values(CleanerProcessStatus::kFetchFailure,
-                   CleanerProcessStatus::kFetchSuccessInvalidProcess,
-                   CleanerProcessStatus::kFetchSuccessValidProcess),
-            Values(CrashPoint::kNone,
-                   CrashPoint::kOnStartup,
-                   CrashPoint::kAfterConnection,
-                   // CrashPoint::kAfterRequestSent is not used because we
-                   // cannot ensure the order between the Mojo request being
-                   // received by Chrome and the connection being lost.
-                   CrashPoint::kAfterResponseReceived),
-            Values(UwsFoundStatus::kNoUwsFound,
-                   UwsFoundStatus::kUwsFoundRebootRequired,
+    Combine(Values(CleanerProcessStatus::kFetchSuccessValidProcess),
+            ValuesIn(kCrashPointsAfterStartup),
+            Values(UwsFoundStatus::kUwsFoundRebootRequired,
                    UwsFoundStatus::kUwsFoundNoRebootRequired),
+            Values(ExtensionCleaningFeatureStatus::kEnabled,
+                   ExtensionCleaningFeatureStatus::kDisabled),
             Values(ItemsReporting::kUnsupported,
                    ItemsReporting::kNotReported,
                    ItemsReporting::kReported),
@@ -744,6 +785,42 @@ INSTANTIATE_TEST_CASE_P(
                    UserResponse::kDismissed)),
     ChromeCleanerControllerTestParamsToString());
 
+// Tests where the process gets past the startup phase but finds nothing to
+// clean. Since we don't progress to any stage where the parameters after
+// UwsFoundStatus are used, their values would not change the test behaviour,
+// so we can save time by only testing one arbitrary value for each.
+INSTANTIATE_TEST_CASE_P(
+    CleanerFindsNothing,
+    ChromeCleanerControllerTest,
+    Combine(Values(CleanerProcessStatus::kFetchSuccessValidProcess),
+            ValuesIn(kCrashPointsAfterStartup),
+            Values(UwsFoundStatus::kNoUwsFound),
+            Values(ExtensionCleaningFeatureStatus::kDisabled),
+            Values(ItemsReporting::kNotReported),
+            Values(ItemsReporting::kNotReported),
+            Values(UserResponse::kDismissed)),
+    ChromeCleanerControllerTestParamsToString());
+
+// Tests where the process fails before starting a scan. This never gets far
+// enough to collect results, so we can save time by not repeating the tests
+// for every possible combination of results parameters.
+INSTANTIATE_TEST_CASE_P(
+    CleanerFailsToStart,
+    ChromeCleanerControllerTest,
+    Combine(Values(CleanerProcessStatus::kFetchFailure,
+                   CleanerProcessStatus::kFetchSuccessInvalidProcess,
+                   CleanerProcessStatus::kFetchSuccessValidProcess),
+            // If the process status is kFetchSuccessValidProcess, crash on
+            // startup. Otherwise the process never starts so the crash point
+            // doesn't matter.
+            Values(CrashPoint::kOnStartup),
+            Values(UwsFoundStatus::kNoUwsFound),
+            Values(ExtensionCleaningFeatureStatus::kDisabled),
+            Values(ItemsReporting::kUnsupported),
+            Values(ItemsReporting::kUnsupported),
+            Values(UserResponse::kAcceptedWithLogs)),
+    ChromeCleanerControllerTestParamsToString());
+
 // Tests for the interaction between reporter runs and all possible states.
 // Signals from reporter execution may lead to state transitions only if there
 // is no cleaner activity, so it's enough to check the state after a signal.