Move ChromeInstallerCleanup checking into mini_installer::Configuration.

The debugging bit that controls whether or not the extracted files are
left behind rather than cleaned up is a kind of configuration, so it's
natural that it reside in the Configuration class. This is a refactor
with no behavior change with one exception: this CL fixes a regression
introduced in r282032.

BUG=None

Change-Id: I6dd594d220b9b75a507f4482af49f083b697b3d0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2302152
Commit-Queue: Greg Thompson <grt@chromium.org>
Reviewed-by: Sébastien Marchand <sebmarchand@chromium.org>
Cr-Commit-Position: refs/heads/master@{#789193}

chrome/installer/mini_installer/configuration.cc

@@ -6,6 +6,7 @@
 
 #include <shellapi.h>  // NOLINT
 #include <stddef.h>
+#include <stdlib.h>
 #include <windows.h>
 
 #include "build/branding_buildflags.h"
@@ -41,6 +42,7 @@ Configuration::~Configuration() {
 bool Configuration::Initialize(HMODULE module) {
   Clear();
   ReadResources(module);
+  ReadRegistry();
   return ParseCommandLine(::GetCommandLine());
 }
 
@@ -59,6 +61,7 @@ void Configuration::Clear() {
   argument_count_ = 0;
   is_system_level_ = false;
   has_invalid_switch_ = false;
+  should_delete_extracted_files_ = true;
   previous_version_ = nullptr;
 }
 
@@ -123,4 +126,15 @@ void Configuration::ReadResources(HMODULE module) {
   previous_version_ = version_string;
 }
 
+void Configuration::ReadRegistry() {
+  // Extracted files should not be deleted iff the user has manually created a
+  // ChromeInstallerCleanup string value in the registry under
+  // HKCU\Software\[Google|Chromium] and set its value to "0".
+  wchar_t value[2] = {};
+  should_delete_extracted_files_ =
+      !RegKey::ReadSZValue(HKEY_CURRENT_USER, kCleanupRegistryKey,
+                           kCleanupRegistryValue, value, _countof(value)) ||
+      value[0] != L'0';
+}
+
 }  // namespace mini_installer

chrome/installer/mini_installer/configuration.h

@@ -52,10 +52,16 @@ class Configuration {
   // Returns the previous version contained in the image's resource.
   const wchar_t* previous_version() const { return previous_version_; }
 
+  // Returns true if extracted files should be deleted prior to exit.
+  bool should_delete_extracted_files() const {
+    return should_delete_extracted_files_;
+  }
+
  protected:
   void Clear();
   bool ParseCommandLine(const wchar_t* command_line);
   void ReadResources(HMODULE module);
+  void ReadRegistry();
 
   wchar_t** args_;
   const wchar_t* chrome_app_guid_;
@@ -64,6 +70,7 @@ class Configuration {
   Operation operation_;
   bool is_system_level_;
   bool has_invalid_switch_;
+  bool should_delete_extracted_files_;
   const wchar_t* previous_version_;
 
  private:

chrome/installer/mini_installer/configuration_test.cc

@@ -40,6 +40,7 @@ class ScopedGoogleUpdateIsMachine {
 class TestConfiguration : public Configuration {
  public:
   explicit TestConfiguration(const wchar_t* command_line) {
+    EXPECT_TRUE(Initialize(::GetModuleHandle(nullptr)));
     EXPECT_TRUE(ParseCommandLine(command_line));
   }
 
@@ -144,4 +145,34 @@ TEST_F(MiniInstallerConfigurationTest, HasInvalidSwitch) {
       TestConfiguration(L"spam.exe --chrome-frame").has_invalid_switch());
 }
 
+TEST_F(MiniInstallerConfigurationTest, DeleteExtractedFilesDefaultTrue) {
+  EXPECT_TRUE(TestConfiguration(L"spam.exe").should_delete_extracted_files());
+}
+
+TEST_F(MiniInstallerConfigurationTest, DeleteExtractedFilesFalse) {
+  ASSERT_EQ(
+      base::win::RegKey(HKEY_CURRENT_USER, kCleanupRegistryKey, KEY_SET_VALUE)
+          .WriteValue(kCleanupRegistryValue, L"0"),
+      ERROR_SUCCESS);
+  EXPECT_FALSE(TestConfiguration(L"spam.exe").should_delete_extracted_files());
+}
+
+TEST_F(MiniInstallerConfigurationTest, DeleteExtractedFilesBogusValues) {
+  ASSERT_EQ(
+      base::win::RegKey(HKEY_CURRENT_USER, kCleanupRegistryKey, KEY_SET_VALUE)
+          .WriteValue(kCleanupRegistryValue, L""),
+      ERROR_SUCCESS);
+  EXPECT_TRUE(TestConfiguration(L"spam.exe").should_delete_extracted_files());
+  ASSERT_EQ(
+      base::win::RegKey(HKEY_CURRENT_USER, kCleanupRegistryKey, KEY_SET_VALUE)
+          .WriteValue(kCleanupRegistryValue, L"1"),
+      ERROR_SUCCESS);
+  EXPECT_TRUE(TestConfiguration(L"spam.exe").should_delete_extracted_files());
+  ASSERT_EQ(
+      base::win::RegKey(HKEY_CURRENT_USER, kCleanupRegistryKey, KEY_SET_VALUE)
+          .WriteValue(kCleanupRegistryValue, L"hello"),
+      ERROR_SUCCESS);
+  EXPECT_TRUE(TestConfiguration(L"spam.exe").should_delete_extracted_files());
+}
+
 }  // namespace mini_installer

chrome/installer/mini_installer/mini_installer.cc

@@ -812,24 +812,6 @@ bool ProcessNonInstallOperations(const Configuration& configuration,
   }
 }
 
-// Returns true if we should delete the temp files we create (default).
-// Returns false iff the user has manually created a ChromeInstallerCleanup
-// string value in the registry under HKCU\\Software\\[Google|Chromium]
-// and set its value to "0".  That explicitly forbids the mini installer from
-// deleting these files.
-// Support for this has been publicly mentioned in troubleshooting tips so
-// we continue to support it.
-bool ShouldDeleteExtractedFiles() {
-  wchar_t value[2] = {0};
-  if (RegKey::ReadSZValue(HKEY_CURRENT_USER, kCleanupRegistryKey,
-                          kCleanupRegistryValue, value, _countof(value)) &&
-      value[0] == L'0') {
-    return false;
-  }
-
-  return true;
-}
-
 ProcessExitResult WMain(HMODULE module) {
   // Always start with deleting potential leftovers from previous installations.
   // This can make the difference between success and failure.  We've seen
@@ -881,7 +863,7 @@ ProcessExitResult WMain(HMODULE module) {
   if (exit_code.IsSuccess())
     exit_code = RunSetup(configuration, archive_path.get(), setup_path.get());
 
-  if (ShouldDeleteExtractedFiles())
+  if (configuration.should_delete_extracted_files())
     DeleteExtractedFiles(base_path.get(), archive_path.get(), setup_path.get());
 
 #if BUILDFLAG(GOOGLE_CHROME_BRANDING)

chrome/installer/mini_installer/mini_installer_constants.cc

@@ -66,8 +66,7 @@ const wchar_t kClientsKeyBase[] = L"Software\\Google\\Update\\Clients\\";
 const wchar_t kClientStateKeyBase[] =
     L"Software\\Google\\Update\\ClientState\\";
 // The path to the key in which kCleanupRegistryValue is found.
-const wchar_t kCleanupRegistryKey[] =
-    L"Software\\Microsoft\\Windows\\CurrentVersion\\Uninstall\\Google Chrome";
+const wchar_t kCleanupRegistryKey[] = L"Software\\Google";
 #else
 // The path to the key containing each app's Clients registry key.
 // No trailing slash on this one because the app's GUID is not appended.