Drastically simplify old version cleanup and add some additional logging.

BUG=75951
TEST=Old version directories are deleted more reliably.


Review URL: https://chromiumcodereview.appspot.com/11235043

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@165493 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 222 insertions, 30 deletions

diff --git a/chrome/chrome_installer.gypi b/chrome/chrome_installer.gypi
index ea02442..850b5f7 100644
--- a/chrome/chrome_installer.gypi
+++ b/chrome/chrome_installer.gypi
@@ -74,10 +74,12 @@
           'dependencies': [
             'installer_util',
             'installer_util_strings',
+            'installer/upgrade_test.gyp:alternate_version_generator_lib',
             '../base/base.gyp:base',
             '../base/base.gyp:base_i18n',
             '../base/base.gyp:test_support_base',
             '../build/temp_gyp/googleurl.gyp:googleurl',
+            '../chrome/chrome.gyp:chrome_version_resources',
             '../content/content.gyp:content_common',
             '../testing/gmock.gyp:gmock',
             '../testing/gtest.gyp:gtest',
@@ -127,6 +129,7 @@
             'installer/util/shell_util_unittest.cc',
             'installer/util/wmi_unittest.cc',
             'installer/util/work_item_list_unittest.cc',
+            '<(SHARED_INTERMEDIATE_DIR)/chrome_version/other_version.rc',
           ],
           'msvs_settings': {
             'VCManifestTool': {
diff --git a/chrome/installer/test/alternate_version_generator.cc b/chrome/installer/test/alternate_version_generator.cc
index e748078..3d03ab77 100644
--- a/chrome/installer/test/alternate_version_generator.cc
+++ b/chrome/installer/test/alternate_version_generator.cc
@@ -39,6 +39,8 @@
 #include "base/path_service.h"
 #include "base/platform_file.h"
 #include "base/process_util.h"
+#include "base/string_util.h"
+#include "base/version.h"
 #include "base/win/pe_image.h"
 #include "base/win/scoped_handle.h"
 #include "chrome/installer/test/pe_image_resources.h"
@@ -103,6 +105,16 @@ class ChromeVersion {
     return ChromeVersion(static_cast<ULONGLONG>(high) << 32 |
                          static_cast<ULONGLONG>(low));
   }
+  static ChromeVersion FromString(const std::string& version_string) {
+    Version version(version_string);
+    DCHECK(version.IsValid());
+    const std::vector<uint16>& c(version.components());
+    return ChromeVersion(static_cast<ULONGLONG>(c[0]) << 48 |
+                         static_cast<ULONGLONG>(c[1]) << 32 |
+                         static_cast<ULONGLONG>(c[2]) << 16 |
+                         static_cast<ULONGLONG>(c[3]));
+  }
+
   ChromeVersion() { }
   explicit ChromeVersion(ULONGLONG value) : version_(value) { }
   WORD major() const { return static_cast<WORD>(version_ >> 48); }
@@ -128,6 +140,7 @@ std::wstring ChromeVersion::ToString() const {
   return std::wstring(&buffer[0], string_len);
 }
 
+
 // A read/write mapping of a file.
 // Note: base::MemoryMappedFile is not used because it doesn't support
 // read/write mappings.  Adding such support across all platforms for this
@@ -331,6 +344,11 @@ void VisitResource(const upgrade_test::EntryPath& path,
 // Updates the version strings and numbers in all of |image_file|'s resources.
 bool UpdateVersionIfMatch(const FilePath& image_file,
                           VisitResourceContext* context) {
+  if (!context ||
+      context->current_version_str.size() < context->new_version_str.size()) {
+    return false;
+  }
+
   bool result = false;
   base::win::ScopedHandle image_handle(base::CreatePlatformFile(
       image_file,
@@ -633,6 +651,29 @@ bool GenerateAlternateVersion(const FilePath& original_installer_path,
 bool GenerateAlternatePEFileVersion(const FilePath& original_file,
                                     const FilePath& target_file,
                                     Direction direction) {
+  VisitResourceContext ctx;
+  if (!GetFileVersion(original_file, &ctx.current_version)) {
+    LOG(DFATAL) << "Failed reading version from \"" << original_file.value()
+                << "\"";
+    return false;
+  }
+  ctx.current_version_str = ctx.current_version.ToString();
+
+  if (!IncrementNewVersion(direction, &ctx)) {
+    LOG(DFATAL) << "Failed to increment version from \""
+                << original_file.value() << "\"";
+    return false;
+  }
+
+  Version new_version(WideToASCII(ctx.new_version_str));
+  GenerateSpecificPEFileVersion(original_file, target_file, new_version);
+
+  return true;
+}
+
+bool GenerateSpecificPEFileVersion(const FilePath& original_file,
+                                   const FilePath& target_file,
+                                   const Version& version) {
   // First copy original_file to target_file.
   if (!file_util::CopyFile(original_file, target_file)) {
     LOG(DFATAL) << "Failed copying \"" << original_file.value()
@@ -647,15 +688,10 @@ bool GenerateAlternatePEFileVersion(const FilePath& original_file,
     return false;
   }
   ctx.current_version_str = ctx.current_version.ToString();
+  ctx.new_version = ChromeVersion::FromString(version.GetString());
+  ctx.new_version_str = ctx.new_version.ToString();
 
-  if (!IncrementNewVersion(direction, &ctx) ||
-      !UpdateVersionIfMatch(target_file, &ctx)) {
-    LOG(DFATAL) << "Failed to update version in \"" << target_file.value()
-                << "\"";
-    return false;
-  }
-
-  return true;
+  return UpdateVersionIfMatch(target_file, &ctx);
 }
 
 }  // namespace upgrade_test
diff --git a/chrome/installer/test/alternate_version_generator.h b/chrome/installer/test/alternate_version_generator.h
index 35dffb1..0ae0d5c 100644
--- a/chrome/installer/test/alternate_version_generator.h
+++ b/chrome/installer/test/alternate_version_generator.h
@@ -10,6 +10,7 @@
 #include <string>
 
 class FilePath;
+class Version;
 
 namespace upgrade_test {
 
@@ -33,10 +34,19 @@ bool GenerateAlternateVersion(const FilePath& original_installer_path,
 // Given a path to a PEImage in |original_file|, copy that file to
 // |target_file|, modifying the version of the copy according to |direction|.
 // Any previous file at |target_file| is clobbered. Returns true on success.
+// Note that |target_file| may still be mutated on failure.
 bool GenerateAlternatePEFileVersion(const FilePath& original_file,
                                     const FilePath& target_file,
                                     Direction direction);
 
+// Given a path to a PEImage in |original_file|, copy that file to
+// |target_file|, modifying the version of the copy according to |version|.
+// Any previous file at |target_file| is clobbered. Returns true on success.
+// Note that |target_file| may still be mutated on failure.
+bool GenerateSpecificPEFileVersion(const FilePath& original_file,
+                                   const FilePath& target_file,
+                                   const Version& version);
+
 }  // namespace upgrade_test
 
 #endif  // CHROME_INSTALLER_TEST_ALTERNATE_VERSION_GENERATOR_H_
diff --git a/chrome/installer/util/installer_state.cc b/chrome/installer/util/installer_state.cc
index e3d1396..b7ab42b 100644
--- a/chrome/installer/util/installer_state.cc
+++ b/chrome/installer/util/installer_state.cc
@@ -10,6 +10,7 @@
 
 #include "base/command_line.h"
 #include "base/file_util.h"
+#include "base/file_version_info.h"
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/string_util.h"
@@ -586,6 +587,28 @@ bool InstallerState::IsFileInUse(const FilePath& file) {
                                              OPEN_EXISTING, 0, 0)).IsValid();
 }
 
+
+void InstallerState::GetExistingExeVersions(
+    std::set<std::string>* existing_versions) const {
+
+  static const wchar_t* const kChromeFilenames[] = {
+    installer::kChromeExe,
+    installer::kChromeNewExe,
+    installer::kChromeOldExe,
+  };
+
+  for (int i = 0; i < arraysize(kChromeFilenames); ++i) {
+    FilePath chrome_exe(target_path().Append(kChromeFilenames[i]));
+    scoped_ptr<FileVersionInfo> file_version_info(
+        FileVersionInfo::CreateFileVersionInfo(chrome_exe));
+    if (file_version_info) {
+      string16 version_string = file_version_info->file_version();
+      if (!version_string.empty() && IsStringASCII(version_string))
+        existing_versions->insert(WideToASCII(version_string));
+    }
+  }
+}
+
 void InstallerState::RemoveOldVersionDirectories(
     const Version& new_version,
     Version* existing_version,
@@ -594,8 +617,16 @@ void InstallerState::RemoveOldVersionDirectories(
   std::vector<FilePath> key_files;
   scoped_ptr<WorkItem> item;
 
-  // Try to delete all directories whose versions are lower than latest_version
-  // and not equal to the existing version (opv).
+  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.
   file_util::FileEnumerator version_enum(target_path(), false,
       file_util::FileEnumerator::DIRECTORIES);
   for (FilePath next_version = version_enum.Next(); !next_version.empty();
@@ -605,26 +636,18 @@ void InstallerState::RemoveOldVersionDirectories(
     // 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() &&
-        version.CompareTo(new_version) < 0 &&
-        (existing_version == NULL || !version.Equals(*existing_version))) {
-      // Collect the key files (relative to the version dir) for all products.
-      key_files.clear();
-      std::for_each(products_.begin(), products_.end(),
-                    std::bind2nd(std::mem_fun(&Product::AddKeyFiles),
-                                 &key_files));
-      // Make the key_paths absolute.
-      const std::vector<FilePath>::iterator end = key_files.end();
-      for (std::vector<FilePath>::iterator scan = key_files.begin();
-           scan != end; ++scan) {
-        *scan = next_version.Append(*scan);
-      }
-
-      VLOG(1) << "Deleting old version directory: " << next_version.value();
-
-      item.reset(WorkItem::CreateDeleteTreeWorkItem(next_version, temp_path,
-                                                    key_files));
-      item->set_ignore_failure(true);
-      item->Do();
+        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 = file_util::Delete(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();
     }
   }
 }
diff --git a/chrome/installer/util/installer_state.h b/chrome/installer/util/installer_state.h
index e6636dbf..ca1bafe 100644
--- a/chrome/installer/util/installer_state.h
+++ b/chrome/installer/util/installer_state.h
@@ -5,6 +5,7 @@
 #ifndef CHROME_INSTALLER_UTIL_INSTALLER_STATE_H_
 #define CHROME_INSTALLER_UTIL_INSTALLER_STATE_H_
 
+#include <set>
 #include <string>
 #include <vector>
 
@@ -218,6 +219,11 @@ 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);
 
diff --git a/chrome/installer/util/installer_state_unittest.cc b/chrome/installer/util/installer_state_unittest.cc
index 316a052..b7e1bd0 100644
--- a/chrome/installer/util/installer_state_unittest.cc
+++ b/chrome/installer/util/installer_state_unittest.cc
@@ -8,6 +8,7 @@
 
 #include "base/base_paths.h"
 #include "base/command_line.h"
+#include "base/file_path.h"
 #include "base/file_util.h"
 #include "base/path_service.h"
 #include "base/process_util.h"
@@ -19,6 +20,7 @@
 #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"
@@ -27,6 +29,7 @@
 #include "chrome/installer/util/installer_state.h"
 #include "chrome/installer/util/master_preferences.h"
 #include "chrome/installer/util/product_unittest.h"
+#include "chrome/installer/util/util_constants.h"
 #include "chrome/installer/util/work_item.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -55,6 +58,9 @@ class MockInstallerState : public InstallerState {
   const 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.
@@ -489,6 +495,114 @@ 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";
+
+  Version new_version(kNewVersion);
+  Version old_version(kOldVersion);
+  Version old_chrome_exe_version(kOldChromeExeVersion);
+  Version chrome_exe_version(kChromeExeVersion);
+  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.
+  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().Append(ASCIIToWide(kOldVersion)),
+    installer_state.target_path().Append(ASCIIToWide(kOldChromeExeVersion)),
+    installer_state.target_path().Append(L"2.1.1.0"),
+    installer_state.target_path().Append(ASCIIToWide(kChromeExeVersion)),
+    installer_state.target_path().Append(ASCIIToWide(kNewVersion)),
+    installer_state.target_path().Append(L"3.9.1.1"),
+  };
+
+  // Create the version directories.
+  for (int i = 0; i < arraysize(version_dirs); i++) {
+    file_util::CreateDirectory(version_dirs[i]);
+    EXPECT_TRUE(file_util::PathExists(version_dirs[i]));
+  }
+
+  // Create exes with the appropriate version resource.
+  // Use the current test exe as a baseline.
+  FilePath exe_path;
+  ASSERT_TRUE(PathService::Get(base::FILE_EXE, &exe_path));
+
+  struct target_info {
+    FilePath target_file;
+    const 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 (int 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.
+  file_util::FileEnumerator version_enum(installer_state.target_path(), false,
+      file_util::FileEnumerator::DIRECTORIES);
+  for (FilePath next_version = version_enum.Next(); !next_version.empty();
+       next_version = version_enum.Next()) {
+    FilePath dir_name(next_version.BaseName());
+    Version version(WideToASCII(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;
+}
+
+
 // A fixture for testing InstallerState::DetermineCriticalVersion.  Individual
 // tests must invoke Initialize() with a critical version.
 class InstallerStateCriticalVersionTest : public ::testing::Test {