Remove JumpListIconsOld directory and set upper limit for delete attempts

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 680db16a6a490256bf10dd17a699b6806db5152a..c6c53d7197a2d63a5567f4d7534c70e7068b07d1 100644
--- a/chrome/browser/win/jumplist.cc
+++ b/chrome/browser/win/jumplist.cc
@@ -11,12 +11,14 @@
 #include "base/bind_helpers.h"
 #include "base/callback_helpers.h"
 #include "base/command_line.h"
+#include "base/files/file_enumerator.h"
 #include "base/files/file_util.h"
 #include "base/macros.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/path_service.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/task_scheduler/post_task.h"
 #include "base/threading/thread.h"
 #include "base/threading/thread_restrictions.h"
 #include "base/trace_event/trace_event.h"
@@ -63,6 +65,9 @@ namespace {
 // Delay jumplist updates to allow collapsing of redundant update requests.
 const int kDelayForJumplistUpdateInMS = 3500;
 
+// Maximum number of icon files allowed to delete per update
+const int kMaxIconFilesDeletedPerUpdate = 100;
+
 // Append the common switches to each shell link.
 void AppendCommonSwitches(ShellLinkItem* shell_link) {
   const char* kSwitchNames[] = { switches::kUserDataDir };
@@ -229,20 +234,192 @@ 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) {
+// Folder delete status enumeration, used in Delete* methods below.
+// This is used for UMA. Do not delete entries, and keep in sync with
+// histograms.xml.
+enum FolderDeleteResult {
+  SUCCEED = 0,
+  // File name's length exceeds MAX_PATH. This won't happen.
+  FAIL_INVALID_FILE_PATH,
+  // JumpListIcons{,Old} directories are read-only. This may heppen.
+  FAIL_READ_ONLY_DIRECTORY,
+  // Since JumpListIcons{,Old} are directories, this won't happen.
+  FAIL_DELETE_SINGLE_FILE,
+  // Delete maximum files allowed succeeds. However, in the process of deleting
+  // these files, it fails to delete some other files. This may happen.
+  FAIL_DELETE_MAX_FILE_PERFECTLY,
+  // Fail to delete maximum files allowed when the maximum attempts allowed
+  // have been used. This may heppen.
+  FAIL_DELETE_MAX_FILES_ANYWAY,
+  // Add new items before this one, always keep this one at the end.
+  END
+};
+
+// This method is similar to base::DeleteFileRecursive in
+// file_util_win.cc with the following differences.
+// 1) It forces deleting files recursively.
+// 2) It has a boolean input parameter |has_upper_limit|. When it is set to
+//    true, this method returns false when the files deleted exceeds
+//    |kMaxIconFilesDeletedPerUpdate| or the files failed to be deleted exceeds
+//    |kMaxIconFilesDeletedPerUpdate| per directory (e.g., |path| if it is
+//    a directory or any of its sub-directories). When it is set to false, all
+//    files under |path| are deleted.
+// 3) Failure cause is recored in |delete_status|.
+bool DeleteFileRecursive(const base::FilePath& path,

[grt (UTC plus 2), 2017/03/21 08:40:26]

if this is only called to do recursive deletes on directories, then i think
DeleteDirectoryRecursive is more clear.

[chengx, 2017/03/22 06:46:28]

Done. Renamed to "DeleteDirectoryRecursive".

+                         const base::FilePath::StringType& pattern,
+                         bool has_upper_limit,
+                         int* delete_status) {

[grt (UTC plus 2), 2017/03/21 08:40:27]

int* -> FolderDeleteResult*

[chengx, 2017/03/22 06:46:27]

Done.

+  base::FileEnumerator traversal(
+      path, false,
+      base::FileEnumerator::FILES | base::FileEnumerator::DIRECTORIES, pattern);
+  int file_deleted = 0, file_fail_to_deted = 0;

[grt (UTC plus 2), 2017/03/21 08:40:26]

file_fail_to_deted -> failure_count

[grt (UTC plus 2), 2017/03/21 08:40:27]

file_deleted -> attempt_count

[grt (UTC plus 2), 2017/03/21 08:40:27]

nit: one definition per line, please

[chengx, 2017/03/22 06:46:28]

I will rename it to |success_count|. I think if we set an upper limit for
|success_count|, we will know that 100 files are deleted if the method returns
true. However, if we set upper limit for |attempt_count|, we won't know how many
files are deleted even when the method returns true, since we don't know what
the |failure_count| is.

[chengx, 2017/03/22 06:46:28]

Done.

[chengx, 2017/03/22 06:46:28]

Done.

+  for (base::FilePath current = traversal.Next(); !current.empty();
+       current = traversal.Next(), file_deleted++) {
+    // Try to clear the read-only bit if we find it.
+    base::FileEnumerator::FileInfo info = traversal.GetInfo();
+    if (info.find_data().dwFileAttributes & FILE_ATTRIBUTE_READONLY) {
+      SetFileAttributes(
+          current.value().c_str(),
+          info.find_data().dwFileAttributes & ~FILE_ATTRIBUTE_READONLY);
+    }
+
+    if (info.IsDirectory()) {
+      // If DeleteFileRecursive or RemoveDirectory fails, return false
+      // intermediately.
+      if (!DeleteFileRecursive(current, pattern, has_upper_limit,
+                               delete_status) ||
+          !::RemoveDirectory(current.value().c_str())) {
+        return false;
+      }
+    } else if (!::DeleteFile(current.value().c_str())) {
+      // If there is no upper limit, return false intermediately. Otherwise,
+      // increment |file_fail_to_deted| until it hits the upper limit.
+      if (!has_upper_limit)
+        return false;
+      file_fail_to_deted++;

[grt (UTC plus 2), 2017/03/21 08:40:27]

since you don't decrement file_deleted here, it's really a count of the # of
attempts rather than the number of successes, so the doc comment is incorrect.
also, what do you think about counting the # of items overall rather than just
the # of files?

also, what does the status mean when you have nested directories? if the max #
of files are deleted three levels deep, should the iteration in the dirs above
continue or not?

[chengx, 2017/03/22 06:46:27]

You are right. It's actually a count of the # of attempts. I have fixed the code
to record the count of the # of successful deletes. 

Also, since there shouldn't be sub-directories in JumpListIcons{,Old}
directories, I think in this CL we'd better just not do recursive deletion. On
the other hand, if there are sub-directories, then we have bigger troubles as
the maximum # of icon files is not 65536 anymore. Instead, it can be much bigger
than that. 

So I would propose to just delete maximum files (not dirs) allowed. If a sub-dir
is detected, then we use |delete_status| to record that sub-dir does exist. We
don't try to delete the stb-dir, and we don't exit the method right away.
Instead, we just increment |failure_count|. After gathering the information from
UMA using this CL, we will know if sub-dir exists and come up with possible
solutions for that.

+    }
+    // If it successfully deletes |kMaxIconFilesDeletedPerUpdate| files, return
+    // true. If some files fail to be delete, record this information in
+    // |delete_status|.
+    if (has_upper_limit && file_deleted > kMaxIconFilesDeletedPerUpdate) {

[grt (UTC plus 2), 2017/03/21 08:40:26]

? file_deleted >= kMaxIconFilesDeletedPerUpdate ?

[chengx, 2017/03/22 06:46:28]

Done.

+      if (file_fail_to_deted > 0)
+        *delete_status = FAIL_DELETE_MAX_FILE_PERFECTLY;
+      return true;
+    }
+    // Return false when failing to delete |kMaxIconFilesDeletedPerUpdate| files
+    // comes before successfully deleting |kMaxIconFilesDeletedPerUpdate| files.
+    if (has_upper_limit && file_fail_to_deted > kMaxIconFilesDeletedPerUpdate) {
+      *delete_status = FAIL_DELETE_MAX_FILES_ANYWAY;
+      return false;
+    }
+  }
+  return true;
+}
+
+// This method is similar to base::DeleteFile in file_util_win.cc
+// with the following differences.
+// 1) It forces deleting files recursively.
+// 2) If |path| is a directory, it won't be deleted even if all its contents are
+//    deleted successfully.
+// 3) It has a boolean input parameter |has_upper_limit|. When it is set to
+//    true, this method returns false when the files deleted exceeds
+//    |kMaxIconFilesDeletedPerUpdate| or the files fail to be deleted exceeds
+//    |kMaxIconFilesDeletedPerUpdate| per directory (e.g., under path if it is
+//    a directory or any of its sub-directory). When it is set to false, all
+//    files under path are trying to be deleted.
+// 4) Failure cause is recored in |delete_status|.
+bool DeleteDirectoryContent(const base::FilePath& path,
+                            bool has_upper_limit,
+                            int* delete_status) {

[grt (UTC plus 2), 2017/03/21 08:40:26]

int* -> FolderDeleteResult*

[chengx, 2017/03/22 06:46:27]

Done.

   base::ThreadRestrictions::AssertIOAllowed();
-  if (from_path.ReferencesParent() || to_path.ReferencesParent())
+
+  if (path.empty())
+    return true;
+
+  if (path.value().length() >= MAX_PATH) {
+    *delete_status = FAIL_INVALID_FILE_PATH;
     return false;
-  if (from_path.value().length() >= MAX_PATH ||
-      to_path.value().length() >= MAX_PATH) {
+  }
+
+  DWORD attr = GetFileAttributes(path.value().c_str());
+  // We're done if we can't find the path.
+  if (attr == INVALID_FILE_ATTRIBUTES)
+    return true;
+  // We may need to clear the read-only bit.
+  if ((attr & FILE_ATTRIBUTE_READONLY) &&
+      !SetFileAttributes(path.value().c_str(),
+                         attr & ~FILE_ATTRIBUTE_READONLY)) {
+    *delete_status = FAIL_READ_ONLY_DIRECTORY;
     return false;
   }
-  return MoveFileEx(from_path.value().c_str(), to_path.value().c_str(), 0) != 0;
+  // If |path| is a file, simply delete it.

[grt (UTC plus 2), 2017/03/21 08:40:26]

this is never expected to be hit, but i agree that it makes sense for this to be
here. please add a comment mentioning that it's unexpected.

[chengx, 2017/03/22 06:46:27]

Done.

+  if (!(attr & FILE_ATTRIBUTE_DIRECTORY)) {
+    bool delete_result = !!::DeleteFile(path.value().c_str());
+    if (!delete_result)
+      *delete_status = FAIL_DELETE_SINGLE_FILE;
+    return delete_result;
+  }
+  // If |path| is a directory, delete all its content but save the raw
+  // directory.
+  return DeleteFileRecursive(path, L"*", has_upper_limit, delete_status);
+}
+
+// This method is similar to base::DeleteFile in file_util_win.cc
+// with the following differences.
+// 1) It forces deleting files recursively.
+// 2) It runs in the asynchronous manner, so there is no return value.
+// 3) It has a boolean input parameter |has_upper_limit|. When it is set to
+//    true, this method returns false when the files deleted exceeds
+//    |kMaxIconFilesDeletedPerUpdate| or the files fail to be deleted exceeds
+//    |kMaxIconFilesDeletedPerUpdate| per directory (e.g., under path if it is
+//    a directory or any of its sub-directory). When it is set to false, all
+//    files under path are trying to be deleted.
+// 4) Failure cause is recored in |delete_status|.
+void DeleteDirectory(const base::FilePath& path,
+                     bool has_upper_limit,
+                     int* delete_status) {

[grt (UTC plus 2), 2017/03/21 08:40:26]

int* -> FolderDeleteResult*

[chengx, 2017/03/22 06:46:28]

Done.

+  base::ThreadRestrictions::AssertIOAllowed();
+
+  if (path.empty())
+    return;
+
+  if (path.value().length() >= MAX_PATH) {
+    *delete_status = FAIL_INVALID_FILE_PATH;
+    return;
+  }
+
+  DWORD attr = GetFileAttributes(path.value().c_str());
+  // We're done if we can't find the path.
+  if (attr == INVALID_FILE_ATTRIBUTES)
+    return;
+  // We may need to clear the read-only bit.
+  if ((attr & FILE_ATTRIBUTE_READONLY) &&
+      !SetFileAttributes(path.value().c_str(),
+                         attr & ~FILE_ATTRIBUTE_READONLY)) {
+    *delete_status = FAIL_READ_ONLY_DIRECTORY;
+    return;
+  }
+  // If |path| is a file, delete it.
+  if (!(attr & FILE_ATTRIBUTE_DIRECTORY)) {
+    ::DeleteFile(path.value().c_str());
+    *delete_status = FAIL_DELETE_SINGLE_FILE;
+    return;
+  }
+  // If |path| is a directory, delete its contents and if it succeeds, remove
+  // the raw directory.
+  if (DeleteFileRecursive(path, L"*", has_upper_limit, delete_status))
+    ::RemoveDirectory(path.value().c_str());
+  return;
+}
+
+// Call DeleteDirectory method and then log the delete status using
+// UMA_HISTOGRAM_ENUMERATION.

[Ilya Sherman, 2017/03/21 20:56:44]

This comment literally just repeats the code, which is not useful.  For a brief
function comment like this, I'd recommend something like "Deletes the directory
at |path| and records the result to UMA."  (Admittedly that's still pretty much
literally repeating the code, but at least it rephrases it into something
slightly more human-natural-language-friendly.  In general, I'm not a huge fan
of doc strings for simple internal methods like these, but it does seem to be
the prevailing Chromium style to include them.)

[chengx, 2017/03/22 06:46:27]

Done.

+void DeleteDirectoryAndLogResults(const base::FilePath& path,
+                                  bool has_upper_limit,
+                                  int* delete_status,
+                                  std::string histogram_name) {

[Ilya Sherman, 2017/03/21 20:56:45]

Why are you passing this string by copy?

[chengx, 2017/03/22 06:46:28]

This parameter is removed in the new patch set.

+  DeleteDirectory(path, has_upper_limit, delete_status);
+  UMA_HISTOGRAM_ENUMERATION(histogram_name, *delete_status, END);

[grt (UTC plus 2), 2017/03/21 08:40:26]

the UMA macros cannot be used with a generated histogram name like this; see
https://cs.chromium.org/chromium/src/base/metrics/histogram_macros.h?type=cs&....
you'll need to use base::LinearHistogram::FactoryGet and Add the sample
manually. alternatively, use "WinJumplist.DeleteStatusJumpListIconsOld" directly
here.

[Ilya Sherman, 2017/03/21 20:56:44]

You can also use UmaHistogramEnumeration() from histogram_functions.h.  But,
yes, if this method is only used to record a single histogram, then there's no
reason to pass in the name as a parameter.

[chengx, 2017/03/22 06:46:27]

The name parameter is removed.

[chengx, 2017/03/22 06:46:28]

I have changed to use "WinJumplist.DeleteStatusJumpListIconsOld" directly.

 }
 
 // Updates the jumplist, once all the data has been fetched.
@@ -267,73 +444,18 @@ void RunUpdateOnFileThread(
     local_recently_closed_pages = data->recently_closed_pages_;
   }
 
-  // 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.DirName().Append(
-      icon_dir.BaseName().value() + FILE_PATH_LITERAL("Old"));
-
-  enum FolderOperationResult {
-    SUCCESS = 0,
-    DELETE_DEST_FAILED = 1 << 0,
-    RENAME_FAILED = 1 << 1,
-    DELETE_SRC_CONTENT_FAILED = 1 << 2,
-    DELETE_SRC_DIR_FAILED = 1 << 3,
-    CREATE_SRC_FAILED = 1 << 4,
-    // This value is beyond the sum of all bit fields above and
-    // should remain last (shifted by one more than the last value)
-    END = 1 << 5
-  };
-
-  // This variable records the status of three folder operations.
-  uint32_t folder_operation_status = FolderOperationResult::SUCCESS;
+  // This variable records the delete status of folder JumpListIcons.
+  int folder_delete_status = SUCCEED;

[grt (UTC plus 2), 2017/03/21 08:40:26]

int -> FolderDeleteResult

[chengx, 2017/03/22 06:46:28]

Done.

 
   base::ScopedClosureRunner log_operation_status_when_done(base::Bind(
-      [](uint32_t* folder_operation_status_ptr) {
-        UMA_HISTOGRAM_ENUMERATION(
-            "WinJumplist.DetailedFolderResultsDeleteUpdated",
-            *folder_operation_status_ptr, FolderOperationResult::END);
+      [](int* folder_delete_status_ptr) {
+        UMA_HISTOGRAM_ENUMERATION("WinJumplist.DeleteStatusJumpListIcons",
+                                  *folder_delete_status_ptr, END);
       },
-      base::Unretained(&folder_operation_status)));
-
-  // 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 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)) {
-      if (!::PathIsDirectoryEmpty(icon_dir.value().c_str())) {
-        folder_operation_status |=
-            FolderOperationResult::DELETE_SRC_CONTENT_FAILED;
-        return;
-      }
-      folder_operation_status |= FolderOperationResult::DELETE_SRC_DIR_FAILED;
-    }
-  } else if (!RenameDirectory(icon_dir, icon_dir_old)) {
-    // If RenameDirectory() fails, delete |icon_dir| to avoid file accumulation
-    // in this directory, which can eventually lead the folder to be huge.
-    folder_operation_status |= FolderOperationResult::RENAME_FAILED;
-    // 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)) {
-      if (!::PathIsDirectoryEmpty(icon_dir.value().c_str())) {
-        folder_operation_status |=
-            FolderOperationResult::DELETE_SRC_CONTENT_FAILED;
-        return;
-      }
-      folder_operation_status |= FolderOperationResult::DELETE_SRC_DIR_FAILED;
-    }
-  }
+      base::Unretained(&folder_delete_status)));
 
-  // If CreateDirectory() fails, exit early.
-  if (!base::CreateDirectory(icon_dir)) {
-    folder_operation_status |= FolderOperationResult::CREATE_SRC_FAILED;
+  // If failing to delete the content in |icon_dir|, exit early.
+  if (!DeleteDirectoryContent(icon_dir, true, &folder_delete_status)) {
     return;
   }
 
@@ -349,6 +471,25 @@ void RunUpdateOnFileThread(
   // with it.
   UpdateJumpList(app_id.c_str(), local_most_visited_pages,
                  local_recently_closed_pages, incognito_availability);
+
+  // Post a background task to delete JumpListIconsOld folder if it exists and
+  // log the delete results to UMA.
+  base::FilePath icon_dir_old = icon_dir.DirName().Append(
+      icon_dir.BaseName().value() + FILE_PATH_LITERAL("Old"));
+
+  if (::PathFileExists(icon_dir_old.value().c_str())) {
+    int jumplisticons_old_delete_status = SUCCEED;

[grt (UTC plus 2), 2017/03/21 08:40:26]

move the definition of this |delete_status| variable into
DeleteDirectoryAndLogResults.

[chengx, 2017/03/22 06:46:28]

Done.

+    std::string histogram_name = "WinJumplist.DeleteStatusJumpListIconsOld";
+    base::PostTaskWithTraits(

[grt (UTC plus 2), 2017/03/21 08:40:26]

since a new task is posted for each jumplist icon update, you may end up with
multiple tasks running in parallel. you can avoid this by using
base::CreateSequencedTaskRunnerWithTraits once to create a SequencedTaskRunner,
then posting this task to that runner each time. there will still be N cleanup
jobs for N jumplist icon updates, but they will always run in sequence. perhaps
JumpList can create this SequencedTaskRunner in its ctor and pass a reference to
it to this function.

[chengx, 2017/03/22 06:46:28]

Thanks for the suggestion. I have added a member nameed |sequenced_task_runner_|
to JumpList class, initialized in JumpList class's constructor, and passed its
const ref to this method. 

+        FROM_HERE,
+        base::TaskTraits()
+            .WithPriority(base::TaskPriority::BACKGROUND)
+            .WithShutdownBehavior(
+                base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN)
+            .MayBlock(),
+        base::Bind(&DeleteDirectoryAndLogResults, icon_dir_old, true,
+                   &jumplisticons_old_delete_status, histogram_name));
+  }
 }
 
 }  // namespace