Improve reg cleanup in InstallServiceWorkItemTest and run it serially.

The test was leaking an empty key on TearDown. With this change, it
deletes the key if it was created in SetUp and also deletes an empty key
left behind by previous test runs.

Additinally, tests from this harness must run serially since they all
interact with the ServiceControlManager and centralized machine
state. As such, build and run them into setup_unittests rather than
installer_util_unittests.

This change also introduces a use of ScopedInstallDetails to configure
state for system-level operations (the only case in which the code under
test will run).

BUG=1059314

Change-Id: I6f3a35cda45704e9195dfae25a90401fb8915c6f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2095540
Auto-Submit: Greg Thompson <grt@chromium.org>
Reviewed-by: S. Ganesh <ganesh@chromium.org>
Commit-Queue: S. Ganesh <ganesh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#748804}

chrome/installer/setup/BUILD.gn

@@ -148,6 +148,7 @@ if (is_win) {
       "//chrome/install_static:install_static_util",
       "//chrome/install_static/test:test_support",
       "//chrome/installer/mini_installer:unit_tests",
+      "//chrome/installer/util:serial_unittests",
       "//chrome/installer/util:test_support",
       "//components/crash/content/app:test_support",
       "//testing/gmock",

chrome/installer/util/BUILD.gn

@@ -278,6 +278,20 @@ if (is_win) {
     ]
   }
 
+  # This source set contains tests that must run serially. These are linked into
+  # setup_unittests, which does so.
+  source_set("serial_unittests") {
+    testonly = true
+    sources = [ "install_service_work_item_unittest.cc" ]
+    deps = [
+      ":with_no_strings",
+      "//base",
+      "//chrome/install_static:install_static_util",
+      "//chrome/install_static/test:test_support",
+      "//testing/gtest",
+    ]
+  }
+
   test("installer_util_unittests") {
     sources = [
       "advanced_firewall_manager_win_unittest.cc",
@@ -298,7 +312,6 @@ if (is_win) {
       "experiment_storage_unittest.cc",
       "experiment_unittest.cc",
       "google_update_settings_unittest.cc",
-      "install_service_work_item_unittest.cc",
       "install_util_unittest.cc",
       "l10n_string_util_unittest.cc",
       "logging_installer_unittest.cc",

chrome/installer/util/install_service_work_item_unittest.cc

@@ -14,6 +14,7 @@
 #include "base/strings/stringprintf.h"
 #include "base/win/registry.h"
 #include "chrome/install_static/install_util.h"
+#include "chrome/install_static/test/scoped_install_details.h"
 #include "chrome/installer/util/work_item.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -44,31 +45,34 @@ class InstallServiceWorkItemTest : public ::testing::Test {
   }
 
   void SetUp() override {
-    base::win::RegKey key;
-    if (ERROR_SUCCESS ==
-        key.Open(HKEY_LOCAL_MACHINE,
-                 install_static::GetClientStateKeyPath().c_str(),
-                 KEY_READ | KEY_WOW64_32KEY)) {
-      preexisting_clientstate_key_ = true;
-    } else {
-      ASSERT_EQ(ERROR_SUCCESS,
-                key.Create(HKEY_LOCAL_MACHINE,
-                           install_static::GetClientStateKeyPath().c_str(),
-                           KEY_READ | KEY_WOW64_32KEY));
-    }
+    DWORD disposition = 0;
+    ASSERT_EQ(
+        base::win::RegKey().CreateWithDisposition(
+            HKEY_LOCAL_MACHINE, install_static::GetClientStateKeyPath().c_str(),
+            &disposition, KEY_READ | KEY_WOW64_32KEY),
+        ERROR_SUCCESS);
+    preexisting_clientstate_key_ = (disposition == REG_OPENED_EXISTING_KEY);
   }
 
   void TearDown() override {
-    if (!preexisting_clientstate_key_) {
-      base::win::RegKey key;
-      if (key.Open(HKEY_LOCAL_MACHINE,
-                   install_static::GetClientStateKeyPath().c_str(),
-                   DELETE | KEY_WOW64_32KEY)) {
-        EXPECT_EQ(ERROR_SUCCESS, key.DeleteKey(L""));
-      }
-    }
+    // Delete the ClientState key created by this test if it is empty. While it
+    // would be ideal to only delete if !preexisting_clientstate_key_, older
+    // variants of this test failed to delete their key during TearDown.
+    auto result =
+        base::win::RegKey(HKEY_LOCAL_MACHINE, L"", KEY_READ | KEY_WOW64_32KEY)
+            .DeleteEmptyKey(install_static::GetClientStateKeyPath().c_str());
+    // Deletion should have succeeded if the key didn't exist to start with. If
+    // the key existed before the test ran, the delete may have succeeded
+    // (because the key was empty to start with) or may have failed because the
+    // key actually has data that should not be removed.
+    if (!preexisting_clientstate_key_)
+      EXPECT_EQ(result, ERROR_SUCCESS);
+    else if (result != ERROR_SUCCESS)
+      EXPECT_EQ(result, ERROR_DIR_NOT_EMPTY);
   }
 
+  // Set up InstallDetails for a system-level install.
+  const install_static::ScopedInstallDetails install_details_{true};
   bool preexisting_clientstate_key_ = false;
 };