Copy setup when quick-enabling app host to user-level from system-level.

chrome/installer/setup/uninstall.cc

Index: chrome/installer/setup/uninstall.cc
diff --git a/chrome/installer/setup/uninstall.cc b/chrome/installer/setup/uninstall.cc
index a8459b99bd412c31140fb274cd82a380c54a571a..82121f96cc28f7a11329bb9fa047d3baec27be47 100644
--- a/chrome/installer/setup/uninstall.cc
+++ b/chrome/installer/setup/uninstall.cc
@@ -181,6 +181,91 @@ void ClearRlzProductState() {
   }
 }
 
+// Decides whether setup.exe and the installer archive should be removed based
+// on the original and installer states:
+// * non-multi product being uninstalled: remove both
+// * any multi product left besides App Host: keep both
+// * only App Host left: keep setup.exe
+void CheckShouldRemoveSetupAndArchive(
+    const installer::InstallationState& original_state,
+    const installer::InstallerState& installer_state,
+    bool* remove_setup,
+    bool* remove_archive) {
+  *remove_setup = true;
+  *remove_archive = true;
+
+  // If any multi-install product is left (other than App Host) we must leave
+  // the installer and archive. For the App Host, we only leave the installer.
+  if (!installer_state.is_multi_install()) {
+    VLOG(1) << "Removing all installer files for a non-multi installation.";
+  } else {
+    for (size_t i = 0; i < BrowserDistribution::NUM_TYPES; ++i) {

[gab, 2012/11/21 22:55:55]

Could add (*remove_setup || *remove_archive) as once they are both false, they
are is no point to keep looping.

[erikwright (departed), 2012/11/23 18:54:13]

Redundant with the return on 215.

+      BrowserDistribution::Type dist_type =
+          static_cast<BrowserDistribution::Type>(i);

[gab, 2012/11/21 22:55:55]

I'm not a big fan of looping over all the potential products this way; I thought
we had a way to get all currently installed products already (I remember seeing
variables like vector<Product> products; which I'm guessing was obtained from
some method we might be able to re-use here?).

[erikwright (departed), 2012/11/23 18:54:13]

Product is from InstallerState.

See the InstallationState API - it has no such method, though one could be
added.

+      const installer::ProductState* product_state =
+          original_state.GetProductState(
+              installer_state.system_install(), dist_type);
+      if (product_state && product_state->is_multi_install() &&
+          !installer_state.FindProduct(dist_type)) {
+        *remove_setup = false;
+        if (dist_type != BrowserDistribution::CHROME_APP_HOST) {
+          VLOG(1) << "Keeping all installer files due to a remaining "
+                  << "multi-install product.";
+          *remove_archive = false;
+          return;
+        }
+        VLOG(1) << "Keeping setup.exe due to a remaining "

[gab, 2012/11/21 22:55:55]

Put the log beside *remove_setup = false; to keep logging and action together.

This will need to be in an 
if(*remove_setup) {
  VLOG(...);
  *remove_setup = false;
}
block unless you don't care about logging this multiple times.

[erikwright (departed), 2012/11/23 18:54:13]

The thing is, it's only true if the condition on 211 is not met. I could put it
in an 'else' block to be more explicit, but that's redundant with the return on
215.

[gab, 2012/11/30 15:19:45]

Ah ok I see, the logic is essentially:

         if (dist_type != BrowserDistribution::CHROME_APP_HOST) {
           VLOG(1) << "Keeping all installer files due to a remaining "
                   << "multi-install product.";
           *remove_setup = false;
           *remove_archive = false;
           return;
         }
         *remove_setup = false;
         VLOG(1) << "Keeping setup.exe due to a remaining "

but you put *remove_setup = false at the top to avoid the duplication... I think
it's slightly harder to read. What about adding a comment like:
// Do not remove setup.exe as there is a remaining multi-install product.
above *remove_setup = false;

+                << "app-host installation.";
+      }
+    }
+    VLOG(1) << "Removing the installer archive.";
+    if (remove_setup)
+      VLOG(1) << "Removing setup.exe.";
+  }
+}
+
+// Determines the version that this uninstall operation is operating on.
+Version GetUninstallVersion(
+    const installer::InstallationState& original_state,
+    bool system_level) {
+  // Find a product version - any product should do.
+  for (size_t i = 0; i < BrowserDistribution::NUM_TYPES; ++i) {
+    const installer::ProductState* product_state =
+        original_state.GetProductState(
+            system_level, static_cast<BrowserDistribution::Type>(i));

[gab, 2012/11/21 22:55:55]

Same comment about getting installed products, if one doesn't exist, a common
method would be nice for this.

[erikwright (departed), 2012/11/23 18:54:13]

Removed this instance.

+    if (product_state)
+      return product_state->version();
+  }
+  NOTREACHED();
+  return Version();

[gab, 2012/11/21 22:55:55]

How about:

Version version;
for (...) {
   ...
   if (product_state) {
     version = ...
     break;
  }
}
DCHECK(version.IsValid());
return version;


One return and no need for NOTREACHED() which feels weird in, what looks at
first sight like, the main return statement for the method.

[erikwright (departed), 2012/11/23 18:54:13]

Removed this.

+}
+
+// Removes all files from the installer directory, leaving setup.exe iff

[gab, 2012/11/21 22:55:55]

s/installer/Installer

[erikwright (departed), 2012/11/23 18:54:13]

IMHO this is a generic, not the specific name 'Installer'. The fact that it has
that name is not relevant here.

+// |remove_setup| is true.

[grt (UTC plus 2), 2012/11/21 20:40:26]

true -> false?

[erikwright (departed), 2012/11/23 18:54:13]

Done.

+// Returns false in case of an error.
+bool RemoveInstallerFiles(const FilePath& install_directory,
+                          bool remove_setup) {
+  using file_util::FileEnumerator;
+  FileEnumerator file_enumerator(
+      install_directory,
+      false,
+      FileEnumerator::FILES | FileEnumerator::DIRECTORIES);
+  while (true) {
+    FilePath to_delete(file_enumerator.Next());
+    if (to_delete.empty())
+      break;
+    if (!remove_setup && to_delete.BaseName().value() == installer::kSetupExe)

[gab, 2012/11/21 22:55:55]

I prefer comparing FilePaths to string16s, i.e.
to_delete.BaseName() == FilePath(installer::kSetupExe);
as arguably FilePath should know on Windows to compare case-insensitively (I
don't think it does now, but it could/should).

[erikwright (departed), 2012/11/23 18:54:13]

That's awesome.

+      continue;
+
+    VLOG(1) << "Deleting install path " << to_delete.value();
+    if (!file_util::Delete(to_delete, true)) {
+      LOG(ERROR) << "Failed to delete path: " << to_delete.value();
+      return false;

[gab, 2012/11/21 22:55:55]

Is an early exit intended here? We usually operate on a best-effort basis (i.e.
we should return false at the end, but still try to delete the other files
first).

+    }
+  }
+  return true;
+}
+
 }  // namespace
 
 namespace installer {
@@ -471,8 +556,6 @@ DeleteResult DeleteAppHostFilesAndFolders(const InstallerState& installer_state,
   if (!file_util::Delete(app_host_exe, false)) {
     result = DELETE_FAILED;
     LOG(ERROR) << "Failed to delete path: " << app_host_exe.value();
-  } else {
-    result = DeleteApplicationProductAndVendorDirectories(target_path);
   }
 
   return result;
@@ -492,14 +575,22 @@ DeleteResult DeleteChromeFilesAndFolders(const InstallerState& installer_state,
   DeleteResult result = DELETE_SUCCEEDED;
 
   using file_util::FileEnumerator;
-  FileEnumerator file_enumerator(target_path, false,
-      FileEnumerator::FILES | FileEnumerator::DIRECTORIES);
+  FileEnumerator file_enumerator(
+      target_path, true, FileEnumerator::FILES | FileEnumerator::DIRECTORIES);

[grt (UTC plus 2), 2012/11/21 20:40:26]

i find this change impenetrable. could you explain the change to make this
recursive and the extra logic on line 589?

[erikwright (departed), 2012/11/21 21:51:10]

Previously, we went through the top-level entries and deleted them (except
app_host.exe).

Now we want to leave the Installer directory for later handling. So we will
recursively enumerate files and folders. It's breadth-first, so if a directory
is deleted we don't review its individual contents. But if the directory is not
deleted we will later review its individual contents.

So:

C:/Users/erikwright/AppData/Local/Google/Chrome
  Application/
    chrome.exe
    app_host.exe
    25.0.1000.0/
      locales/
        ...
      chrome.dll
      Installer/
        setup.exe
        chrome.7z

We delete chrome.exe, leave app_host.exe, and leave 25.0.1000.0 (because it's a
parent of installer_directory).

We delete locales, chrome.dll, and leave Installer (because it's
installer_directory).

We leave setup.exe and chrome.7z (because they are children of
installer_directory).

+
+  const FilePath installer_directory =
+      installer_state.GetInstallerDirectory(installed_version);
   while (true) {
     FilePath to_delete(file_enumerator.Next());
     if (to_delete.empty())
       break;
     if (to_delete.BaseName().value() == installer::kChromeAppHostExe)
       continue;
+    if (to_delete == installer_directory ||
+        installer_directory.IsParent(to_delete) ||
+        to_delete.IsParent(installer_directory)) {
+      continue;
+    }
 
     VLOG(1) << "Deleting install path " << to_delete.value();
     if (!file_util::Delete(to_delete, true)) {
@@ -529,17 +620,6 @@ DeleteResult DeleteChromeFilesAndFolders(const InstallerState& installer_state,
     }
   }
 
-  if (result == DELETE_REQUIRES_REBOOT) {
-    // Delete the Application directory at reboot if empty.
-    ScheduleFileSystemEntityForDeletion(target_path.value().c_str());
-
-    // If we need a reboot to continue, schedule the parent directories for
-    // deletion unconditionally. If they are not empty, the session manager
-    // will not delete them on reboot.
-    ScheduleParentAndGrandparentForDeletion(target_path);
-  } else {
-    result = DeleteApplicationProductAndVendorDirectories(target_path);
-  }
   return result;
 }
 
@@ -1234,31 +1314,22 @@ InstallStatus UninstallProduct(const InstallationState& original_state,
   GetLocalStateFolders(product, &local_state_folders);
   FilePath backup_state_file(BackupLocalStateFile(local_state_folders));
 
-  DeleteResult delete_result = DELETE_SUCCEEDED;
-
   if (product.is_chrome_app_host()) {
     DeleteAppHostFilesAndFolders(installer_state, product_state->version());
   } else if (!installer_state.is_multi_install() ||
              product.is_chrome_binaries()) {
-
-    // In order to be able to remove the folder in which we're running, we
-    // need to move setup.exe out of the install folder.
-    // TODO(tommi): What if the temp folder is on a different volume?
-    MoveSetupOutOfInstallFolder(installer_state, setup_path,
-                                product_state->version());
-    delete_result = DeleteChromeFilesAndFolders(installer_state,
-                                                product_state->version());
+    DeleteResult delete_result = DeleteChromeFilesAndFolders(
+        installer_state, product_state->version());
+    if (delete_result == DELETE_FAILED) {
+      ret = installer::UNINSTALL_FAILED;
+    } else if (delete_result == DELETE_REQUIRES_REBOOT) {
+      ret = installer::UNINSTALL_REQUIRES_REBOOT;
+    }
   }
 
   if (delete_profile)
     DeleteLocalState(local_state_folders, product.is_chrome_frame());
 
-  if (delete_result == DELETE_FAILED) {
-    ret = installer::UNINSTALL_FAILED;
-  } else if (delete_result == DELETE_REQUIRES_REBOOT) {
-    ret = installer::UNINSTALL_REQUIRES_REBOOT;
-  }
-
   if (!force_uninstall) {
     VLOG(1) << "Uninstallation complete. Launching post-uninstall operations.";
     browser_dist->DoPostUninstallOperations(product_state->version(),
@@ -1273,4 +1344,85 @@ InstallStatus UninstallProduct(const InstallationState& original_state,
   return ret;
 }
 
+installer::InstallStatus CleanUpInstallationDirectoryAfterUninstall(
+    const InstallationState& original_state,
+    const InstallerState& installer_state,
+    const CommandLine& cmd_line,
+    installer::InstallStatus uninstall_status) {
+  if (uninstall_status != installer::UNINSTALL_SUCCESSFUL &&
+      uninstall_status != installer::UNINSTALL_REQUIRES_REBOOT) {
+    return uninstall_status;

[gab, 2012/11/21 22:55:55]

Is the goal on UNINSTALL_FAILED to halt the uninstall? I'm not convinced this
was the previous behavior, i.e. should we instead make a best effort at deleting
the files anyways, but simply make sure to keep |uninstall_status| in its
failure state?

[erikwright (departed), 2012/11/23 18:54:13]

Previously, if we failed to delete a file in DeleteChromeFilesAndFolders, we
would halt enumeration of other files (including the installer).

I could go either way, but one advantage to keeping setup.exe is that the user
could theoretically run it again to retry the uninstallation and hopefully
succeed next time.

+  }
+
+  bool remove_setup = true;
+  bool remove_archive = true;
+  CheckShouldRemoveSetupAndArchive(original_state, installer_state,
+                                   &remove_setup, &remove_archive);
+  if (!remove_archive)
+    return uninstall_status;
+
+  Version uninstall_version(

[grt (UTC plus 2), 2012/11/21 20:40:26]

it seems safer to me to get the path to setup.exe with:
  if (!PathService::Get(base::FILE_EXE, &setup_path)) {
    NOTREACHED();
    return installer::UNINSTALL_FAILED;
  }
and the install_directory with:
  FilePath install_directory(setup_path.DirName());
since that doesn't rely on "pv" in the registry existing and pointing to the
current setup.exe.

[erikwright (departed), 2012/11/21 21:51:10]

Done naively, that actually seems more dangerous to me. If setup.exe is executed
from some other location we would potentially wreak havoc.

Option could be to only bother with this cleanup step if the current .exe is
inside target_path. And if it is, then assume that its parent directory is the
install directory.

This would be mirrored in DeleteChromeFilesAndFolders (the directory to protect
would be the parent directory of the current .exe, if it is a child of
target_path).

SGTY?

+      GetUninstallVersion(original_state, installer_state.system_install()));
+  if (!uninstall_version.IsValid())
+    return installer::UNINSTALL_FAILED;
+
+  if (remove_setup) {
+    // In order to be able to remove the folder in which we're running, we
+    // need to move setup.exe out of the install folder.
+    // TODO(tommi): What if the temp folder is on a different volume?
+    MoveSetupOutOfInstallFolder(installer_state,
+                                cmd_line.GetProgram(),
+                                uninstall_version);
+  }
+
+  FilePath install_directory(
+      installer_state.GetInstallerDirectory(uninstall_version));
+  if (!RemoveInstallerFiles(install_directory, remove_setup))
+    return installer::UNINSTALL_FAILED;
+
+  if (remove_setup) {
+    const FilePath& target_path = installer_state.target_path();
+    if (target_path.empty()) {
+      LOG(ERROR) << "No installation destination path.";
+      return installer::UNINSTALL_FAILED;
+    }
+
+    if (DELETE_SUCCEEDED != DeleteEmptyDir(install_directory))

[grt (UTC plus 2), 2012/11/21 20:40:26]

i think chromium style says to swap these

[erikwright (departed), 2012/11/23 18:54:13]

Done.

+      return installer::UNINSTALL_FAILED;
+    FilePath to_delete = install_directory.DirName();
+    while (to_delete != target_path) {
+      DeleteResult delete_result = DeleteEmptyDir(to_delete);
+      if (delete_result == DELETE_SUCCEEDED) {
+        to_delete = to_delete.DirName();
+        continue;
+      }
+      if (delete_result == DELETE_NOT_EMPTY &&

[gab, 2012/11/21 22:55:55]

else if?

This doesn't actually change the flow, but it feels more readable to me.

+          uninstall_status == installer::UNINSTALL_REQUIRES_REBOOT) {
+        break;
+      }
+      return installer::UNINSTALL_FAILED;
+
+    }
+
+
+    if (uninstall_status == installer::UNINSTALL_REQUIRES_REBOOT) {
+      // Delete the Application directory at reboot if empty.
+      ScheduleFileSystemEntityForDeletion(target_path.value().c_str());
+
+      // If we need a reboot to continue, schedule the parent directories for
+      // deletion unconditionally. If they are not empty, the session manager
+      // will not delete them on reboot.
+      ScheduleParentAndGrandparentForDeletion(target_path);
+      return installer::UNINSTALL_REQUIRES_REBOOT;
+    }
+
+    if (installer::DELETE_FAILED ==

[grt (UTC plus 2), 2012/11/21 20:40:26]

swap here, too

[erikwright (departed), 2012/11/23 18:54:13]

Done.

+        DeleteApplicationProductAndVendorDirectories(target_path)) {
+      return installer::UNINSTALL_FAILED;
+    }
+  }
+
+  return installer::UNINSTALL_SUCCESSFUL;
+}
+
 }  // namespace installer