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

chrome/installer/setup/install.cc

Index: chrome/installer/setup/install.cc
diff --git a/chrome/installer/setup/install.cc b/chrome/installer/setup/install.cc
index 588550c6a3e31f03b55764a8ba2db6a962ff5409..ce8ddcf519644e3c1d8552099ca392f58d922a8e 100644
--- a/chrome/installer/setup/install.cc
+++ b/chrome/installer/setup/install.cc
@@ -11,11 +11,13 @@
 #include <memory>
 #include <string>
 
+#include "base/command_line.h"
 #include "base/files/file_enumerator.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/logging.h"
 #include "base/numerics/safe_conversions.h"
+#include "base/process/launch.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
@@ -680,15 +682,33 @@ InstallStatus InstallOrUpdateProduct(
       }
     }
 
+    // Delete files that belong to old versions of Chrome. If that fails during
+    // a not-in-use update, launch a --delete-old-version process. If this is an
+    // in-use update, a --delete-old-versions process will be launched when
+    // executables are renamed.
     installer_state.SetStage(REMOVING_OLD_VERSIONS);
-    // TODO(fdoray): Launch a cleanup process when this fails during a not-in-
-    // use update. crbug.com/451546
-    DeleteOldVersions(installer_state.target_path());
+    if (!DeleteOldVersions(installer_state.target_path()) &&
+        !base::PathExists(

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

rather than checking for new_chrome.exe again, you could check for the result
codes that represent that state:
(result == IN_USE_UPDATED || result == IN_USE_DOWNGRADE)

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

Done.

+            installer_state.target_path().Append(installer::kChromeNewExe))) {
+      LaunchDeleteOldVersionsProcess(installer_state);
+    }
   }
 
   return result;
 }
 
+void LaunchDeleteOldVersionsProcess(const InstallerState& installer_state) {
+  base::LaunchOptions launch_options;
+  launch_options.start_hidden = true;

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

i think it's prudent to use:
  launch_options.force_breakaway_from_job_ = true;
so that anything running the installer in a job object (msiexec may do this)
won't wait around for the deleter process to complete.

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

Done.

+  // Make sure not to launch from a version directory. Otherwise, it won't be
+  // possible to delete it.
+  launch_options.current_directory = installer_state.target_path();
+  base::CommandLine command_line(base::MakeAbsoluteFilePath(
+      base::CommandLine::ForCurrentProcess()->GetProgram()));

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

the currently running setup.exe during an update is in a temp dir created by
mini_installer.exe somewheres, so it isn't the right choice. since different
callers of this function know which setup.exe to use (InstallOrUpdateProduct
knows where the new one is, and the rename dance knows that the currently
running one is correct), how about changing this fn to take a FilePath to the
setup.exe that should do the work? InstallOrUpdateProduct should formulate one
with:

installer_state.GetInstallerDirectory(new_version).Append(setup_path.BaseName())

and the rename dance could have it passed down from HandleNonInstallBlahBlah
which already has the value from the path service.

for future reference, note that
base::CommandLine::ForCurrentProcess()->GetProgram() may not accurately name the
current executable. using the PathService to get FILE_EXE is preferable.

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

Done.

+  command_line.AppendSwitch(switches::kDeleteOldVersions);

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

we generally try to use the command-line args that specify which product is
being operated on, at which level, and propagate verbose logging. to get the
right product flags, it's probably easiest for this function to suss out whether
the binaries are being updated or just chrome (either single-install or canary):

  // Deleting old versions is relevant if multi-install binaries are being
  // updated or if single-install Chrome is.
  const Product* product =
    installer_state.FindProduct(BrowserDistribution::CHROME_BINARIES);
  if (!product)
    product = installer_state.FindProduct(BrowserDistribution::CHROME_BROWSER);
  if (!product)
    return;

from there, you can ask the |product| to add its special switches (i.e.,
--chrome-sxs or --multi-install) to command_line:

  product->AppendProductFlags(&command_line);

and then add in --system-level and --verbose-logging (which could be conditional
on installer_state.verbose_logging(), but i suggest always having it on for
starters):

  if (installer_state.system_install())
    command_line.AppendSwitch(switches::kSystemLevel);
  // Unconditionally enable verbose logging for now to make diagnosing potential
  // failures possible.
  command_line.AppendSwitch(switches::kVerboseLogging);

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

Done.

+  base::LaunchProcess(command_line, launch_options);

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

logging around this would be useful. maybe:
  VLOG(1) << "Launching \"" << command_line.GetCommandLineString() << "\" to
delete old versions.";
  base::Process process = base::LaunchProcess(command_line, launch_options);
  PLOG_IF(ERROR, !process)
      << "Failed to launch \""<< command_line.GetCommandLineString() << "\"";

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

Done.

+}
+
 void HandleOsUpgradeForBrowser(const installer::InstallerState& installer_state,
                                const installer::Product& chrome,
                                const base::Version& installed_version) {