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..dc25e7ee35b6eb02f11e3d58cf295574f16dc2a3 100644
--- a/chrome/browser/win/jumplist.cc
+++ b/chrome/browser/win/jumplist.cc
@@ -11,6 +11,7 @@
 #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"
@@ -63,6 +64,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 +233,115 @@ 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) {
+// This method is an exact copy of base::DeleteFileRecursive except that it has
+// an upper limit of file numbers to delete each time.
+// Deletes all files and directories in a path under the limit.
+// Returns false on the first failure it encounters, or the number of files
+// deleted exceeds the upper limit |upper_limit|.
+bool DeleteFileRecursiveWithUpperLimit(
+    const base::FilePath& path,
+    const base::FilePath::StringType& pattern,
+    bool recursive,
+    int upper_limit = INT_MAX) {

[grt (UTC plus 2), 2017/03/20 09:15:36]

it's confusing for a caller to invoke DeleteFileRecursiveWithUpperLimit with
three args, since "WithUpperLimit" is meaningless in that case. can you do
something other than the default arg here?

[chengx, 2017/03/21 01:37:10]

I renamed this method to DeleteFileRecursive, removed input parameter
|upper_limit|, but added a boolean input parameter |has_upper_limit|. If it is
set to true, the upper limit is set to |kMaxIconFilesDeletedPerUpdate|. Since
these methods are only used here, I think not over-generalizing them is
preferred as you suggested.

+  base::FileEnumerator traversal(
+      path, false,
+      base::FileEnumerator::FILES | base::FileEnumerator::DIRECTORIES, pattern);
+  int file_deleted = 0;
+  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) &&
+        (recursive || !info.IsDirectory())) {
+      SetFileAttributes(
+          current.value().c_str(),
+          info.find_data().dwFileAttributes & ~FILE_ATTRIBUTE_READONLY);
+    }
+
+    if (info.IsDirectory()) {
+      if (recursive && (!DeleteFileRecursiveWithUpperLimit(current, pattern,

[grt (UTC plus 2), 2017/03/20 09:15:36]

|file_deleted| should not be incremented if IsDirectory && !recursive since
nothing is deleted, no?

[chengx, 2017/03/21 01:37:10]

I have removed the |recursive| input parameter, so now (IsDirectory &&
!recursive) is always false and the condition you mentioned is gone.

+                                                           true, upper_limit) ||
+                        !::RemoveDirectory(current.value().c_str())))
+        return false;

[grt (UTC plus 2), 2017/03/20 09:15:37]

nit: braces around this since the conditional spans multiple lines

[chengx, 2017/03/21 01:37:11]

Done.

+    } else if (!::DeleteFile(current.value().c_str())) {
+      return false;

[grt (UTC plus 2), 2017/03/20 09:15:37]

why not keep going trying to delete other files in this case (without bumping
|file_deleted|)? should this not be a best-effort delete? i think it makes sense
to return false if not all files were deleted, but is there a reason to bail out
on the first failure?

[chengx, 2017/03/21 01:37:10]

I think it just made sense if it returns false when deletion of any file fails.
If it returned true after 100 files are deleted, there was no way to know if
there was a failure in deleting files at all. Now since I have recorded all the
delete status, not returning false right away should not lose any information. I
have updated the logic that when there is an upper limit set for deletion, not
return intermediately but keep trying deleting files until the upper limit is
hit.

+    }
+    if (file_deleted > upper_limit)
+      return false;
+  }
+  return true;
+}
+
+// This method deletes a file or content of a directory.
+// When deleting the content of a directory, |upper_limit| can be specified to
+// to set up the maximum number of files in this directory and any of its
+// sub-directory.

[grt (UTC plus 2), 2017/03/20 09:15:36]

are there ever sub-directories in JumplistIcons{,Old}?

[chengx, 2017/03/21 01:37:10]

There should not be sub-dir in JumplistIcos{,Old} according to the code logic,
and I have not seen users reported this.

I added "... any of its sub-directory." above for the method alone as it does
handle subdirectories, not for JumplistIcons folders.

+bool DeleteDirectoryContentWithUpperLimit(const base::FilePath& path,
+                                          bool recursive,

[grt (UTC plus 2), 2017/03/20 09:15:36]

don't overly generalize this -- the only caller passes |recursive| = true, so
make the function do exactly that -- a recursive delete.

[chengx, 2017/03/21 01:37:10]

Sure, the input parameter |recursive| is gone now. I tend to keep
|has_upper_limit| although currently all the calls set it to true. I will use it
soon I think.

+                                          int upper_limit = INT_MAX) {
   base::ThreadRestrictions::AssertIOAllowed();
-  if (from_path.ReferencesParent() || to_path.ReferencesParent())
+
+  if (path.empty())
+    return true;
+
+  if (path.value().length() >= MAX_PATH)
     return false;
-  if (from_path.value().length() >= MAX_PATH ||
-      to_path.value().length() >= MAX_PATH) {
+
+  // Handle any path with wildcards.

[grt (UTC plus 2), 2017/03/20 09:15:36]

remove wildcard support here -- it's unused

[chengx, 2017/03/21 01:37:10]

Done.

+  if (path.BaseName().value().find_first_of(L"*?") !=
+      base::FilePath::StringType::npos) {
+    return DeleteFileRecursiveWithUpperLimit(
+        path.DirName(), path.BaseName().value(), recursive, upper_limit);
+  }
+  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)) {
     return false;
   }
-  return MoveFileEx(from_path.value().c_str(), to_path.value().c_str(), 0) != 0;
+  // If |path| is a file, simply delete it.
+  if (!(attr & FILE_ATTRIBUTE_DIRECTORY))
+    return !!::DeleteFile(path.value().c_str());
+  // If |path| is a directory and |recursive| is true, delete all its content
+  // but save the row directory. The raw directory is deleted if

[grt (UTC plus 2), 2017/03/20 09:15:37]

row -> raw

[chengx, 2017/03/21 01:37:11]

Done. Sorry for the typo.

+  // |remove_directory| is true.
+  return !recursive ||
+         DeleteFileRecursiveWithUpperLimit(path, L"*", true, upper_limit);
+}
+
+// This method deletes a file or all the content of a directory depending on
+// what |path| is. There method is run in the asynchronous manner, so there is
+// return value.
+void DeleteDirectory(const base::FilePath& path, bool remove_directory = true) {
+  base::ThreadRestrictions::AssertIOAllowed();
+
+  if (path.empty() || path.value().length() >= MAX_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)) {
+    return;
+  }
+  // If |path| is a file, delete it.
+  if (!(attr & FILE_ATTRIBUTE_DIRECTORY)) {
+    ::DeleteFile(path.value().c_str());
+    return;
+  }
+  // If |path| is a directory, delete all its content. The raw directory is
+  // deleted if |remove_directory| is true.
+  if (DeleteFileRecursiveWithUpperLimit(path, L"*", true) && remove_directory)
+    ::RemoveDirectory(path.value().c_str());
+  return;
 }
 
 // Updates the jumplist, once all the data has been fetched.
@@ -267,73 +366,32 @@ 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
+  enum FolderDeleteResult {
+    SUCCEED = 0,

[grt (UTC plus 2), 2017/03/20 09:15:36]

SUCCEED and FAILED are very generic. do you want to capture:
- deleted all files
- deleted max files allowed
- failed to delete all requested files
- ?

[chengx, 2017/03/21 01:37:11]

This enum is updated. It has all the possible failure modes now.

+    FAILED,
+    // Add new items before this one, always keep this one at the end.
+    END
   };
 
-  // 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.
+  uint32_t folder_delete_status = FolderDeleteResult::SUCCEED;

[grt (UTC plus 2), 2017/03/20 09:15:37]

FolderDeleteResult isn't an "enum class", so no need to qualify.

[chengx, 2017/03/21 01:37:11]

Done.

 
   base::ScopedClosureRunner log_operation_status_when_done(base::Bind(
-      [](uint32_t* folder_operation_status_ptr) {
+      [](uint32_t* folder_delete_status_ptr) {
         UMA_HISTOGRAM_ENUMERATION(
             "WinJumplist.DetailedFolderResultsDeleteUpdated",
-            *folder_operation_status_ptr, FolderOperationResult::END);
+            *folder_delete_status_ptr, FolderDeleteResult::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;
-    }
-  }
-
-  // If CreateDirectory() fails, exit early.
-  if (!base::CreateDirectory(icon_dir)) {
-    folder_operation_status |= FolderOperationResult::CREATE_SRC_FAILED;
+      base::Unretained(&folder_delete_status)));
+
+  // If failing to delete the content in |icon_dir|, post a low-priority task
+  // to try to delete it later and exit early.
+  if (!DeleteDirectoryContentWithUpperLimit(icon_dir, true,
+                                            kMaxIconFilesDeletedPerUpdate)) {
+    folder_delete_status |= FolderDeleteResult::FAILED;

[grt (UTC plus 2), 2017/03/20 09:15:36]

is folder_delete_status a bitfield or just a variable? if the former, please
change the type to "enum FolderDeleteResult : uint32_t" and give FAILED an
explicit value. if not, leave the enum type as-is, but still give FAILED an
explicit value since it's used in a histogram. it's also good practice to add a
comment to the enum saying something like:
 // Used for UMA. Do not delete entries, and keep in sync with histograms.xml.

[chengx, 2017/03/21 01:37:10]

In this CL, it should be changed to a variable. So I will leave the enum type as
it is. I tend to list all failure modes in the enum rather than a single
"FAILED". Also, comment added as what you suggested. For this
|folder_delete_status| variable, it should be int instead of uint now.

+    BrowserThread::PostAfterStartupTask(

[grt (UTC plus 2), 2017/03/20 09:15:36]

PostAfterStartupTask is only needed if you're posting a task during startup that
you want to run sometime later. in this case, you can post the task using the
new base/task_scheduler API.

beware that you may get a request to update the jumplist while this task is of
running. think about what sort of synchronization is needed. for example, should
the next jumplist update wait until this delete task from a previous run has
completed? otherwise could you have a race where thread 1 is deleting files
created by thread 2?

[chengx, 2017/03/21 01:37:10]

Thanks for the reminder. This is something that needs to be well considered.
Making the next jumplist update wait until this background task from previous
run finishes, or invoking another icon creation task after the background delete
task finishes, seems not optimal either way. Maybe try to use another
"JumpListIconsNew" directory to hold the icons, while deleting the two existing
folders using two background tasks. I will create a google doc and write down
all these methods for discussion.

But now, I would prefer to not posting this background delete task for
JumpListIcons directory. This CL alone will allow us to evaluate the effects of
removing the JumpListIconsOld directory related operations. Also, I have added
UMA to record the detailed delete status for both JumpListIcons{,Old} folders,
which should give us more information to decide which way to go later on.

+        FROM_HERE, BrowserThread::GetTaskRunnerForThread(BrowserThread::FILE),
+        base::Bind(&DeleteDirectory, icon_dir, false));
     return;
   }
 
@@ -349,6 +407,17 @@ void RunUpdateOnFileThread(
   // with it.
   UpdateJumpList(app_id.c_str(), local_most_visited_pages,
                  local_recently_closed_pages, incognito_availability);
+
+  // Post a low-priority task to delete the content in |icon_dir_old| if there
+  // is any.
+  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())) {
+    BrowserThread::PostAfterStartupTask(

[grt (UTC plus 2), 2017/03/20 09:15:37]

comments above apply here as well. does it suffice to post this task once during
the lifetime of a browser process? if the task runs as TaskPriority::BACKGROUND
with TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN, it can quietly do its job in
the bg and never needs to be posted more than once. if you're concerned about it
hogging a thread for a very, very long time, you could have it chunk its work
by, for example, deleting 100 files then reposting itself so that other work can
be done before the next 100 are deleted.

[chengx, 2017/03/21 01:37:10]

Thanks for pointing to these APIs and traits that shall be used. I did have
concern that the job would hog a thread for a very long time. As suggested, I
have set the upper limit (i.e., 100) for the amount of files allowed to be
deleted per update. Since the deletion here is for the JumpListIconsOld
directory, there will be no deletion/creation race condition happening here.
Self-invoking deletion of 100 files periodically sounds a good idea. However,
since it is suspicious that ::DeleteFile can mark file as deleted while it is
not, I am reluctant to self-invoke the deletion task. Moreover, as this
JumpListIconsOld folder is no longer used, we can allow its size to go down
gradually instead of all of a sudden. This is a trade-off indeed, and I prefer
just trying to delete 100 files per update.

+        FROM_HERE, BrowserThread::GetTaskRunnerForThread(BrowserThread::FILE),
+        base::Bind(&DeleteDirectory, icon_dir_old, true));
+  }
 }
 
 }  // namespace