Drastically simplify old version cleanup and add some additional logg…

…ing.

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

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': {

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

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_

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();
     }
   }
 }

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);