Create Service with new name if creation with original name fails.

We create the service with a new name if the creation with the original
name failed. I think this should take care of failures, and may perhaps
fix issue 1023080 as well. The idea here is that in some cases, a
deletion of the original service may not be complete and there may still
has some stale state left over in the SCM database. In such cases, we
may be unable to open it or recreate it. In these cases, we now will
create a new name and install a new service.

Bug: 1055470
Change-Id: I7e0f5e76892fe29fa597c77cd4b64702a3f717c1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2071387Reviewed-by: Greg Thompson <grt@chromium.org>
Commit-Queue: S. Ganesh <ganesh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#746101}

chrome/installer/util/install_service_work_item_impl.cc

@@ -145,20 +145,20 @@ bool InstallServiceWorkItemImpl::DoImpl() {
     const bool succeeded = InstallNewService();
     if (succeeded) {
       RecordServiceInstallResult(ServiceInstallResult::kSucceededFreshInstall);
-    } else {
-      PLOG(ERROR) << "Failed to install service";
-      RecordServiceInstallResult(ServiceInstallResult::kFailedFreshInstall);
-      RecordWin32ApiErrorCode(kCreateService);
+      return succeeded;
     }
 
-    return succeeded;
-  }
-
-  // It is preferable to do a lightweight upgrade of the existing service,
-  // instead of deleting and recreating a new service, since it is less
-  // likely to fail. Less intrusive to the SCM and to AV/Anti-malware programs.
-  if (UpgradeService())
+    PLOG(ERROR) << "Failed to install service";
+    RecordServiceInstallResult(ServiceInstallResult::kFailedFreshInstall);
+    RecordWin32ApiErrorCode(kCreateService);
+    // Fall through to try installing the service by generating a new name.
+  } else if (UpgradeService()) {
+    // It is preferable to do a lightweight upgrade of the existing service,
+    // instead of deleting and recreating a new service, since it is less
+    // likely to fail. Less intrusive to the SCM and to AV/Anti-malware
+    // programs.
     return true;
+  }
 
   // Save the original service name. Then create a new service name so as to not
   // conflict with the previous one to be safe, then install the new service.
@@ -335,15 +335,12 @@ bool InstallServiceWorkItemImpl::SetServiceName(
     const base::string16& service_name) const {
   base::win::RegKey key;
 
-  // This assumes that a WorkItem to create the key has already executed before
-  // this WorkItem. this is generally true since one is added in
-  // AddUninstallShortcutWorkItems.
-  auto result = key.Open(HKEY_LOCAL_MACHINE,
-                         install_static::GetClientStateKeyPath().c_str(),
-                         KEY_SET_VALUE | KEY_WOW64_32KEY);
+  auto result = key.Create(HKEY_LOCAL_MACHINE,
+                           install_static::GetClientStateKeyPath().c_str(),
+                           KEY_SET_VALUE | KEY_WOW64_32KEY);
   if (result != ERROR_SUCCESS) {
     ::SetLastError(result);
-    DPLOG(ERROR) << "key.Open failed";
+    DPLOG(ERROR) << "key.Create failed";
     return false;
   }
 

chrome/installer/util/install_service_work_item_unittest.cc

@@ -65,7 +65,7 @@ TEST_F(InstallServiceWorkItemTest, Do_MultiSzToVector) {
 }
 
 // This test is flaky, see https://crbug.com/1055470
-TEST_F(InstallServiceWorkItemTest, DISABLED_Do_FreshInstall) {
+TEST_F(InstallServiceWorkItemTest, Do_FreshInstall) {
   auto item = std::make_unique<InstallServiceWorkItem>(
       kServiceName, kServiceDisplayName,
       base::CommandLine(base::FilePath(kServiceProgramPath)));
@@ -79,7 +79,7 @@ TEST_F(InstallServiceWorkItemTest, DISABLED_Do_FreshInstall) {
 }
 
 // This test is flaky, see https://crbug.com/1055470
-TEST_F(InstallServiceWorkItemTest, DISABLED_Do_FreshInstallThenDeleteService) {
+TEST_F(InstallServiceWorkItemTest, Do_FreshInstallThenDeleteService) {
   auto item = std::make_unique<InstallServiceWorkItem>(
       kServiceName, kServiceDisplayName,
       base::CommandLine(base::FilePath(kServiceProgramPath)));
@@ -92,7 +92,7 @@ TEST_F(InstallServiceWorkItemTest, DISABLED_Do_FreshInstallThenDeleteService) {
 }
 
 // This test is flaky, see https://crbug.com/1055470
-TEST_F(InstallServiceWorkItemTest, DISABLED_Do_UpgradeNoChanges) {
+TEST_F(InstallServiceWorkItemTest, Do_UpgradeNoChanges) {
   auto item = std::make_unique<InstallServiceWorkItem>(
       kServiceName, kServiceDisplayName,
       base::CommandLine(base::FilePath(kServiceProgramPath)));
@@ -113,7 +113,7 @@ TEST_F(InstallServiceWorkItemTest, DISABLED_Do_UpgradeNoChanges) {
 }
 
 // This test is flaky, see https://crbug.com/1055470
-TEST_F(InstallServiceWorkItemTest, DISABLED_Do_UpgradeChangedCmdLine) {
+TEST_F(InstallServiceWorkItemTest, Do_UpgradeChangedCmdLine) {
   auto item = std::make_unique<InstallServiceWorkItem>(
       kServiceName, kServiceDisplayName,
       base::CommandLine(base::FilePath(kServiceProgramPath)));