Delete old files after an update.

This is a variation of r378802 which was reverted because of
a Dr Memory error (solved by using GenerateAlternatePEFileVersion
instead of GenerateSpecificPEFileVersion). r378802 also caused
problems with shortcuts crbug.com/592040 (solved by not letting
the deletion of old files cause a rollback).

With this CL, the installer deletes all files that belong to old
versions of Chrome that are not in use after an update.

Eventually, the DeleteOldVersions() will be invoked again to delete
old versions that were in use at time of the update.

BUG=451546
TEST=
-- Not-in-use --
1. Run mini_installer.exe --chrome --multi-install --verbose-logging --do-not-launch-chrome
- The install directory should contain chrome.exe and a matching version directory.
- There should be no other executable or version directory in the install directory.
2. Run next_version_mini_installer.exe --multi-install --verbose-logging --do-not-launch-chrome
- The install directory should contain chrome.exe and a matching version directory. The version should be higher than at the previous step.
- There should be no other executable or version directory in the install directory.

-- In-use --
1. Run mini_installer.exe --chrome --multi-install --verbose-logging --do-not-launch-chrome
- The install directory should contain chrome.exe and a matching version directory.
- There should be no other executable or version directory in the install directory.
2. Launch Chrome.
3. Run next_version_mini_installer.exe --multi-install --verbose-logging --do-not-launch-chrome.
- The install directory should contain chrome.exe and a matching version directory + new_chrome.exe and a matching version directory.
- There should be no other executable or version directory in the install directory.

Review-Url: https://codereview.chromium.org/2273113002
Cr-Commit-Position: refs/heads/master@{#414842}

chrome/installer/setup/install.cc

@@ -32,6 +32,7 @@
 #include "chrome/installer/util/browser_distribution.h"
 #include "chrome/installer/util/create_reg_key_work_item.h"
 #include "chrome/installer/util/delete_after_reboot_helper.h"
+#include "chrome/installer/util/delete_old_versions.h"
 #include "chrome/installer/util/google_update_constants.h"
 #include "chrome/installer/util/helper.h"
 #include "chrome/installer/util/install_util.h"
@@ -680,11 +681,9 @@ InstallStatus InstallOrUpdateProduct(
     }
 
     installer_state.SetStage(REMOVING_OLD_VERSIONS);
-
-    installer_state.RemoveOldVersionDirectories(
-        new_version,
-        existing_version.get(),
-        install_temp_path);
+    // TODO(fdoray): Launch a cleanup process when this fails during a not-in-
+    // use update. crbug.com/451546
+    DeleteOldVersions(installer_state.target_path());
   }
 
   return result;

chrome/installer/util/BUILD.gn

@@ -159,6 +159,8 @@ static_library("with_no_strings") {
       "create_dir_work_item.h",
       "create_reg_key_work_item.cc",
       "create_reg_key_work_item.h",
+      "delete_old_versions.cc",
+      "delete_old_versions.h",
       "delete_reg_key_work_item.cc",
       "delete_reg_key_work_item.h",
       "delete_reg_value_work_item.cc",
@@ -292,6 +294,7 @@ if (is_win) {
       "create_dir_work_item_unittest.cc",
       "create_reg_key_work_item_unittest.cc",
       "delete_after_reboot_helper_unittest.cc",
+      "delete_old_versions_unittest.cc",
       "delete_reg_key_work_item_unittest.cc",
       "delete_reg_value_work_item_unittest.cc",
       "delete_tree_work_item_unittest.cc",

chrome/installer/util/delete_old_versions.cc

+// Copyright 2016 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/util/delete_old_versions.h"
+
+#include <map>
+#include <memory>
+#include <set>
+#include <vector>
+
+#include "base/file_version_info.h"
+#include "base/files/file.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/stl_util.h"
+#include "base/version.h"
+#include "chrome/installer/util/util_constants.h"
+
+namespace installer {
+
+namespace {
+
+using PathVector = std::vector<base::FilePath>;
+using DirectorySet = std::set<base::FilePath>;
+using ExecutableMap = std::map<base::FilePath, PathVector>;
+
+// Returns the name of the version directory for executable |exe_path|.
+base::FilePath GetExecutableVersionDirName(const base::FilePath& exe_path) {
+  std::unique_ptr<FileVersionInfo> file_version_info(
+      FileVersionInfo::CreateFileVersionInfo(exe_path));
+  if (!file_version_info.get())
+    return base::FilePath();
+  return base::FilePath(file_version_info->file_version());
+}
+
+// Returns the names of the old version directories found in |install_dir|. The
+// directories named after the version of chrome.exe or new_chrome.exe are
+// excluded.
+DirectorySet GetOldVersionDirectories(const base::FilePath& install_dir) {
+  const base::FilePath new_chrome_exe_version_dir_name =
+      GetExecutableVersionDirName(install_dir.Append(kChromeNewExe));
+  const base::FilePath chrome_exe_version_dir_name =
+      GetExecutableVersionDirName(install_dir.Append(kChromeExe));
+
+  DirectorySet directories;
+  base::FileEnumerator enum_directories(install_dir, false,
+                                        base::FileEnumerator::DIRECTORIES);
+  for (base::FilePath directory_path = enum_directories.Next();
+       !directory_path.empty(); directory_path = enum_directories.Next()) {
+    const base::FilePath directory_name = directory_path.BaseName();
+    const base::Version version(directory_name.AsUTF8Unsafe());
+    const size_t kNumChromeVersionComponents = 4;
+    if (version.IsValid() &&
+        version.components().size() == kNumChromeVersionComponents &&
+        directory_name != new_chrome_exe_version_dir_name &&
+        directory_name != chrome_exe_version_dir_name) {
+      directories.insert(directory_name);
+    }
+  }
+  return directories;
+}
+
+// Returns a map where the keys are version directory names and values are paths
+// of old_chrome*.exe executables found in |install_dir|.
+ExecutableMap GetOldExecutables(const base::FilePath& install_dir) {
+  ExecutableMap executables;
+  base::FileEnumerator enum_executables(install_dir, false,
+                                        base::FileEnumerator::FILES,
+                                        FILE_PATH_LITERAL("old_chrome*.exe"));
+  for (base::FilePath exe_path = enum_executables.Next(); !exe_path.empty();
+       exe_path = enum_executables.Next()) {
+    executables[GetExecutableVersionDirName(exe_path)].push_back(exe_path);
+  }
+  return executables;
+}
+
+// Deletes directories that are in |directories| and don't have a matching
+// executable in |executables|. Returns false if any such directories could not
+// be deleted.
+bool DeleteDirectoriesWithoutMatchingExecutable(
+    const DirectorySet& directories,
+    const ExecutableMap& executables,
+    const base::FilePath& install_dir) {
+  bool success = true;
+  for (const base::FilePath& directory_name : directories) {
+    // Delete the directory if it doesn't have a matching executable.
+    if (!base::ContainsKey(executables, directory_name)) {
+      const base::FilePath directory_path = install_dir.Append(directory_name);
+      LOG(WARNING) << "Attempting to delete stray directory "
+                   << directory_path.value();
+      if (!base::DeleteFile(directory_path, true)) {
+        PLOG(ERROR) << "Failed to delete stray directory "
+                    << directory_path.value();
+        success = false;
+      }
+    }
+  }
+  return success;
+}
+
+// Deletes executables that are in |executables| and don't have a matching
+// directory in |directories|. Returns false if any such files could not be
+// deleted.
+bool DeleteExecutablesWithoutMatchingDirectory(
+    const DirectorySet& directories,
+    const ExecutableMap& executables) {
+  bool success = true;
+  for (const auto& version_and_executables : executables) {
+    const auto& version_dir_name = version_and_executables.first;
+    const auto& executables_for_version = version_and_executables.second;
+
+    // Don't delete the executables if they have a matching directory.
+    if (base::ContainsValue(directories, version_dir_name))
+      continue;
+
+    // Delete executables for version |version_dir_name|.
+    for (const auto& executable_path : executables_for_version) {
+      const base::FilePath executable_name = executable_path.BaseName();
+      LOG(WARNING) << "Attempting to delete stray executable "
+                   << executable_path.value();
+      if (!base::DeleteFile(executable_path, false)) {
+        PLOG(ERROR) << "Failed to delete stray executable "
+                    << executable_path.value();
+        success = false;
+      }
+    }
+  }
+  return success;
+}
+
+// Opens |path| with options that prevent the file from being read or written
+// via another handle. As long as the returned object is alive, it is guaranteed
+// that |path| isn't in use. It can however be deleted.
+base::File GetFileLock(const base::FilePath& path) {
+  return base::File(path, base::File::FLAG_OPEN | base::File::FLAG_READ |
+                              base::File::FLAG_EXCLUSIVE_READ |
+                              base::File::FLAG_EXCLUSIVE_WRITE |
+                              base::File::FLAG_SHARE_DELETE);
+}
+
+// Deletes |version_directory| and all executables in |version_executables| if
+// no .exe or .dll file for the version is in use. Returns false if any file
+// or directory for the version could not be deleted.
+bool DeleteVersion(const base::FilePath& version_directory,
+                   const PathVector& version_executables) {
+  std::vector<base::File> locks;
+  PathVector locked_file_paths;
+
+  // Lock .exe/.dll files in |version_directory|.
+  base::FileEnumerator enum_version_directory(version_directory, true,
+                                              base::FileEnumerator::FILES);
+  for (base::FilePath path = enum_version_directory.Next(); !path.empty();
+       path = enum_version_directory.Next()) {
+    if (!path.MatchesExtension(FILE_PATH_LITERAL(".exe")) &&
+        !path.MatchesExtension(FILE_PATH_LITERAL(".dll"))) {
+      continue;
+    }
+    locks.push_back(GetFileLock(path));
+    if (!locks.back().IsValid()) {
+      LOG(WARNING) << "Failed to delete old version "
+                   << version_directory.value() << " because " << path.value()
+                   << " is in use.";
+      return false;
+    }
+    locked_file_paths.push_back(path);
+  }
+
+  // Lock executables in |version_executables|.
+  for (const base::FilePath& executable_path : version_executables) {
+    locks.push_back(GetFileLock(executable_path));
+    if (!locks.back().IsValid()) {
+      LOG(WARNING) << "Failed to delete old version "
+                   << version_directory.value() << " because "
+                   << executable_path.value() << " is in use.";
+      return false;
+    }
+    locked_file_paths.push_back(executable_path);
+  }
+
+  bool success = true;
+
+  // Delete locked files. The files won't actually be deleted until the locks
+  // are released.
+  for (const base::FilePath& locked_file_path : locked_file_paths) {
+    if (!base::DeleteFile(locked_file_path, false)) {
+      PLOG(ERROR) << "Failed to delete locked file "
+                  << locked_file_path.value();
+      success = false;
+    }
+  }
+
+  // Release the locks, causing the locked files to actually be deleted. The
+  // version directory can't be deleted before this is done.
+  locks.clear();
+
+  // Delete the version directory.
+  if (!base::DeleteFile(version_directory, true)) {
+    PLOG(ERROR) << "Failed to delete version directory "
+                << version_directory.value();
+    success = false;
+  }
+
+  return success;
+}
+
+// For each executable in |executables| that has a matching directory in
+// |directories|, tries to delete the executable and the matching directory. No
+// deletion occurs for a given version if a .exe or .dll file for that version
+// is in use. Returns false if any directory/executables pair could not be
+// deleted.
+bool DeleteMatchingExecutablesAndDirectories(
+    const DirectorySet& directories,
+    const ExecutableMap& executables,
+    const base::FilePath& install_dir) {
+  bool success = true;
+  for (const auto& directory_name : directories) {
+    // Don't delete the version unless the directory has at least one matching
+    // executable.
+    auto version_executables_it = executables.find(directory_name);
+    if (version_executables_it == executables.end())
+      continue;
+
+    // Try to delete all files for the version.
+    success &= DeleteVersion(install_dir.Append(directory_name),
+                             version_executables_it->second);
+  }
+  return success;
+}
+
+}  // namespace
+
+bool DeleteOldVersions(const base::FilePath& install_dir) {
+  const DirectorySet old_directories = GetOldVersionDirectories(install_dir);
+  const ExecutableMap old_executables = GetOldExecutables(install_dir);
+
+  bool success = true;
+  success &= DeleteDirectoriesWithoutMatchingExecutable(
+      old_directories, old_executables, install_dir);
+  success &= DeleteExecutablesWithoutMatchingDirectory(old_directories,
+                                                       old_executables);
+  success &= DeleteMatchingExecutablesAndDirectories(
+      old_directories, old_executables, install_dir);
+
+  return success;
+}
+
+}  // namespace installer

chrome/installer/util/delete_old_versions.h

+// Copyright 2016 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.
+
+#ifndef CHROME_INSTALLER_UTIL_DELETE_OLD_VERSIONS_H_
+#define CHROME_INSTALLER_UTIL_DELETE_OLD_VERSIONS_H_
+
+namespace base {
+class FilePath;
+}
+
+namespace installer {
+
+// Deletes files that belong to old versions of Chrome. chrome.exe,
+// new_chrome.exe and their associated version directories are never deleted.
+// Also, no file is deleted for a given version if a .exe or .dll file for that
+// version is in use. Returns true if no files that belong to an old version of
+// Chrome remain.
+bool DeleteOldVersions(const base::FilePath& install_dir);
+
+}  // namespace installer
+
+#endif  // CHROME_INSTALLER_UTIL_DELETE_OLD_VERSIONS_H_

chrome/installer/util/delete_old_versions_unittest.cc

+// Copyright 2016 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/util/delete_old_versions.h"
+
+#include <set>
+
+#include "base/files/file.h"
+#include "base/files/file_enumerator.h"
+#include "base/files/file_path.h"
+#include "base/files/file_util.h"
+#include "base/files/scoped_temp_dir.h"
+#include "base/logging.h"
+#include "base/macros.h"
+#include "base/path_service.h"
+#include "base/strings/string16.h"
+#include "base/strings/string_util.h"
+#include "base/version.h"
+#include "chrome/installer/test/alternate_version_generator.h"
+#include "chrome/installer/util/util_constants.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+using upgrade_test::Direction;
+
+namespace installer {
+
+namespace {
+
+class DeleteOldVersionsTest : public testing::Test {
+ protected:
+  DeleteOldVersionsTest() = default;
+
+  void SetUp() override { ASSERT_TRUE(install_dir_.CreateUniqueTempDir()); }
+
+  // Creates a copy of the current executable with a distinct version of name
+  // |name| in |install_dir_|. Depending on |direction|, the version of the
+  // created executable is higher or lower than the version of the current
+  // executable.
+  base::string16 CreateExecutable(const base::string16& name,
+                                  Direction direction) {
+    base::FilePath current_exe_path;
+    if (!base::PathService::Get(base::FILE_EXE, &current_exe_path))
+      return base::string16();
+
+    return upgrade_test::GenerateAlternatePEFileVersion(
+        current_exe_path, install_dir().Append(name), direction);
+  }
+
+  // Creates in |install_dir_| a directory named |name| containing a subset of
+  // dummy files impersonating a Chrome version directory.
+  bool CreateVersionDirectory(const base::string16& name) {
+    static constexpr char kDummyContent[] = "dummy";
+    const base::FilePath version_dir_path(install_dir().Append(name));
+
+    return base::CreateDirectory(install_dir().Append(name)) &&
+           base::CreateDirectory(version_dir_path.Append(L"Installer")) &&
+           base::WriteFile(version_dir_path.Append(L"chrome.dll"),
+                           kDummyContent, sizeof(kDummyContent)) &&
+           base::WriteFile(version_dir_path.Append(L"nacl64.exe"),
+                           kDummyContent, sizeof(kDummyContent)) &&
+           base::WriteFile(version_dir_path.Append(L"icudtl.dat"),
+                           kDummyContent, sizeof(kDummyContent)) &&
+           base::WriteFile(version_dir_path.Append(L"Installer\\setup.exe"),
+                           kDummyContent, sizeof(kDummyContent));
+  }
+
+  // Returns the relative paths of all files and directories in |install_dir_|.
+  using FilePathSet = std::set<base::FilePath>;
+  FilePathSet GetInstallDirContent() const {
+    std::set<base::FilePath> content;
+    base::FileEnumerator file_enumerator(
+        install_dir(), true,
+        base::FileEnumerator::FILES | base::FileEnumerator::DIRECTORIES);
+    for (base::FilePath path = file_enumerator.Next(); !path.empty();
+         path = file_enumerator.Next()) {
+      DCHECK(base::StartsWith(path.value(), install_dir().value(),
+                              base::CompareCase::SENSITIVE));
+      content.insert(base::FilePath(
+          path.value().substr(install_dir().value().size() + 1)));
+    }
+    return content;
+  }
+
+  // Adds to |file_path_set| all files and directories that are expected to be
+  // found in the version directory |version| before any attempt to delete it.
+  void AddVersionFiles(const base::string16& version,
+                       FilePathSet* file_path_set) {
+    file_path_set->insert(base::FilePath(version));
+    file_path_set->insert(base::FilePath(version).Append(L"chrome.dll"));
+    file_path_set->insert(base::FilePath(version).Append(L"nacl64.exe"));
+    file_path_set->insert(base::FilePath(version).Append(L"icudtl.dat"));
+    file_path_set->insert(base::FilePath(version).Append(L"Installer"));
+    file_path_set->insert(
+        base::FilePath(version).Append(L"Installer\\setup.exe"));
+  }
+
+  base::FilePath install_dir() const { return install_dir_.path(); }
+
+ private:
+  base::ScopedTempDir install_dir_;
+
+  DISALLOW_COPY_AND_ASSIGN(DeleteOldVersionsTest);
+};
+
+}  // namespace
+
+// An old executable without a matching directory should be deleted.
+TEST_F(DeleteOldVersionsTest, DeleteOldExecutableWithoutMatchingDirectory) {
+  ASSERT_FALSE(
+      CreateExecutable(installer::kChromeOldExe, Direction::PREVIOUS_VERSION)
+          .empty());
+
+  DeleteOldVersions(install_dir());
+  EXPECT_TRUE(GetInstallDirContent().empty());
+}
+
+// chrome.exe and new_chrome.exe should never be deleted.
+TEST_F(DeleteOldVersionsTest, DeleteNewExecutablesWithoutMatchingDirectory) {
+  ASSERT_FALSE(
+      CreateExecutable(installer::kChromeExe, Direction::PREVIOUS_VERSION)
+          .empty());
+  ASSERT_FALSE(
+      CreateExecutable(installer::kChromeNewExe, Direction::NEXT_VERSION)
+          .empty());
+
+  DeleteOldVersions(install_dir());
+  FilePathSet expected_install_dir_content;
+  expected_install_dir_content.insert(base::FilePath(installer::kChromeExe));
+  expected_install_dir_content.insert(base::FilePath(installer::kChromeNewExe));
+  EXPECT_EQ(expected_install_dir_content, GetInstallDirContent());
+}
+
+// A directory without a matching executable should be deleted.
+TEST_F(DeleteOldVersionsTest, DeleteDirectoryWithoutMatchingExecutable) {
+  static constexpr base::char16 kVersion[] = L"48.0.0.0";
+  ASSERT_TRUE(CreateVersionDirectory(kVersion));
+
+  DeleteOldVersions(install_dir());
+  EXPECT_TRUE(GetInstallDirContent().empty());
+}
+
+// A pair of matching old executable/version directory that is not in use should
+// be deleted.
+TEST_F(DeleteOldVersionsTest, DeleteOldExecutableWithMatchingDirectory) {
+  const base::string16 version_a =
+      CreateExecutable(installer::kChromeOldExe, Direction::PREVIOUS_VERSION);
+  ASSERT_FALSE(version_a.empty());
+  ASSERT_TRUE(CreateVersionDirectory(version_a));
+
+  DeleteOldVersions(install_dir());
+  EXPECT_TRUE(GetInstallDirContent().empty());
+}
+
+// chrome.exe, new_chrome.exe and their matching version directories should
+// never be deleted.
+TEST_F(DeleteOldVersionsTest, DeleteNewExecutablesWithMatchingDirectory) {
+  const base::string16 version_a =
+      CreateExecutable(installer::kChromeExe, Direction::PREVIOUS_VERSION);
+  ASSERT_FALSE(version_a.empty());
+  ASSERT_TRUE(CreateVersionDirectory(version_a));
+  const base::string16 version_b =
+      CreateExecutable(installer::kChromeNewExe, Direction::NEXT_VERSION);
+  ASSERT_FALSE(version_b.empty());
+  ASSERT_TRUE(CreateVersionDirectory(version_b));
+
+  DeleteOldVersions(install_dir());
+
+  FilePathSet expected_install_dir_content;
+  expected_install_dir_content.insert(base::FilePath(installer::kChromeExe));
+  AddVersionFiles(version_a, &expected_install_dir_content);
+  expected_install_dir_content.insert(base::FilePath(installer::kChromeNewExe));
+  AddVersionFiles(version_b, &expected_install_dir_content);
+  EXPECT_EQ(expected_install_dir_content, GetInstallDirContent());
+}
+
+// chrome.exe, new_chrome.exe and their matching version directories should
+// never be deleted, even when files named old_chrome*.exe have the same
+// versions as chrome.exe/new_chrome.exe. The old_chrome*.exe files, however,
+// should be deleted.
+TEST_F(DeleteOldVersionsTest,
+       DeleteNewExecutablesWithMatchingDirectoryAndOldExecutables) {
+  const base::string16 version_a =
+      CreateExecutable(installer::kChromeExe, Direction::PREVIOUS_VERSION);
+  ASSERT_FALSE(version_a.empty());
+  ASSERT_TRUE(CreateVersionDirectory(version_a));
+  const base::string16 version_b =
+      CreateExecutable(installer::kChromeNewExe, Direction::NEXT_VERSION);
+  ASSERT_FALSE(version_b.empty());
+  ASSERT_TRUE(CreateVersionDirectory(version_b));
+  EXPECT_EQ(version_a,
+            CreateExecutable(L"old_chrome.exe", Direction::PREVIOUS_VERSION));
+  EXPECT_EQ(version_b,
+            CreateExecutable(L"old_chrome2.exe", Direction::NEXT_VERSION));
+
+  DeleteOldVersions(install_dir());
+
+  FilePathSet expected_install_dir_content;
+  expected_install_dir_content.insert(base::FilePath(installer::kChromeExe));
+  AddVersionFiles(version_a, &expected_install_dir_content);
+  expected_install_dir_content.insert(base::FilePath(installer::kChromeNewExe));
+  AddVersionFiles(version_b, &expected_install_dir_content);
+  EXPECT_EQ(expected_install_dir_content, GetInstallDirContent());
+}
+
+// No file should be deleted for a given version if the executable is in use.
+TEST_F(DeleteOldVersionsTest, DeleteVersionWithExecutableInUse) {
+  const base::string16 version_a =
+      CreateExecutable(installer::kChromeOldExe, Direction::PREVIOUS_VERSION);
+  ASSERT_FALSE(version_a.empty());
+  ASSERT_TRUE(CreateVersionDirectory(version_a));
+
+  base::File file_in_use(install_dir().Append(installer::kChromeOldExe),
+                         base::File::FLAG_OPEN | base::File::FLAG_READ);
+  ASSERT_TRUE(file_in_use.IsValid());
+
+  DeleteOldVersions(install_dir());
+
+  FilePathSet expected_install_dir_content;
+  expected_install_dir_content.insert(base::FilePath(installer::kChromeOldExe));
+  AddVersionFiles(version_a, &expected_install_dir_content);
+  EXPECT_EQ(expected_install_dir_content, GetInstallDirContent());
+}
+
+// No file should be deleted for a given version if a .dll file in the version
+// directory is in use.
+TEST_F(DeleteOldVersionsTest, DeleteVersionWithVersionDirectoryDllInUse) {
+  const base::string16 version_a =
+      CreateExecutable(installer::kChromeOldExe, Direction::PREVIOUS_VERSION);
+  ASSERT_FALSE(version_a.empty());
+  ASSERT_TRUE(CreateVersionDirectory(version_a));
+
+  base::File file_in_use(install_dir().Append(version_a).Append(L"chrome.dll"),
+                         base::File::FLAG_OPEN | base::File::FLAG_READ);
+  ASSERT_TRUE(file_in_use.IsValid());
+
+  DeleteOldVersions(install_dir());
+
+  FilePathSet expected_install_dir_content;
+  expected_install_dir_content.insert(base::FilePath(installer::kChromeOldExe));
+  AddVersionFiles(version_a, &expected_install_dir_content);
+  EXPECT_EQ(expected_install_dir_content, GetInstallDirContent());
+}
+
+// No file should be deleted for a given version if a .exe file in the version
+// directory is in use.
+TEST_F(DeleteOldVersionsTest, DeleteVersionWithVersionDirectoryExeInUse) {
+  const base::string16 version_a =
+      CreateExecutable(installer::kChromeOldExe, Direction::PREVIOUS_VERSION);
+  ASSERT_FALSE(version_a.empty());
+  ASSERT_TRUE(CreateVersionDirectory(version_a));
+
+  base::File file_in_use(
+      install_dir().Append(version_a).Append(L"Installer\\setup.exe"),
+      base::File::FLAG_OPEN | base::File::FLAG_READ);
+  ASSERT_TRUE(file_in_use.IsValid());
+
+  DeleteOldVersions(install_dir());
+
+  FilePathSet expected_install_dir_content;
+  expected_install_dir_content.insert(base::FilePath(installer::kChromeOldExe));
+  AddVersionFiles(version_a, &expected_install_dir_content);
+  EXPECT_EQ(expected_install_dir_content, GetInstallDirContent());
+}
+
+// If an installation directory contains a file named chrome.exe with a matching
+// directory v1 and a file named old_chrome.exe with a matching directory v2,
+// old_chrome.exe and v2 should be deleted but chrome.exe and v1 shouldn't.
+TEST_F(DeleteOldVersionsTest, TypicalAfterRenameState) {
+  const base::string16 version_a =
+      CreateExecutable(installer::kChromeOldExe, Direction::PREVIOUS_VERSION);
+  ASSERT_FALSE(version_a.empty());
+  ASSERT_TRUE(CreateVersionDirectory(version_a));
+  const base::string16 version_b =
+      CreateExecutable(installer::kChromeExe, Direction::NEXT_VERSION);
+  ASSERT_FALSE(version_b.empty());
+  ASSERT_TRUE(CreateVersionDirectory(version_b));
+
+  DeleteOldVersions(install_dir());
+
+  FilePathSet expected_install_dir_content;
+  expected_install_dir_content.insert(base::FilePath(installer::kChromeExe));
+  AddVersionFiles(version_b, &expected_install_dir_content);
+  EXPECT_EQ(expected_install_dir_content, GetInstallDirContent());
+}
+
+}  // namespace installer

chrome/installer/util/installer_state.cc

@@ -12,12 +12,9 @@
 #include <utility>
 
 #include "base/command_line.h"
-#include "base/file_version_info.h"
-#include "base/files/file_enumerator.h"
 #include "base/files/file_util.h"
 #include "base/logging.h"
 #include "base/macros.h"
-#include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/win/registry.h"
 #include "base/win/scoped_handle.h"
@@ -514,69 +511,6 @@ bool InstallerState::AnyExistsAndIsInUse(const InstallationState& machine_state,
   return false;
 }
 
-void InstallerState::GetExistingExeVersions(
-    std::set<std::string>* existing_versions) const {
-
-  static const wchar_t* const kChromeFilenames[] = {
-    installer::kChromeExe,
-    installer::kChromeNewExe,
-    installer::kChromeOldExe,
-  };
-
-  for (size_t i = 0; i < arraysize(kChromeFilenames); ++i) {
-    base::FilePath chrome_exe(target_path().Append(kChromeFilenames[i]));
-    std::unique_ptr<FileVersionInfo> file_version_info(
-        FileVersionInfo::CreateFileVersionInfo(chrome_exe));
-    if (file_version_info) {
-      base::string16 version_string = file_version_info->file_version();
-      if (!version_string.empty() && base::IsStringASCII(version_string))
-        existing_versions->insert(base::UTF16ToASCII(version_string));
-    }
-  }
-}
-
-void InstallerState::RemoveOldVersionDirectories(
-    const base::Version& new_version,
-    base::Version* existing_version,
-    const base::FilePath& temp_path) const {
-  base::Version version;
-  std::unique_ptr<WorkItem> item;
-
-  std::set<std::string> existing_version_strings;
-  existing_version_strings.insert(new_version.GetString());
-  if (existing_version)
-    existing_version_strings.insert(existing_version->GetString());
-
-  // Make sure not to delete any version dir that is "referenced" by an existing
-  // Chrome executable.
-  GetExistingExeVersions(&existing_version_strings);
-
-  // Try to delete all directories that are not in the set we care to keep.
-  base::FileEnumerator version_enum(target_path(), false,
-                                    base::FileEnumerator::DIRECTORIES);
-  for (base::FilePath next_version = version_enum.Next(); !next_version.empty();
-       next_version = version_enum.Next()) {
-    base::FilePath dir_name(next_version.BaseName());
-    version = base::Version(base::UTF16ToASCII(dir_name.value()));
-    // Delete the version folder if it is less than the new version and not
-    // equal to the old version (if we have an old version).
-    if (version.IsValid() &&
-        existing_version_strings.count(version.GetString()) == 0) {
-      // Note: temporarily log old version deletion at ERROR level to make it
-      // more likely we see this in the installer log.
-      LOG(ERROR) << "Deleting old version directory: " << next_version.value();
-
-      // Attempt to recursively delete the old version dir.
-      bool delete_succeeded = base::DeleteFile(next_version, true);
-
-      // Note: temporarily log old version deletion at ERROR level to make it
-      // more likely we see this in the installer log.
-      LOG_IF(ERROR, !delete_succeeded)
-          << "Failed to delete old version directory: " << next_version.value();
-    }
-  }
-}
-
 void InstallerState::AddComDllList(
     std::vector<base::FilePath>* com_dll_list) const {
   for (auto* product : products_)

chrome/installer/util/installer_state.h

@@ -8,7 +8,6 @@
 #include <stdint.h>
 
 #include <memory>
-#include <set>
 #include <string>
 #include <vector>
 
@@ -185,13 +184,6 @@ class InstallerState {
   // (for example <target_path>\Google\Chrome\Application\<Version>\Installer)
   base::FilePath GetInstallerDirectory(const base::Version& version) const;
 
-  // Try to delete all directories under |temp_path| whose versions are less
-  // than |new_version| and not equal to |existing_version|. |existing_version|
-  // may be NULL.
-  void RemoveOldVersionDirectories(const base::Version& new_version,
-                                   base::Version* existing_version,
-                                   const base::FilePath& temp_path) const;
-
   // Adds to |com_dll_list| the list of COM DLLs that are to be registered
   // and/or unregistered. The list may be empty.
   void AddComDllList(std::vector<base::FilePath>* com_dll_list) const;
@@ -251,11 +243,6 @@ class InstallerState {
   bool IsMultiInstallUpdate(const MasterPreferences& prefs,
                             const InstallationState& machine_state);
 
-  // Enumerates all files named one of
-  // [chrome.exe, old_chrome.exe, new_chrome.exe] in target_path_ and
-  // returns their version numbers in a set.
-  void GetExistingExeVersions(std::set<std::string>* existing_versions) const;
-
   // Sets this object's level and updates the root_key_ accordingly.
   void set_level(Level level);
 

chrome/installer/util/installer_state_unittest.cc

@@ -11,7 +11,6 @@
 
 #include "base/base_paths.h"
 #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/files/scoped_temp_dir.h"
@@ -25,7 +24,6 @@
 #include "base/win/registry.h"
 #include "base/win/scoped_handle.h"
 #include "chrome/common/chrome_constants.h"
-#include "chrome/installer/test/alternate_version_generator.h"
 #include "chrome/installer/util/fake_installation_state.h"
 #include "chrome/installer/util/fake_product_state.h"
 #include "chrome/installer/util/google_update_constants.h"
@@ -70,276 +68,8 @@ class MockInstallerState : public InstallerState {
   const base::Version& critical_update_version() const {
     return critical_update_version_;
   }
-  void GetExistingExeVersions(std::set<std::string>* existing_version_strings) {
-    return InstallerState::GetExistingExeVersions(existing_version_strings);
-  }
 };
 
-// Simple function to dump some text into a new file.
-void CreateTextFile(const std::wstring& filename,
-                    const std::wstring& contents) {
-  std::ofstream file;
-  file.open(filename.c_str());
-  ASSERT_TRUE(file.is_open());
-  file << contents;
-  file.close();
-}
-
-void BuildSingleChromeState(const base::FilePath& target_dir,
-                            MockInstallerState* installer_state) {
-  base::CommandLine cmd_line = base::CommandLine::FromString(L"setup.exe");
-  MasterPreferences prefs(cmd_line);
-  InstallationState machine_state;
-  machine_state.Initialize();
-  installer_state->Initialize(cmd_line, prefs, machine_state);
-  installer_state->set_target_path(target_dir);
-  EXPECT_TRUE(installer_state->FindProduct(BrowserDistribution::CHROME_BROWSER)
-      != NULL);
-}
-
-wchar_t text_content_1[] = L"delete me";
-wchar_t text_content_2[] = L"delete me as well";
-
-// Delete version directories. Everything lower than the given version
-// should be deleted.
-TEST_F(InstallerStateTest, Delete) {
-  // TODO(grt): move common stuff into the test fixture.
-  // Create a Chrome dir
-  base::FilePath chrome_dir(test_dir_.path());
-  chrome_dir = chrome_dir.AppendASCII("chrome");
-  base::CreateDirectory(chrome_dir);
-  ASSERT_TRUE(base::PathExists(chrome_dir));
-
-  base::FilePath chrome_dir_1(chrome_dir);
-  chrome_dir_1 = chrome_dir_1.AppendASCII("1.0.1.0");
-  base::CreateDirectory(chrome_dir_1);
-  ASSERT_TRUE(base::PathExists(chrome_dir_1));
-
-  base::FilePath chrome_dir_2(chrome_dir);
-  chrome_dir_2 = chrome_dir_2.AppendASCII("1.0.2.0");
-  base::CreateDirectory(chrome_dir_2);
-  ASSERT_TRUE(base::PathExists(chrome_dir_2));
-
-  base::FilePath chrome_dir_3(chrome_dir);
-  chrome_dir_3 = chrome_dir_3.AppendASCII("1.0.3.0");
-  base::CreateDirectory(chrome_dir_3);
-  ASSERT_TRUE(base::PathExists(chrome_dir_3));
-
-  base::FilePath chrome_dir_4(chrome_dir);
-  chrome_dir_4 = chrome_dir_4.AppendASCII("1.0.4.0");
-  base::CreateDirectory(chrome_dir_4);
-  ASSERT_TRUE(base::PathExists(chrome_dir_4));
-
-  base::FilePath chrome_dll_1(chrome_dir_1);
-  chrome_dll_1 = chrome_dll_1.AppendASCII("chrome.dll");
-  CreateTextFile(chrome_dll_1.value(), text_content_1);
-  ASSERT_TRUE(base::PathExists(chrome_dll_1));
-
-  base::FilePath chrome_dll_2(chrome_dir_2);
-  chrome_dll_2 = chrome_dll_2.AppendASCII("chrome.dll");
-  CreateTextFile(chrome_dll_2.value(), text_content_1);
-  ASSERT_TRUE(base::PathExists(chrome_dll_2));
-
-  base::FilePath chrome_dll_3(chrome_dir_3);
-  chrome_dll_3 = chrome_dll_3.AppendASCII("chrome.dll");
-  CreateTextFile(chrome_dll_3.value(), text_content_1);
-  ASSERT_TRUE(base::PathExists(chrome_dll_3));
-
-  base::FilePath chrome_dll_4(chrome_dir_4);
-  chrome_dll_4 = chrome_dll_4.AppendASCII("chrome.dll");
-  CreateTextFile(chrome_dll_4.value(), text_content_1);
-  ASSERT_TRUE(base::PathExists(chrome_dll_4));
-
-  MockInstallerState installer_state;
-  BuildSingleChromeState(chrome_dir, &installer_state);
-  base::Version latest_version("1.0.4.0");
-  {
-    base::ScopedTempDir temp_dir;
-    ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
-    installer_state.RemoveOldVersionDirectories(latest_version, NULL,
-                                                temp_dir.path());
-  }
-
-  // old versions should be gone
-  EXPECT_FALSE(base::PathExists(chrome_dir_1));
-  EXPECT_FALSE(base::PathExists(chrome_dir_2));
-  EXPECT_FALSE(base::PathExists(chrome_dir_3));
-  // the latest version should stay
-  EXPECT_TRUE(base::PathExists(chrome_dll_4));
-}
-
-// Delete older version directories, keeping the one in used intact.
-TEST_F(InstallerStateTest, DeleteInUsed) {
-  // Create a Chrome dir
-  base::FilePath chrome_dir(test_dir_.path());
-  chrome_dir = chrome_dir.AppendASCII("chrome");
-  base::CreateDirectory(chrome_dir);
-  ASSERT_TRUE(base::PathExists(chrome_dir));
-
-  base::FilePath chrome_dir_1(chrome_dir);
-  chrome_dir_1 = chrome_dir_1.AppendASCII("1.0.1.0");
-  base::CreateDirectory(chrome_dir_1);
-  ASSERT_TRUE(base::PathExists(chrome_dir_1));
-
-  base::FilePath chrome_dir_2(chrome_dir);
-  chrome_dir_2 = chrome_dir_2.AppendASCII("1.0.2.0");
-  base::CreateDirectory(chrome_dir_2);
-  ASSERT_TRUE(base::PathExists(chrome_dir_2));
-
-  base::FilePath chrome_dir_3(chrome_dir);
-  chrome_dir_3 = chrome_dir_3.AppendASCII("1.0.3.0");
-  base::CreateDirectory(chrome_dir_3);
-  ASSERT_TRUE(base::PathExists(chrome_dir_3));
-
-  base::FilePath chrome_dir_4(chrome_dir);
-  chrome_dir_4 = chrome_dir_4.AppendASCII("1.0.4.0");
-  base::CreateDirectory(chrome_dir_4);
-  ASSERT_TRUE(base::PathExists(chrome_dir_4));
-
-  base::FilePath chrome_dll_1(chrome_dir_1);
-  chrome_dll_1 = chrome_dll_1.AppendASCII("chrome.dll");
-  CreateTextFile(chrome_dll_1.value(), text_content_1);
-  ASSERT_TRUE(base::PathExists(chrome_dll_1));
-
-  base::FilePath chrome_dll_2(chrome_dir_2);
-  chrome_dll_2 = chrome_dll_2.AppendASCII("chrome.dll");
-  CreateTextFile(chrome_dll_2.value(), text_content_1);
-  ASSERT_TRUE(base::PathExists(chrome_dll_2));
-
-  // Open the file to make it in use.
-  std::ofstream file;
-  file.open(chrome_dll_2.value().c_str());
-
-  base::FilePath chrome_othera_2(chrome_dir_2);
-  chrome_othera_2 = chrome_othera_2.AppendASCII("othera.dll");
-  CreateTextFile(chrome_othera_2.value(), text_content_2);
-  ASSERT_TRUE(base::PathExists(chrome_othera_2));
-
-  base::FilePath chrome_otherb_2(chrome_dir_2);
-  chrome_otherb_2 = chrome_otherb_2.AppendASCII("otherb.dll");
-  CreateTextFile(chrome_otherb_2.value(), text_content_2);
-  ASSERT_TRUE(base::PathExists(chrome_otherb_2));
-
-  base::FilePath chrome_dll_3(chrome_dir_3);
-  chrome_dll_3 = chrome_dll_3.AppendASCII("chrome.dll");
-  CreateTextFile(chrome_dll_3.value(), text_content_1);
-  ASSERT_TRUE(base::PathExists(chrome_dll_3));
-
-  base::FilePath chrome_dll_4(chrome_dir_4);
-  chrome_dll_4 = chrome_dll_4.AppendASCII("chrome.dll");
-  CreateTextFile(chrome_dll_4.value(), text_content_1);
-  ASSERT_TRUE(base::PathExists(chrome_dll_4));
-
-  MockInstallerState installer_state;
-  BuildSingleChromeState(chrome_dir, &installer_state);
-  base::Version latest_version("1.0.4.0");
-  base::Version existing_version("1.0.1.0");
-  {
-    base::ScopedTempDir temp_dir;
-    ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
-    installer_state.RemoveOldVersionDirectories(latest_version,
-                                                &existing_version,
-                                                temp_dir.path());
-  }
-
-  // the version defined as the existing version should stay
-  EXPECT_TRUE(base::PathExists(chrome_dir_1));
-  // old versions not in used should be gone
-  EXPECT_FALSE(base::PathExists(chrome_dir_3));
-  // every thing under in used version should stay
-  EXPECT_TRUE(base::PathExists(chrome_dir_2));
-  EXPECT_TRUE(base::PathExists(chrome_dll_2));
-  EXPECT_TRUE(base::PathExists(chrome_othera_2));
-  EXPECT_TRUE(base::PathExists(chrome_otherb_2));
-  // the latest version should stay
-  EXPECT_TRUE(base::PathExists(chrome_dll_4));
-}
-
-// Tests a few basic things of the Package class.  Makes sure that the path
-// operations are correct
-TEST_F(InstallerStateTest, Basic) {
-  const bool multi_install = false;
-  const bool system_level = true;
-  base::CommandLine cmd_line = base::CommandLine::FromString(
-      std::wstring(L"setup.exe") +
-      (multi_install ? L" --multi-install --chrome" : L"") +
-      (system_level ? L" --system-level" : L""));
-  MasterPreferences prefs(cmd_line);
-  InstallationState machine_state;
-  machine_state.Initialize();
-  MockInstallerState installer_state;
-  installer_state.Initialize(cmd_line, prefs, machine_state);
-  installer_state.set_target_path(test_dir_.path());
-  EXPECT_EQ(test_dir_.path().value(), installer_state.target_path().value());
-  EXPECT_EQ(1U, installer_state.products().size());
-
-  const char kOldVersion[] = "1.2.3.4";
-  const char kNewVersion[] = "2.3.4.5";
-
-  base::Version new_version(kNewVersion);
-  base::Version old_version(kOldVersion);
-  ASSERT_TRUE(new_version.IsValid());
-  ASSERT_TRUE(old_version.IsValid());
-
-  base::FilePath installer_dir(
-      installer_state.GetInstallerDirectory(new_version));
-  EXPECT_FALSE(installer_dir.empty());
-
-  base::FilePath new_version_dir(installer_state.target_path().Append(
-      base::UTF8ToWide(new_version.GetString())));
-  base::FilePath old_version_dir(installer_state.target_path().Append(
-      base::UTF8ToWide(old_version.GetString())));
-
-  EXPECT_FALSE(base::PathExists(new_version_dir));
-  EXPECT_FALSE(base::PathExists(old_version_dir));
-
-  EXPECT_FALSE(base::PathExists(installer_dir));
-  base::CreateDirectory(installer_dir);
-  EXPECT_TRUE(base::PathExists(new_version_dir));
-
-  base::CreateDirectory(old_version_dir);
-  EXPECT_TRUE(base::PathExists(old_version_dir));
-
-  // Create a fake chrome.dll key file in the old version directory.  This
-  // should prevent the old version directory from getting deleted.
-  base::FilePath old_chrome_dll(old_version_dir.Append(installer::kChromeDll));
-  EXPECT_FALSE(base::PathExists(old_chrome_dll));
-
-  // Hold on to the file exclusively to prevent the directory from
-  // being deleted.
-  base::win::ScopedHandle file(
-    ::CreateFile(old_chrome_dll.value().c_str(), GENERIC_READ,
-                 0, NULL, OPEN_ALWAYS, 0, NULL));
-  EXPECT_TRUE(file.IsValid());
-  EXPECT_TRUE(base::PathExists(old_chrome_dll));
-
-  base::ScopedTempDir temp_dir;
-  ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
-
-  // Don't explicitly tell the directory cleanup logic not to delete the
-  // old version, rely on the key files to keep it around.
-  installer_state.RemoveOldVersionDirectories(new_version,
-                                              NULL,
-                                              temp_dir.path());
-
-  // The old directory should still exist.
-  EXPECT_TRUE(base::PathExists(old_version_dir));
-  EXPECT_TRUE(base::PathExists(new_version_dir));
-
-  // Now close the file handle to make it possible to delete our key file.
-  file.Close();
-
-  installer_state.RemoveOldVersionDirectories(new_version,
-                                              NULL,
-                                              temp_dir.path());
-  // The new directory should still exist.
-  EXPECT_TRUE(base::PathExists(new_version_dir));
-
-  // Now, the old directory and key file should be gone.
-  EXPECT_FALSE(base::PathExists(old_chrome_dll));
-  EXPECT_FALSE(base::PathExists(old_version_dir));
-}
-
 TEST_F(InstallerStateTest, WithProduct) {
   const bool multi_install = false;
   const bool system_level = true;
@@ -494,11 +224,9 @@ TEST_F(InstallerStateTest, IsFileInUse) {
   {
     // Open a handle to the file with the same access mode and sharing options
     // as the loader.
-    base::win::ScopedHandle temp_handle(
-        CreateFile(temp_file.value().c_str(),
-                   SYNCHRONIZE | FILE_EXECUTE,
-                   FILE_SHARE_DELETE | FILE_SHARE_READ,
-                   NULL, OPEN_EXISTING, 0, 0));
+    base::win::ScopedHandle temp_handle(CreateFile(
+        temp_file.value().c_str(), SYNCHRONIZE | FILE_EXECUTE,
+        FILE_SHARE_DELETE | FILE_SHARE_READ, NULL, OPEN_EXISTING, 0, 0));
     ASSERT_TRUE(temp_handle.IsValid());
 
     // The file should now be in use.
@@ -509,112 +237,6 @@ TEST_F(InstallerStateTest, IsFileInUse) {
   EXPECT_FALSE(MockInstallerState::IsFileInUse(temp_file));
 }
 
-TEST_F(InstallerStateTest, RemoveOldVersionDirs) {
-  MockInstallerState installer_state;
-  installer_state.set_target_path(test_dir_.path());
-  EXPECT_EQ(test_dir_.path().value(), installer_state.target_path().value());
-
-  const char kOldVersion[] = "2.0.0.0";
-  const char kNewVersion[] = "3.0.0.0";
-  const char kOldChromeExeVersion[] = "2.1.0.0";
-  const char kChromeExeVersion[] = "2.1.1.1";
-  const char kNewChromeExeVersion[] = "3.0.0.0";
-
-  base::Version new_version(kNewVersion);
-  base::Version old_version(kOldVersion);
-  base::Version old_chrome_exe_version(kOldChromeExeVersion);
-  base::Version chrome_exe_version(kChromeExeVersion);
-  base::Version new_chrome_exe_version(kNewChromeExeVersion);
-
-  ASSERT_TRUE(new_version.IsValid());
-  ASSERT_TRUE(old_version.IsValid());
-  ASSERT_TRUE(old_chrome_exe_version.IsValid());
-  ASSERT_TRUE(chrome_exe_version.IsValid());
-  ASSERT_TRUE(new_chrome_exe_version.IsValid());
-
-  // Set up a bunch of version dir paths.
-  base::FilePath version_dirs[] = {
-    installer_state.target_path().Append(L"1.2.3.4"),
-    installer_state.target_path().Append(L"1.2.3.5"),
-    installer_state.target_path().Append(L"1.2.3.6"),
-    installer_state.target_path().AppendASCII(kOldVersion),
-    installer_state.target_path().AppendASCII(kOldChromeExeVersion),
-    installer_state.target_path().Append(L"2.1.1.0"),
-    installer_state.target_path().AppendASCII(kChromeExeVersion),
-    installer_state.target_path().AppendASCII(kNewVersion),
-    installer_state.target_path().Append(L"3.9.1.1"),
-  };
-
-  // Create the version directories.
-  for (size_t i = 0; i < arraysize(version_dirs); i++) {
-    base::CreateDirectory(version_dirs[i]);
-    EXPECT_TRUE(base::PathExists(version_dirs[i]));
-  }
-
-  // Create exes with the appropriate version resource.
-  // Use the current test exe as a baseline.
-  base::FilePath exe_path;
-  ASSERT_TRUE(PathService::Get(base::FILE_EXE, &exe_path));
-
-  struct target_info {
-    base::FilePath target_file;
-    const base::Version& target_version;
-  } targets[] = {
-    { installer_state.target_path().Append(installer::kChromeOldExe),
-      old_chrome_exe_version },
-    { installer_state.target_path().Append(installer::kChromeExe),
-      chrome_exe_version },
-    { installer_state.target_path().Append(installer::kChromeNewExe),
-      new_chrome_exe_version },
-  };
-  for (size_t i = 0; i < arraysize(targets); ++i) {
-    ASSERT_TRUE(upgrade_test::GenerateSpecificPEFileVersion(
-        exe_path, targets[i].target_file, targets[i].target_version));
-  }
-
-  // Call GetExistingExeVersions, validate that picks up the
-  // exe resources.
-  std::set<std::string> expected_exe_versions;
-  expected_exe_versions.insert(kOldChromeExeVersion);
-  expected_exe_versions.insert(kChromeExeVersion);
-  expected_exe_versions.insert(kNewChromeExeVersion);
-
-  std::set<std::string> actual_exe_versions;
-  installer_state.GetExistingExeVersions(&actual_exe_versions);
-  EXPECT_EQ(expected_exe_versions, actual_exe_versions);
-
-  // Call RemoveOldVersionDirectories
-  installer_state.RemoveOldVersionDirectories(new_version,
-                                              &old_version,
-                                              installer_state.target_path());
-
-  // What we expect to have left.
-  std::set<std::string> expected_remaining_dirs;
-  expected_remaining_dirs.insert(kOldVersion);
-  expected_remaining_dirs.insert(kNewVersion);
-  expected_remaining_dirs.insert(kOldChromeExeVersion);
-  expected_remaining_dirs.insert(kChromeExeVersion);
-  expected_remaining_dirs.insert(kNewChromeExeVersion);
-
-  // Enumerate dirs in target_path(), ensure only desired remain.
-  base::FileEnumerator version_enum(installer_state.target_path(), false,
-                                    base::FileEnumerator::DIRECTORIES);
-  for (base::FilePath next_version = version_enum.Next(); !next_version.empty();
-       next_version = version_enum.Next()) {
-    base::FilePath dir_name(next_version.BaseName());
-    base::Version version(base::UTF16ToASCII(dir_name.value()));
-    if (version.IsValid()) {
-      EXPECT_TRUE(expected_remaining_dirs.erase(version.GetString()))
-          << "Unexpected version dir found: " << version.GetString();
-    }
-  }
-
-  std::set<std::string>::const_iterator iter(
-      expected_remaining_dirs.begin());
-  for (; iter != expected_remaining_dirs.end(); ++iter)
-    ADD_FAILURE() << "Expected to find version dir for " << *iter;
-}
-
 TEST_F(InstallerStateTest, InitializeTwice) {
   // Override these paths so that they can be found after the registry override
   // manager is in place.