Delete old files after an update.

chrome/installer/util/delete_old_versions.cc

+// Copyright (c) 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 <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/memory/scoped_ptr.h"
+#include "base/strings/string16.h"
+#include "chrome/installer/util/util_constants.h"
+
+namespace {
+
+// Returns the version of the executable |exe_path|.
+base::string16 GetExecutableVersion(const base::FilePath& exe_path) {
+  scoped_ptr<FileVersionInfo> file_version_info(
+      FileVersionInfo::CreateFileVersionInfo(exe_path));
+  if (!file_version_info.get())
+    return base::string16();
+  return file_version_info->file_version();
+}
+
+// Returns the directory names found in |install_dir|. The directories named
+// after the version of chrome.exe or new_chrome.exe are excluded.
+using DirectorySet = std::set<base::string16>;
+DirectorySet GetDirectories(const base::FilePath& install_dir) {
+  const base::string16 new_chrome_exe_version =
+      GetExecutableVersion(install_dir.Append(installer::kChromeNewExe));
+  const base::string16 chrome_exe_version =
+      GetExecutableVersion(install_dir.Append(installer::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::string16 directory_name = directory_path.BaseName().value();

[grt (UTC plus 2), 2016/02/10 20:02:04]

nit: make this and the container use base::FilePath rather than string16.

[fdoray, 2016/02/18 17:59:20]

Done.

+    if (directory_name != new_chrome_exe_version &&
+        directory_name != chrome_exe_version) {
+      directories.insert(directory_name);

[grt (UTC plus 2), 2016/02/10 20:02:04]

for the sake of caution, perhaps this should only consider directories that are
named W.X.Y.Z. wdyt?

[fdoray, 2016/02/18 17:59:20]

Done.

+    }
+  }
+  return directories;
+}
+
+// Returns a map where the keys are versions and values are paths of
+// old_chrome*.exe executables found in |install_dir|.
+using ExecutableVector = std::vector<base::FilePath>;

[grt (UTC plus 2), 2016/02/10 20:02:04]

nit: change this type to PathVector and use it on line 122

[fdoray, 2016/02/18 17:59:20]

Done.

+using ExecutableMap = std::map<base::string16, ExecutableVector>;

[grt (UTC plus 2), 2016/02/10 20:02:03]

maybe this should also map a FilePath to a vector of FilePaths.
GetExecutableVersion would need to return a FilePath, so it could be changed to
GetExecutableVersionDirName or somesuch

[fdoray, 2016/02/18 17:59:20]

Done.

+ExecutableMap GetExecutables(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[GetExecutableVersion(exe_path)].push_back(exe_path);
+  }
+  return executables;
+}
+
+// Deletes directories that are in |directories| and don't have a matching
+// executable in |executables|.
+bool DeleteDirectoriesWithoutMatchingExecutable(
+    const DirectorySet& directories,
+    const ExecutableMap& executables,
+    const base::FilePath& install_dir) {
+  bool success = true;
+  for (const base::string16& directory_name : directories) {
+    // Delete the directory if it doesn't have a matching executable.
+    if (executables.find(directory_name) == executables.end())

[grt (UTC plus 2), 2016/02/10 20:02:04]

nit:
#include "base/stl_util.h"
    if (!ContainsValue(executables, directory_name))

[fdoray, 2016/02/18 17:59:20]

Done.

+      success &= base::DeleteFile(install_dir.Append(directory_name), true);

[grt (UTC plus 2), 2016/02/10 20:02:03]

we expect that this will never happen, so i think it makes sense to LOG(WARNING)
that it's about to happen, and PLOG(ERROR) if the delete fails.

[fdoray, 2016/02/18 17:59:20]

Done.

+  }
+  return success;
+}
+
+// Deletes executables that are in |executables| and don't have a matching
+// directory in |directories|.
+bool DeleteExecutablesWithoutMatchingDirectory(
+    const DirectorySet& directories,
+    const ExecutableMap& executables) {
+  bool success = true;
+  for (const auto& version_and_executables : executables) {
+    const auto& version = 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 (directories.find(version) != directories.end())

[grt (UTC plus 2), 2016/02/10 20:02:03]

    if (ContainsValue(directories, version))

[fdoray, 2016/02/18 17:59:20]

Done.

+      continue;
+
+    // Delete executables for version |version|.
+    for (const auto& executable_path : executables_for_version)
+      success &= base::DeleteFile(executable_path, false);

[grt (UTC plus 2), 2016/02/10 20:02:04]

we expect that this will never happen, so i think it makes sense to LOG(WARNING)
that it's about to happen, and PLOG(ERROR) if the delete fails.

[fdoray, 2016/02/18 17:59:20]

Done.

+  }
+  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.
+bool DeleteVersion(const base::FilePath& version_directory,
+                   const ExecutableVector& version_executables) {
+  std::vector<base::File> locks;
+  std::vector<base::FilePath> 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(L".exe") || path.MatchesExtension(L".dll")) {

[grt (UTC plus 2), 2016/02/10 20:02:04]

nit: use FILE_PATH_LITERAL("") rather than L"" for constants like these

[grt (UTC plus 2), 2016/02/10 20:02:04]

nit: reverse the logic here to "continue" when not either one to reduce
indentation below

[fdoray, 2016/02/18 17:59:20]

Done.

[fdoray, 2016/02/18 17:59:20]

Done.

+      base::File lock(GetFileLock(path));

[grt (UTC plus 2), 2016/02/10 20:02:04]

wdyt of doing away with the local here and on line 140?
       locks.push_back(GetFileLock(path));
       if (!locks.back().IsValid())
         return false;

[fdoray, 2016/02/18 17:59:20]

Done.

+      if (!lock.IsValid())
+        return false;

[grt (UTC plus 2), 2016/02/10 20:02:03]

It makes sense to use LOG(WARNING) to indicate what file(s) are in use so we can
see from logs why the deletion fails.

[fdoray, 2016/02/18 17:59:20]

Done.

+      locks.push_back(std::move(lock));

[grt (UTC plus 2), 2016/02/10 20:02:03]

#include <utility> if you keep this

[fdoray, 2016/02/18 17:59:20]

I no longer use std::move.

+      locked_file_paths.push_back(path);
+    }
+  }
+
+  // Lock executables in |version_executables|.
+  for (const base::FilePath& executable_path : version_executables) {
+    base::File lock(GetFileLock(executable_path));
+    if (!lock.IsValid())
+      return false;

[grt (UTC plus 2), 2016/02/10 20:02:03]

same comment here about logging

[fdoray, 2016/02/18 17:59:20]

Done.

+    locks.push_back(std::move(lock));
+    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)
+    success &= base::DeleteFile(locked_file_path, false);

[grt (UTC plus 2), 2016/02/10 20:02:04]

this isn't ever expected to fail, right? PLOG(ERROR) in case it does.

[fdoray, 2016/02/18 17:59:20]

Done.

+
+  // 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.
+  success &= base::DeleteFile(version_directory, true);

[grt (UTC plus 2), 2016/02/10 20:02:04]

PLOG(ERROR) when this fails so we understand why

[fdoray, 2016/02/18 17:59:20]

Done.

+
+  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.
+bool DeleteMatchingExecutablesAndDirectories(
+    const DirectorySet& directories,
+    const ExecutableMap& executables,
+    const base::FilePath& install_dir) {
+  bool success = true;
+  for (const auto directory_name : directories) {

[grt (UTC plus 2), 2016/02/10 20:02:04]

nit: const auto&

[fdoray, 2016/02/18 17:59:20]

Done.

+    ExecutableMap::const_iterator version_executables_it =
+        executables.find(directory_name);
+
+    // Check if the directory has at least one matching executable.
+    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 directories = GetDirectories(install_dir);
+  const ExecutableMap executables = GetExecutables(install_dir);
+
+  bool success = true;
+  success &= DeleteDirectoriesWithoutMatchingExecutable(

[grt (UTC plus 2), 2016/02/10 20:02:04]

I'm not a fan of using bitwise operations on bools like this. Is there another
way? Is it important for this function to return a bool (it's currently
ignored)?

[fdoray, 2016/02/18 17:59:20]

I want the function to return a bool so that we know when to launch/keep alive a
cleanup process. I didn't write if (!DeleteDirectories... ||
!DeleteExecutables... | ...) because of short-circuit evaluation. I could write:

if (!DeleteDirectories...)
  success = false;
if (!DeleteExecutables...)
  success = false;
if (!DeleteMatching...)
  success = false;

Or I could get rid of all the |success| variables and check after the fact if
there is remaining old files. 

+      directories, executables, install_dir);
+  success &=
+      DeleteExecutablesWithoutMatchingDirectory(directories, executables);
+  success &= DeleteMatchingExecutablesAndDirectories(directories, executables,
+                                                     install_dir);
+
+  return success;
+}