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

chrome/browser/win/jumplist.cc

 // Copyright 2012 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.
 
 #include "chrome/browser/win/jumplist.h"
 
+#include <windows.h>
+
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/callback_helpers.h"
 #include "base/command_line.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/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"
 #include "chrome/browser/history/top_sites_factory.h"
 #include "chrome/browser/metrics/jumplist_metrics_win.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/sessions/tab_restore_service_factory.h"
 #include "chrome/browser/shell_integration_win.h"
 #include "chrome/common/chrome_constants.h"
 #include "chrome/common/chrome_switches.h"
@@ skipping 190 matching lines @@
   if (!UpdateTaskCategory(&jumplist_updater, incognito_availability))
     return false;
 
   // Commit this transaction and send the updated JumpList to Windows.
   if (!jumplist_updater.CommitUpdate())
     return false;
 
   return true;
 }
 
+// Base::Move() tries to rename a file and if this fails, it tries copy-n-delete

[grt (UTC plus 2), 2017/03/09 10:11:48]

nit: document what this function does before explaining why; see
https://google.github.io/styleguide/cppguide.html#Function_Comments for details.

i would also be inclined to make this RenameDirectory and document that this is
all it should be used for with something like:

// Renames the directory |from_dir| to |to_path|. Fails if any process has a
handle open in |from_dir| or if |to_path| exists.

[chengx, 2017/03/09 23:57:36]

Done.

+// This Rename method has the same functionality of its "rename" part.
+bool Rename(const base::FilePath& from_path, const base::FilePath& to_path) {
+  if (from_path.ReferencesParent() || to_path.ReferencesParent())
+    return false;
+  base::ThreadRestrictions::AssertIOAllowed();

[grt (UTC plus 2), 2017/03/09 10:11:48]

nit: move the assert to the top of the function

[chengx, 2017/03/09 23:57:36]

Done.

+  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(),
+                    MOVEFILE_REPLACE_EXISTING) != 0;

[grt (UTC plus 2), 2017/03/09 10:11:48]

MOVEFILE_REPLACE_EXISTING -> 0 since you're moving a directory (msdn says "This
value cannot be used if lpNewFileName or lpExistingFileName names a
directory.").

[chengx, 2017/03/09 23:57:36]

Ah, yes you are right.

+}
+
 // Updates the jumplist, once all the data has been fetched.
 void RunUpdateOnFileThread(
     IncognitoModePrefs::Availability incognito_availability,
     const std::wstring& app_id,
     const base::FilePath& icon_dir,
     base::RefCountedData<JumpListData>* ref_counted_data) {
   JumpListData* data = &ref_counted_data->data;
   ShellLinkItemList local_most_visited_pages;
   ShellLinkItemList local_recently_closed_pages;
 
   {
     base::AutoLock auto_lock(data->list_lock_);
     // Make sure we are not out of date: if icon_urls_ is not empty, then
     // another notification has been received since we processed this one
     if (!data->icon_urls_.empty())
       return;
 
     // Make local copies of lists so we can release the lock.
     local_most_visited_pages = data->most_visited_pages_;
     local_recently_closed_pages = data->recently_closed_pages_;
   }
 
   // Delete the directory which contains old icon files, rename the current

[grt (UTC plus 2), 2017/03/09 10:11:48]

why jump through these hoops with different directories? can you not delete the
old files and put new files in the same dir? this seems less prone to failure,
but perhaps i'm missing something (i'm not very familiar with the jumplist
code).

[chengx, 2017/03/09 23:57:36]

This is a very good question. I was confused too why we need to keep two
jumplist folders when I started to touch jumplist bug. My guess was that this
can increase robustness. If creating new jumplist icons fails, we can recover
the old icons by moving them back. However, I don't see this recover strategy
implemented.

   // icon directory, and create a new directory which contains new JumpList
   // icon files.
   base::FilePath icon_dir_old(icon_dir.value() + L"Old");

[grt (UTC plus 2), 2017/03/09 10:11:48]

this should be:
  base::FilePath icon_dir_old =
icon_dir.DirName().Append(icon_dir.BaseName().value() +
FILE_PATH_LITERAL("Old"));
or something like that. the end result is the same, but it's safer to use the
FilePath API correctly.

[chengx, 2017/03/09 23:57:36]

Done.

 
   enum FolderOperationResult {
     SUCCESS = 0,
     DELETE_DEST_FAILED = 1 << 0,
     MOVE_FAILED = 1 << 1,
     DELETE_SRC_FAILED = 1 << 2,
     CREATE_SRC_FAILED = 1 << 3,
     // 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 << 4
   };
 
   // This variable records the status of three folder operations.
   uint32_t folder_operation_status = FolderOperationResult::SUCCESS;
 
   base::ScopedClosureRunner log_operation_status_when_done(base::Bind(
       [](uint32_t* folder_operation_status_ptr) {
         UMA_HISTOGRAM_ENUMERATION("WinJumplist.DetailedFolderResults",
                                   *folder_operation_status_ptr,
                                   FolderOperationResult::END);
       },
       base::Unretained(&folder_operation_status)));
 
   // If deletion of |icon_dir_old| fails, do not move |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)) {

[grt (UTC plus 2), 2017/03/09 10:11:48]

if you need to do this directory dance, you can make it more robust by making
sure that the target of the rename below has a different name on the filesystem
than the dir you're deleting here. in other words, i would break this up into:
1. enumerate items in icon_dir's parent to find the set of all dir names that
begin with icon_dir.BaseName(), except for icon_dir itself.
2. generate a new "Old" temp dir that does not already exist in the set above.
3. delete all dirs in that set above
4. rename icon_dir to the new "Old" temp dir (that does not have the same name
as any that was just deleted)

[chengx, 2017/03/09 23:57:36]

We now have two folders, namely JumplistIcon and JumplistIconOld. Do you mean we
can rename JumplistIcon to any other arbitrary ones, so that it is different
from JumplistIconOld (the one deleted)? Correct me if I am wrong. If you do mean
this, I would be hesitated to do this, as deletion of |icon_dir_old| does fail
for reasons. Then we will have a lot of folders bloated up if renaming the
folder to different ones. Also, the current logic is that if deletion of
|icon_dir_old| fails, then renameDirectory() is not run; even if it runs, it
will fail as |icon_dir_old| still exists. So it should be okay here.

     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 (!base::DeleteFile(icon_dir, true)) {
       folder_operation_status |= FolderOperationResult::DELETE_SRC_FAILED;
       return;
     }
-  } else if (!base::Move(icon_dir, icon_dir_old)) {
+  } else if (!Rename(icon_dir, icon_dir_old)) {
     folder_operation_status |= FolderOperationResult::MOVE_FAILED;
     // If Move() fails, delete |icon_dir| to avoid file accumulation in this

[grt (UTC plus 2), 2017/03/09 10:11:48]

"If Rename() fails..." (or RenameDirectory if you accept my comment above)

[chengx, 2017/03/09 23:57:36]

Done.

     // directory, which can eventually lead the folder to be huge.
     if (!base::DeleteFile(icon_dir, true)) {

[grt (UTC plus 2), 2017/03/09 10:11:48]

this could potentially delete the contents of |icon_dir| but fail to delete the
dir itself. consider handling this case by checking to see if the dir is not
empty at this point and only failing in that case.

another failure mode is that some of the icons in the dir that were used by the
previous jumplist were deleted. does the jumplist need to be updated in this
case?

[chengx, 2017/03/09 23:57:36]

Agreed. I have added a check here.
 
I think in this case, it is better to skip jumplist update. Although keeping the
jumplist icons up-to-date is good, bloating the jumplist folder gives users much
more pain.

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

What does it mean if some of the icons that are referenced by the old jumplist
are deleted?

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

I thought you were asking "What if deletion of jumplistIcon folder partially
succeeds? Say we always have 10 items in it. The deletion fails after deleting 6
of them. Then do we go ahead and update the jumplist, i.e., writing another 10
items into it (we cannot write just the extra 4)?"

Then my opinion is we don't update the jumplist in this case, as it will now
increase the jumplistIcon folder to contain 14 files. Then the accumulation
starts and bad things will happen.

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

Yes, that's exactly my question. I was under the impression that the files in
this directory are somehow referenced by some other data structure elsewhere
that the shell knows about from the last time the jumplist was created/updated.
So let's say we have a perfectly sane and well-functioning jumplist with 10
icons in it. Then we fail to rename the directory and instead start deleting
items in it -- at this point we're taking away the images that are actively
referenced by the shell somehow, no? What happens if the user pokes Chrome's
icon to bring up the jumplist? Don't we need to tell the shell somehow "hey,
forget about these icons i told you about before, 'cause i just deleted them out
from under you"?
By "update" I meant "tell the shell that the sands have shifted beneath it", not
"write a whole new set of icons to disk."

       folder_operation_status |= FolderOperationResult::DELETE_SRC_FAILED;
       return;
     }
   }
 
   if (!base::CreateDirectory(icon_dir))
     folder_operation_status |= FolderOperationResult::CREATE_SRC_FAILED;
 
   // Create temporary icon files for shortcuts in the "Most Visited" category.
   CreateIconFiles(icon_dir, local_most_visited_pages);
@@ skipping 329 matching lines @@
 void JumpList::TopSitesLoaded(history::TopSites* top_sites) {
 }
 
 void JumpList::TopSitesChanged(history::TopSites* top_sites,
                                ChangeReason change_reason) {
   top_sites->GetMostVisitedURLs(
       base::Bind(&JumpList::OnMostVisitedURLsAvailable,
                  weak_ptr_factory_.GetWeakPtr()),
       false);
 }