Minor fixes to chrome_cleaner to prepare for refactor

This fixes some errors I noticed while doing a refactor. Keeping them to
their own patch will make the refactor more clear:

* Remove an unused include.
* ChromePromptImpl::PromptUser should never be called twice. If this assertion
  is violated, on_prompt_user_ (a OnceCallback) will be null the second time.
  This case was handled incorrectly: the result callback would never be called,
  which is a runtime error in mojo. Better to just DCHECK.
* Add a PromptUser call to NotInstalledExtensionTest, to ensure that
  DisableExtensions returns false because the extension is not installed and not
  just because the user was never prompted.
* Add a missed early return on an invalidated WeakPtr.
* Simplify the MockChromeCleanerProcess setup to reduce repeated code.

R=olivierli

Bug: 969139
Change-Id: I6ad47831d02487309dc35aa5c67d7533178ae386
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1641994Reviewed-by: proberge <proberge@chromium.org>
Commit-Queue: Joe Mason <joenotcharles@google.com>
Cr-Commit-Position: refs/heads/master@{#666351}

chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_controller_impl_win.cc

@@ -637,6 +637,7 @@ void ChromeCleanerControllerImpl::WeakOnPromptUser(
     base::CreateSingleThreadTaskRunnerWithTraits({BrowserThread::IO})
         ->PostTask(FROM_HERE, base::BindOnce(std::move(prompt_user_callback),
                                              PromptAcceptance::DENIED));
+    return;
   }
 
   controller->OnPromptUser(std::move(scanner_results),

chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_controller_impl_win_unittest.cc

@@ -476,23 +476,11 @@ class ChromeCleanerControllerTest
 };
 
 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.
+  MockChromeCleanerProcess mock_cleaner_process;
+  EXPECT_TRUE(mock_cleaner_process.InitWithCommandLine(
+      *base::CommandLine::ForCurrentProcess()));
   if (::testing::Test::HasFailure())
     return MockChromeCleanerProcess::kInternalTestFailureExitCode;
-
-  MockChromeCleanerProcess mock_cleaner_process(options,
-                                                chrome_mojo_pipe_token);
   return mock_cleaner_process.Run();
 }
 

chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_controller_win.h

@@ -13,7 +13,6 @@
 #include "base/macros.h"
 #include "chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_scanner_results_win.h"
 #include "chrome/browser/safe_browsing/chrome_cleaner/sw_reporter_invocation_win.h"
-#include "components/chrome_cleaner/public/interfaces/chrome_prompt.mojom.h"
 
 class Profile;
 

chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_runner_win_unittest.cc

@@ -367,23 +367,11 @@ class ChromeCleanerRunnerTest
 };
 
 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.
+  MockChromeCleanerProcess mock_cleaner_process;
+  EXPECT_TRUE(mock_cleaner_process.InitWithCommandLine(
+      *base::CommandLine::ForCurrentProcess()));
   if (::testing::Test::HasFailure())
     return MockChromeCleanerProcess::kInternalTestFailureExitCode;
-
-  MockChromeCleanerProcess mock_cleaner_process(options,
-                                                chrome_mojo_pipe_token);
   return mock_cleaner_process.Run();
 }
 

chrome/browser/safe_browsing/chrome_cleaner/mock_chrome_cleaner_process_win.cc

@@ -21,6 +21,7 @@
 #include "base/values.h"
 #include "build/build_config.h"
 #include "chrome/grit/generated_resources.h"
+#include "components/chrome_cleaner/public/constants/constants.h"
 #include "extensions/browser/extension_registry.h"
 #include "extensions/common/extension.h"
 #include "extensions/common/manifest.h"
@@ -323,10 +324,16 @@ int MockChromeCleanerProcess::Options::ExpectedExitCode(
   return kDeclinedExitCode;
 }
 
-MockChromeCleanerProcess::MockChromeCleanerProcess(
-    const Options& options,
-    const std::string& chrome_mojo_pipe_token)
-    : options_(options), chrome_mojo_pipe_token_(chrome_mojo_pipe_token) {}
+bool MockChromeCleanerProcess::InitWithCommandLine(
+    const base::CommandLine& command_line) {
+  if (!Options::FromCommandLine(command_line, &options_))
+    return false;
+  chrome_mojo_pipe_token_ = command_line.GetSwitchValueASCII(
+      chrome_cleaner::kChromeMojoPipeTokenSwitch);
+  if (chrome_mojo_pipe_token_.empty())
+    return false;
+  return true;
+}
 
 int MockChromeCleanerProcess::Run() {
   // We use EXPECT_*() macros to get good log lines, but since this code is run

chrome/browser/safe_browsing/chrome_cleaner/mock_chrome_cleaner_process_win.h

@@ -22,16 +22,12 @@ namespace safe_browsing {
 //
 // 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 = ...
-//   EXPECT_FALSE(chrome_mojo_pipe_token.empty())
 //
+//   MockChromeCleanerProcess mock_cleaner_process;
+//   EXPECT_TRUE(mock_cleaner_process.InitWithCommandLine(*command_line));
 //   if (::testing::Test::HasFailure())
 //     return MockChromeCleanerProcess::kInternalTestFailureExitCode;
-//   MockChromeCleanerProcess mock_cleaner_process(options,
-//                                                 chrome_mojo_pipe_token);
+//
 //   return mock_cleaner_process.Run();
 // }
 class MockChromeCleanerProcess {
@@ -157,8 +153,10 @@ class MockChromeCleanerProcess {
         chrome_cleaner::mojom::PromptAcceptance::UNSPECIFIED;
   };
 
-  MockChromeCleanerProcess(const Options& options,
-                           const std::string& chrome_mojo_pipe_token);
+  MockChromeCleanerProcess() = default;
+  ~MockChromeCleanerProcess() = default;
+
+  bool InitWithCommandLine(const base::CommandLine& command_line);
 
   // Call this in the main function of the mock Chrome Cleaner process. Returns
   // the exit code that should be used when the process exits.

chrome/browser/safe_browsing/chrome_cleaner/srt_chrome_prompt_impl_win.cc

@@ -53,7 +53,7 @@ void ChromePromptImpl::PromptUser(
       ChromeCleanerScannerResults::RegistryKeyCollection;
   using ExtensionCollection = ChromeCleanerScannerResults::ExtensionCollection;
 
-  if (on_prompt_user_) {
+  DCHECK(on_prompt_user_);
   if (base::FeatureList::IsEnabled(kChromeCleanupExtensionsFeature) &&
       extension_ids) {
     extension_ids_ = extension_ids.value();
@@ -63,16 +63,15 @@ void ChromePromptImpl::PromptUser(
 
   ChromeCleanerScannerResults scanner_results(
       FileCollection(files_to_delete.begin(), files_to_delete.end()),
-        registry_keys ? RegistryKeyCollection(registry_keys->begin(),
-                                              registry_keys->end())
+      registry_keys
+          ? RegistryKeyCollection(registry_keys->begin(), registry_keys->end())
           : RegistryKeyCollection(),
-        extension_ids_.empty() ? ExtensionCollection()
-                               : ExtensionCollection(extension_ids_.begin(),
-                                                     extension_ids_.end()));
+      extension_ids_.empty()
+          ? ExtensionCollection()
+          : ExtensionCollection(extension_ids_.begin(), extension_ids_.end()));
 
   std::move(on_prompt_user_)
       .Run(std::move(scanner_results), std::move(callback));
-  }
 }
 
 // The |extensions_ids| passed to this function are a subset of the

chrome/browser/safe_browsing/chrome_cleaner/srt_delete_extension_win_unittest.cc

@@ -158,6 +158,7 @@ TEST_F(ExtensionDeletionEnabledTest, CantDeleteNonPromptedExtensions) {
       std::make_unique<ChromePromptImpl>(extension_service, nullptr,
                                          base::DoNothing(), base::DoNothing());
 
+  // Call DisableExtensions without prompting.
   std::vector<base::string16> extensions_to_disable{extension_ids[0]};
   chrome_prompt->DisableExtensions(
       extensions_to_disable,
@@ -186,6 +187,7 @@ TEST_F(ExtensionDeletionEnabledTest, CantDeleteNonPromptedExtensions) {
                 base::UTF16ToUTF8(extension_ids[2])),
             nullptr);
 
+  // Prompt for an extension but try to disable different ones.
   chrome_prompt->PromptUser({}, {}, {{extension_ids[2]}}, base::DoNothing());
   chrome_prompt->DisableExtensions(
       {extension_ids[0], extension_ids[1]},
@@ -244,6 +246,7 @@ TEST_F(ExtensionDeletionEnabledTest, NotInstalledExtensionTest) {
       std::make_unique<ChromePromptImpl>(extension_service, nullptr,
                                          base::DoNothing(), base::DoNothing());
 
+  chrome_prompt->PromptUser({}, {}, extension_ids, base::DoNothing());
   chrome_prompt->DisableExtensions(
       extension_ids, base::BindOnce([](bool result) { EXPECT_FALSE(result); }));
 }