Move JumplistIcon to JumplistIconOld only via renaming, not copy-n-delete

chrome/browser/win/jumplist.cc

Index: chrome/browser/win/jumplist.cc
diff --git a/chrome/browser/win/jumplist.cc b/chrome/browser/win/jumplist.cc
index 3c114eeab9aa26465d4946a0390a3ba229b22062..3ffddb28132bb73173603422d8243bb7297391bb 100644
--- a/chrome/browser/win/jumplist.cc
+++ b/chrome/browser/win/jumplist.cc
@@ -4,6 +4,9 @@
 
 #include "chrome/browser/win/jumplist.h"
 
+#include <Shlwapi.h>
+#include <windows.h>
+
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/callback_helpers.h"
@@ -15,6 +18,7 @@
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/threading/thread.h"
+#include "base/threading/thread_restrictions.h"
 #include "base/trace_event/trace_event.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/favicon/favicon_service_factory.h"
@@ -225,6 +229,22 @@ bool UpdateJumpList(const wchar_t* app_id,
   return true;
 }
 
+// Renames the directory |from_dir| to |to_path|. This method fails if any
+// process has a handle open in |from_dir| or if |to_path| exists. Base::Move()
+// tries to rename a file and if this fails, it tries copy-n-delete; This
+// RenameDirectory method only does the rename part.
+bool RenameDirectory(const base::FilePath& from_path,
+                     const base::FilePath& to_path) {
+  base::ThreadRestrictions::AssertIOAllowed();
+  if (from_path.ReferencesParent() || to_path.ReferencesParent())
+    return false;
+  if (from_path.value().length() >= MAX_PATH ||
+      to_path.value().length() >= MAX_PATH) {
+    return false;
+  }
+  return MoveFileEx(from_path.value().c_str(), to_path.value().c_str(), 0) != 0;
+}
+
 // Updates the jumplist, once all the data has been fetched.
 void RunUpdateOnFileThread(
     IncognitoModePrefs::Availability incognito_availability,
@@ -250,7 +270,8 @@ void RunUpdateOnFileThread(
   // Delete the directory which contains old icon files, rename the current
   // icon directory, and create a new directory which contains new JumpList
   // icon files.
-  base::FilePath icon_dir_old(icon_dir.value() + L"Old");
+  base::FilePath icon_dir_old = icon_dir.DirName().Append(
+      icon_dir.BaseName().value() + FILE_PATH_LITERAL("Old"));
 
   enum FolderOperationResult {
     SUCCESS = 0,
@@ -274,25 +295,28 @@ void RunUpdateOnFileThread(
       },
       base::Unretained(&folder_operation_status)));
 
-  // If deletion of |icon_dir_old| fails, do not move |icon_dir| to
+  // If deletion of |icon_dir_old| fails, do not rename |icon_dir| to
   // |icon_dir_old|, instead, delete |icon_dir| directly to avoid bloating
   // |icon_dir_old| by moving more things to it.
   if (!base::DeleteFile(icon_dir_old, true)) {
     folder_operation_status |= FolderOperationResult::DELETE_DEST_FAILED;
-    // If deletion of |icon_dir| fails, exit early. This skips creating the same
-    // directory and updating jumplist icons to avoid bloating the JumplistIcons
-    // folder.
+    // If deletion of any item in |icon_dir| fails, exit early. If deletion of
+    // all the items succeeds while only deletion of the dir fails, it is okay
+    // to proceed. This skips creating the same directory and updating jumplist
+    // icons to avoid bloating the JumplistIcons folder.
     if (!base::DeleteFile(icon_dir, true)) {
       folder_operation_status |= FolderOperationResult::DELETE_SRC_FAILED;
-      return;
+      if (!::PathIsDirectoryEmpty(icon_dir.value().c_str()))

[grt (UTC plus 2), 2017/03/10 09:24:06]

should you add something else to folder_operation_status for the "else" clause
of this so you can see how often it happens? same q below on line 318.

[chengx, 2017/03/10 21:49:40]

Sure, I have updated enum FolderOperationResult and folder_operation_status
accordingly.

+        return;
     }
-  } else if (!base::Move(icon_dir, icon_dir_old)) {
+  } else if (!RenameDirectory(icon_dir, icon_dir_old)) {
     folder_operation_status |= FolderOperationResult::MOVE_FAILED;
-    // If Move() fails, delete |icon_dir| to avoid file accumulation in this
-    // directory, which can eventually lead the folder to be huge.
+    // If RenameDirectory() fails, delete |icon_dir| to avoid file accumulation
+    // in this directory, which can eventually lead the folder to be huge.
     if (!base::DeleteFile(icon_dir, true)) {
       folder_operation_status |= FolderOperationResult::DELETE_SRC_FAILED;
-      return;
+      if (!::PathIsDirectoryEmpty(icon_dir.value().c_str()))
+        return;
     }
   }