Errors while deleting old version dirs no longer cause them to grow in odd ways.

InstallerState::RemoveOldVersionDirectories now tells its DeleteTreeWorkItem instances to ignore failures.  Deleting the old version dirs is a best-effort thing.  Should one fail to be deletable on account of a file or directory being in use (other than the key files: chrome.dll, npchrome_frame.dll, and chrome_frame_helper.exe), the installer will no go on its merry way rather than trying to un-do the delete (which is error-prone to say the least).

Also, during installation, don't make success of the install contingent on being able to delete old_chrome.exe.  Basically for the same reasons.  If we can't delete it because it's in use by someone/thing, just leave it there.  It'll be cleaned up on a later install, or when new_chrome.exe is later swapped into place.  This may reduce our INSTALL_FAILED errors.

BUG=100218
TEST=see bug

Review URL: http://codereview.chromium.org/8299008

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

3 files changed, 53 insertions, 50 deletions

diff --git a/chrome/installer/util/delete_tree_work_item.cc b/chrome/installer/util/delete_tree_work_item.cc
index a5bc7f92..812c1d0 100644
--- a/chrome/installer/util/delete_tree_work_item.cc
+++ b/chrome/installer/util/delete_tree_work_item.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2009 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 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.
 
@@ -32,7 +32,8 @@ DeleteTreeWorkItem::DeleteTreeWorkItem(
     const FilePath& temp_path,
     const std::vector<FilePath>& key_paths)
     : root_path_(root_path),
-      temp_path_(temp_path) {
+      temp_path_(temp_path),
+      copied_to_backup_(false) {
   if (!SafeCast(key_paths.size(), &num_key_files_)) {
     NOTREACHED() << "Impossibly large key_paths collection";
   } else if (num_key_files_ != 0) {
@@ -57,15 +58,19 @@ bool DeleteTreeWorkItem::Do() {
   for (ptrdiff_t i = 0; !abort && i != num_key_files_; ++i) {
     FilePath& key_file = key_paths_[i];
     ScopedTempDir& backup = key_backup_paths_[i];
-    if (!backup.CreateUniqueTempDirUnderPath(temp_path_)) {
-      PLOG(ERROR) << "Could not create temp dir in " << temp_path_.value();
-      abort = true;
-    } else if (!file_util::CopyFile(key_file,
-                   backup.path().Append(key_file.BaseName()))) {
-      PLOG(ERROR) << "Could not back up " << key_file.value()
-                  << " to directory " << backup.path().value();
-      abort = true;
-    } else {
+    if (!ignore_failure_) {
+      if (!backup.CreateUniqueTempDirUnderPath(temp_path_)) {
+        PLOG(ERROR) << "Could not create temp dir in " << temp_path_.value();
+        abort = true;
+      } else if (!file_util::CopyFile(key_file,
+                     backup.path().Append(key_file.BaseName()))) {
+        PLOG(ERROR) << "Could not back up " << key_file.value()
+                    << " to directory " << backup.path().value();
+        abort = true;
+        backup.Delete();
+      }
+    }
+    if (!abort) {
       HANDLE file = ::CreateFile(key_file.value().c_str(), FILE_ALL_ACCESS,
                                  FILE_SHARE_DELETE, NULL, OPEN_EXISTING, 0,
                                  NULL);
@@ -115,6 +120,8 @@ bool DeleteTreeWorkItem::Do() {
           LOG(ERROR) << "can not copy " << root_path_.value()
                      << " to backup path " << backup.value();
           return false;
+        } else {
+          copied_to_backup_ = true;
         }
       }
     }
@@ -132,7 +139,8 @@ void DeleteTreeWorkItem::Rollback() {
   if (ignore_failure_)
     return;
 
-  if (!backup_path_.path().empty()) {
+  if (copied_to_backup_) {
+    DCHECK(!backup_path_.path().empty());
     FilePath backup = backup_path_.path().Append(root_path_.BaseName());
     if (file_util::PathExists(backup))
       file_util::Move(backup, root_path_);
diff --git a/chrome/installer/util/delete_tree_work_item.h b/chrome/installer/util/delete_tree_work_item.h
index ab604cc..0e44495 100644
--- a/chrome/installer/util/delete_tree_work_item.h
+++ b/chrome/installer/util/delete_tree_work_item.h
@@ -53,6 +53,9 @@ class DeleteTreeWorkItem : public WorkItem {
 
   // The temporary directory into which the original root_path_ has been moved.
   ScopedTempDir backup_path_;
+
+  // Set to true once root_path_ has been copied into backup_path_.
+  bool copied_to_backup_;
 };
 
 #endif  // CHROME_INSTALLER_UTIL_DELETE_TREE_WORK_ITEM_H_
diff --git a/chrome/installer/util/installer_state.cc b/chrome/installer/util/installer_state.cc
index ba0740d..bc365de 100644
--- a/chrome/installer/util/installer_state.cc
+++ b/chrome/installer/util/installer_state.cc
@@ -448,50 +448,42 @@ void InstallerState::RemoveOldVersionDirectories(
     const Version& new_version,
     Version* existing_version,
     const FilePath& temp_path) const {
-  file_util::FileEnumerator version_enum(target_path(), false,
-      file_util::FileEnumerator::DIRECTORIES);
   scoped_ptr<Version> version;
   std::vector<FilePath> key_files;
+  scoped_ptr<WorkItem> item;
 
-  // We try to delete all directories whose versions are lower than
-  // latest_version.
-  FilePath next_version = version_enum.Next();
-  while (!next_version.empty()) {
-    file_util::FileEnumerator::FindInfo find_data = {0};
-    version_enum.GetFindInfo(&find_data);
-    VLOG(1) << "directory found: " << find_data.cFileName;
-    version.reset(Version::GetVersionFromString(
-                      WideToASCII(find_data.cFileName)));
-    if (version.get()) {
-      // 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->CompareTo(new_version) < 0 &&
-          (existing_version == NULL ||
-           version->CompareTo(*existing_version) != 0)) {
-        key_files.clear();
-        std::for_each(products_.begin(), products_.end(),
-                      std::bind2nd(std::mem_fun(&Product::AddKeyFiles),
-                                   &key_files));
-        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);
-        }
+  // Try to delete all directories whose versions are lower than latest_version
+  // and not equal to the existing version (opv).
+  file_util::FileEnumerator version_enum(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.reset(Version::GetVersionFromString(WideToASCII(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.get() &&
+        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 directory: " << next_version.value();
+      VLOG(1) << "Deleting old version directory: " << next_version.value();
 
-        scoped_ptr<WorkItem> item(
-            WorkItem::CreateDeleteTreeWorkItem(next_version, temp_path,
-                                               key_files));
-        if (!item->Do()) {
-          LOG(ERROR) << "Failed to delete old version directory: "
-                     << next_version.value();
-          item->Rollback();
-        }
-      }
+      item.reset(WorkItem::CreateDeleteTreeWorkItem(next_version, temp_path,
+                                                    key_files));
+      item->set_ignore_failure(true);
+      item->Do();
     }
-
-    next_version = version_enum.Next();
   }
 }