Support --delete-old-versions in setup.exe.

chrome/installer/setup/setup_main.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/installer/setup/setup_main.h"
 
 #include <windows.h>
 #include <msi.h>
 #include <shellapi.h>
 #include <shlobj.h>
@@ skipping 38 matching lines @@
 #include "chrome/installer/setup/install.h"
 #include "chrome/installer/setup/install_worker.h"
 #include "chrome/installer/setup/installer_crash_reporting.h"
 #include "chrome/installer/setup/installer_metrics.h"
 #include "chrome/installer/setup/setup_constants.h"
 #include "chrome/installer/setup/setup_singleton.h"
 #include "chrome/installer/setup/setup_util.h"
 #include "chrome/installer/setup/uninstall.h"
 #include "chrome/installer/util/browser_distribution.h"
 #include "chrome/installer/util/delete_after_reboot_helper.h"
+#include "chrome/installer/util/delete_old_versions.h"
 #include "chrome/installer/util/delete_tree_work_item.h"
 #include "chrome/installer/util/google_update_constants.h"
 #include "chrome/installer/util/google_update_settings.h"
 #include "chrome/installer/util/google_update_util.h"
 #include "chrome/installer/util/helper.h"
 #include "chrome/installer/util/html_dialog.h"
 #include "chrome/installer/util/install_util.h"
 #include "chrome/installer/util/installation_state.h"
 #include "chrome/installer/util/installation_validator.h"
 #include "chrome/installer/util/installer_state.h"
@@ skipping 310 matching lines @@
             Product* chrome = installer_state->AddProduct(&multi_chrome);
             VLOG(1) << "Broken install of product to be included for repair: "
                     << chrome->distribution()->GetDisplayName();
           }  // else uninstall the binaries in UninstallBinariesIfUnused.
         }
       }
     }
   }
 }
 
+// Repetitively attempts to delete all files that belong to old versions of
+// Chrome from |install_dir|. Waits 15 seconds before the first attempt and 5
+// minutes after each unsuccessful attempt. Returns when no files that belong to
+// an old version of Chrome remain or when another process tries to acquire the
+// SetupSingleton.
+installer::InstallStatus RepeatDeleteOldVersionsUntilSuccess(

[grt (UTC plus 2), 2016/09/13 08:10:40]

it looks like we'll be blind to how this performs. how difficult would it be to
have persistent histograms written for this case so that you can use UMA in here
to log things like the # of long sleeps needed before termination, and whether
termination was due to success or interrupt (see the call to
EndPersistentHistogramStorage at the end of wWinMain)?

[fdoray, 2016/09/13 21:34:54]

Done.

+    const base::FilePath& install_dir,
+    const installer::SetupSingleton& setup_singleton) {
+  // Wait 15 seconds because trying to delete old files right away is likely to
+  // fail. Indeed, this is called in 2 occasions:
+  // - When the installer fails to delete old files after a not-in-use update:
+  //   retrying immediately is likely to fail again.
+  // - When executables are successfully renamed on Chrome startup or shutdown:
+  //   old files can't be deleted because Chrome is still in use.
+  if (setup_singleton.WaitForInterrupt(base::TimeDelta::FromSeconds(15)))
+    return installer::SETUP_SINGLETON_RELEASED;
+
+  for (;;) {

[grt (UTC plus 2), 2016/09/13 08:10:40]

why not?
  while (true) {

[fdoray, 2016/09/13 21:34:54]

Done.

+    if (installer::DeleteOldVersions(install_dir)) {
+      LOG(INFO) << "Successfully deleted old files from --delete-old-versions "

[grt (UTC plus 2), 2016/09/13 08:10:40]

please use VLOG(1) here and below

[fdoray, 2016/09/13 21:34:54]

Done.

+                   "process.";
+      return installer::DELETE_OLD_VERSIONS_SUCCESS;
+    }
+
+    LOG(INFO) << "Failed to delete old files from --delete-old-versions "

[grt (UTC plus 2), 2016/09/13 08:10:40]

does this case mean that no old files were deleted, or that not all old files
were deleted? if the latter, please change to "Failed to delete all old..."

[fdoray, 2016/09/13 21:34:54]

Done. It means that not *all* old files were deleted. Some files may have been
deleted.

+                 "process. Will try again in 5 minutes.";
+    if (setup_singleton.WaitForInterrupt(base::TimeDelta::FromMinutes(5)))
+      return installer::SETUP_SINGLETON_RELEASED;
+  }
+}
+
 // This function is called when --rename-chrome-exe option is specified on
 // setup.exe command line. This function assumes an in-use update has happened
 // for Chrome so there should be a file called new_chrome.exe on the file
 // system and a key called 'opv' in the registry. This function will move
 // new_chrome.exe to chrome.exe and delete 'opv' key in one atomic operation.
 // This function also deletes elevation policies associated with the old version
 // if they exist.
 installer::InstallStatus RenameChromeExecutables(
     const InstallationState& original_state,
     InstallerState* installer_state) {
@@ skipping 48 matching lines @@
     install_list->AddDeleteRegValueWorkItem(reg_root,
                                             version_key,
                                             KEY_WOW64_32KEY,
                                             google_update::kRegRenameCmdField);
   }
   // old_chrome.exe is still in use in most cases, so ignore failures here.
   install_list->AddDeleteTreeWorkItem(chrome_old_exe, temp_path.path())
       ->set_best_effort(true);
 
   installer::InstallStatus ret = installer::RENAME_SUCCESSFUL;
-  if (!install_list->Do()) {
+  if (install_list->Do()) {
+    installer::LaunchDeleteOldVersionsProcess(*installer_state);

[grt (UTC plus 2), 2016/09/13 08:10:40]

in this specific case, the current setup.exe (PathService::Get(base::FILE_EXE))
is the right one. so perhaps fetch that here and pass it in, or have
HandleNonInstallCmdLineOptions pass it into this fn to be passed along.

[fdoray, 2016/09/13 21:34:54]

Done.

+  } else {
     LOG(ERROR) << "Renaming of executables failed. Rolling back any changes.";
     install_list->Rollback();
     ret = installer::RENAME_FAILED;
   }
   // temp_path's dtor will take care of deleting or scheduling itself for
   // deletion at reboot when this scope closes.
   VLOG(1) << "Deleting temporary directory " << temp_path.path().value();
 
   return ret;
 }
@@ skipping 761 matching lines @@
           status = installer::IN_USE_UPDATED;
       } else {
         if (ShellUtil::RegisterChromeBrowser(chrome_install->distribution(),
             chrome_exe, suffix, false))
           status = installer::IN_USE_UPDATED;
       }
     } else {
       LOG(DFATAL) << "Can't register browser - Chrome distribution not found";
     }
     *exit_code = InstallUtil::GetInstallReturnCode(status);
-  } else if (cmd_line.HasSwitch(installer::switches::kRenameChromeExe)) {
-    // If --rename-chrome-exe is specified, we want to rename the executables
-    // and exit.
+  } else if (cmd_line.HasSwitch(installer::switches::kDeleteOldVersions) ||
+             cmd_line.HasSwitch(installer::switches::kRenameChromeExe)) {
     std::unique_ptr<installer::SetupSingleton> setup_singleton(
         installer::SetupSingleton::Acquire(
             cmd_line, MasterPreferences::ForCurrentProcess(), original_state,
             installer_state));
-    if (!setup_singleton)
+    if (!setup_singleton) {
       *exit_code = installer::SETUP_SINGLETON_ACQUISITION_FAILED;
-    else
+    } else if (cmd_line.HasSwitch(installer::switches::kDeleteOldVersions)) {
+      *exit_code = RepeatDeleteOldVersionsUntilSuccess(
+          installer_state->target_path(), *setup_singleton);
+    } else {
+      DCHECK(cmd_line.HasSwitch(installer::switches::kRenameChromeExe));
       *exit_code = RenameChromeExecutables(*original_state, installer_state);
+    }
   } else if (cmd_line.HasSwitch(
                  installer::switches::kRemoveChromeRegistration)) {
     // This is almost reverse of --register-chrome-browser option above.
     // Here we delete Chrome browser registration. This option should only
     // be used when setup.exe is launched with admin rights. We do not
     // make any user specific changes in this option.
     base::string16 suffix;
     if (cmd_line.HasSwitch(
             installer::switches::kRegisterChromeBrowserSuffix)) {
       suffix = cmd_line.GetSwitchValueNative(
@@ skipping 585 matching lines @@
       return exit_code;
     } else {
       LOG(ERROR) << "Non admin user can not install system level Chrome.";
       installer_state.WriteInstallerResult(installer::INSUFFICIENT_RIGHTS,
           IDS_INSTALL_INSUFFICIENT_RIGHTS_BASE, NULL);
       return installer::INSUFFICIENT_RIGHTS;
     }
   }
 
   std::unique_ptr<installer::SetupSingleton> setup_singleton(
       installer::SetupSingleton::Acquire(cmd_line, prefs, &original_state,

[grt (UTC plus 2), 2016/09/13 08:10:41]

how about UMA for how much time it takes to acquire the singleton so that we
know if long-running deletes are blocking updates (once a delete starts, it runs
to completion, yes?)?

[fdoray, 2016/09/13 21:34:54]

It can't take more than 5+5 seconds = 10 seconds
https://cs.chromium.org/chromium/src/chrome/installer/setup/setup_singleton.c...
and we can already know how often it fails using the installer result written
below. Do we really need another histogram to know exactly how much time we had
to wait?

Note: "once a delete starts, it runs to completion, yes?": NO
When another process tries to acquire the SetupSingleton, it signals an event
https://cs.chromium.org/chromium/src/chrome/installer/setup/setup_singleton.c...
that causes the --delete-old-versions process to exit as soon as it calls
SetupSingleton::WaitForInterrupt().

[grt (UTC plus 2), 2016/09/14 10:42:10]

I forgot about the short timeout. No, I think that's enough.
My question is about once installer::DeleteOldVersions is called. On an old
machine with a slow spinning disk where the process is running at low priority
(it should, no?), how long could it possibly take to delete some directory
trees? Hopefully not long, but once this bit of processing starts it won't stop
due to another process trying to acquire the singleton. Should the process not
run at background CPU and IO priority?

                                          &installer_state));
   if (!setup_singleton) {
     installer_state.WriteInstallerResult(
         installer::SETUP_SINGLETON_ACQUISITION_FAILED,
         IDS_INSTALL_SINGLETON_ACQUISITION_FAILED_BASE, nullptr);
     return installer::SETUP_SINGLETON_ACQUISITION_FAILED;
   }
 
   UninstallMultiChromeFrameIfPresent(cmd_line, prefs,
                                      &original_state, &installer_state);
@@ skipping 44 matching lines @@
     // never invoked directly by Google Update.
     return_code = InstallUtil::GetInstallReturnCode(install_status);
   }
 
   installer::EndPersistentHistogramStorage(installer_state.target_path(),
                                            system_install);
   VLOG(1) << "Installation complete, returning: " << return_code;
 
   return return_code;
 }