Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2732313002:
Move JumplistIcon to JumplistIconOld only via renaming, not copy-n-delete (Closed)
DescriptionMove JumplistIcon to JumplistIconOld only via renaming, not copy-n-delete
In jumplist related IO operations, one step is to move folder
JumplistIcon to JumplistIconOld using base::Move(). The method tries to
rename the folder and copy-n-delete if renaming fails. For users with
these folders corrupted and bloated already, copy-n-delete is very
expensive and may lead those icon files to be never deleted when e.g.,
copy succeeds yet delete fails.
As folders JumplistIcon and JumplistIconOld are in the same disk volume,
renaming should be working fine and enough for users whose jumplists are
working normally. So this change won't affect those Chrome users at all.
Local tests have been done on my machine and proved it.
This CL also updates the logic when deletion of folder JumplistIcon
fails. Previous logic is that if base::DeleteFile() function call fails,
updating jumplist is skipped. This can be potentially wrong if deletion
of the content in JumplistIcon succeeds but deletion of folder alone
fails. In this case, we should still continue with jumplist update,
which is fixed in this CL. UMA histogram is also updated to record this
change.
UMA metrics will be kept being monitored for this change.
BUG=40407, 179576
Review-Url: https://codereview.chromium.org/2732313002
Cr-Commit-Position: refs/heads/master@{#456505}
Committed: https://chromium.googlesource.com/chromium/src/+/2d05b7ac09a50214f67c7a87c875c9cd6544dce1
Patch Set 1 #
Total comments: 19
Patch Set 2 : Address comments. #
Total comments: 2
Patch Set 3 : Update folder_operation_status. #Patch Set 4 : Add a new metric in histograms.xml #
Total comments: 4
Patch Set 5 : Fix comments in histograms.xml #
Total comments: 2
Patch Set 6 : Address comments. #
Total comments: 1
Messages
Total messages: 55 (38 generated)
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Rename a file, not copy-n-delete. BUG= ========== to ========== Move JumplistIcon to JumplistIconOld only via renaming, not copy-n-delete In jumplist related IO operations, one step is to move folder JumplistIcon to JumplistIconOld using base::Move(). The method tries to rename the folder and copy-n-delete if renaming fails. For users with these folders corrupted and bloated already, copy-n-delete is very expensive and may lead to those icons files never deleted when e.g., copy succeeds yet delete fails. As folders JumplistIcon and JumplistIconOld are in the same disk volume, renaming should be working fine and enough for users whose jumplists are working normally. So this change won't affect those Chrome users at all. Local tests have been done on my corp machine and proved it. UMA metrics will be kept being monitored for this change. If things are changing in a negative way, this CL will be reverted. If things are changing positively, the Rename method now added in jumplist.cc may be considered to move to \\base, or not. BUG=40407, 179576 ==========
chengx@chromium.org changed reviewers: + grt@chromium.org
PTAL~
Description was changed from ========== Move JumplistIcon to JumplistIconOld only via renaming, not copy-n-delete In jumplist related IO operations, one step is to move folder JumplistIcon to JumplistIconOld using base::Move(). The method tries to rename the folder and copy-n-delete if renaming fails. For users with these folders corrupted and bloated already, copy-n-delete is very expensive and may lead to those icons files never deleted when e.g., copy succeeds yet delete fails. As folders JumplistIcon and JumplistIconOld are in the same disk volume, renaming should be working fine and enough for users whose jumplists are working normally. So this change won't affect those Chrome users at all. Local tests have been done on my corp machine and proved it. UMA metrics will be kept being monitored for this change. If things are changing in a negative way, this CL will be reverted. If things are changing positively, the Rename method now added in jumplist.cc may be considered to move to \\base, or not. BUG=40407, 179576 ========== to ========== Move JumplistIcon to JumplistIconOld only via renaming, not copy-n-delete In jumplist related IO operations, one step is to move folder JumplistIcon to JumplistIconOld using base::Move(). The method tries to rename the folder and copy-n-delete if renaming fails. For users with these folders corrupted and bloated already, copy-n-delete is very expensive and may lead to those icons files never deleted when e.g., copy succeeds yet delete fails. As folders JumplistIcon and JumplistIconOld are in the same disk volume, renaming should be working fine and enough for users whose jumplists are working normally. So this change won't affect those Chrome users at all. Local tests have been done on my corp machine and proved it. UMA metrics will be kept being monitored for this change. If things are changing in a negative way, this CL will be reverted. BUG=40407, 179576 ==========
Are you aware that MoveFile on a directory will fail if any file within the directory is open by any process on the system? Is it expected that sometimes an item in the jumplist dir is held open by the shell?
On 2017/03/08 09:14:06, grt (UTC plus 1) wrote: > Are you aware that MoveFile on a directory will fail if any file within the > directory is open by any process on the system? Is it expected that sometimes an > item in the jumplist dir is held open by the shell? Yes, I know MoveFile on a dir fails if any of its files is open by another process. However, I don't think it is expected that an item in the jumplist dir is open by the shell or any other process. I think MoveFile tries to rename the dir first. If it fails due to some reason, it then tries copy and delete, which is very expensive and potentially can keep the dir huge in cases where jumplist dir is already huge. This CL is just to test how much impact doing copy-n-delete can bring. It will be reverted if I see this put Canary users in a worse situation.
if base::Move is being handled by the copy-n-delete path in the wild, then it's most likely because users have scanners and such that are getting in the way by holding files open, or that they have scanners and such that are getting in the way of the preceding delete. while this small change in its own will get rid of the copying part of base::Move, it won't do anything to make jumplists work better for users. i've dropped a few comments for things that i think have a decent chance of helping. https://codereview.chromium.org/2732313002/diff/1/chrome/browser/win/jumplist.cc File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2732313002/diff/1/chrome/browser/win/jumplist... chrome/browser/win/jumplist.cc:231: // Base::Move() tries to rename a file and if this fails, it tries copy-n-delete 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. https://codereview.chromium.org/2732313002/diff/1/chrome/browser/win/jumplist... chrome/browser/win/jumplist.cc:236: base::ThreadRestrictions::AssertIOAllowed(); nit: move the assert to the top of the function https://codereview.chromium.org/2732313002/diff/1/chrome/browser/win/jumplist... chrome/browser/win/jumplist.cc:242: MOVEFILE_REPLACE_EXISTING) != 0; MOVEFILE_REPLACE_EXISTING -> 0 since you're moving a directory (msdn says "This value cannot be used if lpNewFileName or lpExistingFileName names a directory."). https://codereview.chromium.org/2732313002/diff/1/chrome/browser/win/jumplist... chrome/browser/win/jumplist.cc:267: // Delete the directory which contains old icon files, rename the current 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). https://codereview.chromium.org/2732313002/diff/1/chrome/browser/win/jumplist... chrome/browser/win/jumplist.cc:270: base::FilePath icon_dir_old(icon_dir.value() + L"Old"); 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. https://codereview.chromium.org/2732313002/diff/1/chrome/browser/win/jumplist... chrome/browser/win/jumplist.cc:297: if (!base::DeleteFile(icon_dir_old, true)) { 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) https://codereview.chromium.org/2732313002/diff/1/chrome/browser/win/jumplist... chrome/browser/win/jumplist.cc:308: // If Move() fails, delete |icon_dir| to avoid file accumulation in this "If Rename() fails..." (or RenameDirectory if you accept my comment above) https://codereview.chromium.org/2732313002/diff/1/chrome/browser/win/jumplist... chrome/browser/win/jumplist.cc:310: if (!base::DeleteFile(icon_dir, true)) { 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?
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks for pointing this out. Honestly, I am confused by how base::DeleteFile() proceeds. Say one user has both JumplistIcon and JumplistIconOld folder bloated already. They are already in Gbytes. Now the code is trying to delete JumplistIconOld. When it traverses to an item that the scanner has already opened, DeleteFileRecursive() returns false immediately as in https://cs.chromium.org/chromium/src/base/files/file_util_win.cc?dr=CSs&q=fil..., and then base::DeleteFile() returns false immediately. It may takes a long time to return this "false" as there are a lot of items deleted already. However, it does delete a lot of items this time, and the JumplistIconOld folder's size should decrease gradually after a number of delete tries. The same applies to JumplistIcon folder. I know this is all in theory, and some other things unexpected can happen. For example, the item marked deleted is not deleted at all, therefore the JumplistIconOld folder's size is not shrinking at all. I know you are rewriting base::Delete(), and hopefully it helps here too. I want to land this CL simply trying to get rid of copy-n-paste in the move step, which is another step that could take a long time. If this CL helps a little or completely nothing, then I could say base::DeleteFile() is problematic with high confidence. On 2017/03/09 10:11:49, grt (UTC plus 1) wrote: > if base::Move is being handled by the copy-n-delete path in the wild, then it's > most likely because users have scanners and such that are getting in the way by > holding files open, or that they have scanners and such that are getting in the > way of the preceding delete. while this small change in its own will get rid of > the copying part of base::Move, it won't do anything to make jumplists work > better for users. i've dropped a few comments for things that i think have a > decent chance of helping. https://codereview.chromium.org/2732313002/diff/1/chrome/browser/win/jumplist.cc File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2732313002/diff/1/chrome/browser/win/jumplist... chrome/browser/win/jumplist.cc:231: // Base::Move() tries to rename a file and if this fails, it tries copy-n-delete On 2017/03/09 10:11:48, grt (UTC plus 1) wrote: > 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. Done. https://codereview.chromium.org/2732313002/diff/1/chrome/browser/win/jumplist... chrome/browser/win/jumplist.cc:236: base::ThreadRestrictions::AssertIOAllowed(); On 2017/03/09 10:11:48, grt (UTC plus 1) wrote: > nit: move the assert to the top of the function Done. https://codereview.chromium.org/2732313002/diff/1/chrome/browser/win/jumplist... chrome/browser/win/jumplist.cc:242: MOVEFILE_REPLACE_EXISTING) != 0; On 2017/03/09 10:11:48, grt (UTC plus 1) wrote: > MOVEFILE_REPLACE_EXISTING -> 0 since you're moving a directory (msdn says "This > value cannot be used if lpNewFileName or lpExistingFileName names a > directory."). Ah, yes you are right. https://codereview.chromium.org/2732313002/diff/1/chrome/browser/win/jumplist... chrome/browser/win/jumplist.cc:267: // Delete the directory which contains old icon files, rename the current On 2017/03/09 10:11:48, grt (UTC plus 1) wrote: > 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). 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. https://codereview.chromium.org/2732313002/diff/1/chrome/browser/win/jumplist... chrome/browser/win/jumplist.cc:270: base::FilePath icon_dir_old(icon_dir.value() + L"Old"); On 2017/03/09 10:11:48, grt (UTC plus 1) wrote: > 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. Done. https://codereview.chromium.org/2732313002/diff/1/chrome/browser/win/jumplist... chrome/browser/win/jumplist.cc:297: if (!base::DeleteFile(icon_dir_old, true)) { On 2017/03/09 10:11:48, grt (UTC plus 1) wrote: > 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) 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. https://codereview.chromium.org/2732313002/diff/1/chrome/browser/win/jumplist... chrome/browser/win/jumplist.cc:308: // If Move() fails, delete |icon_dir| to avoid file accumulation in this On 2017/03/09 10:11:48, grt (UTC plus 1) wrote: > "If Rename() fails..." (or RenameDirectory if you accept my comment above) Done. https://codereview.chromium.org/2732313002/diff/1/chrome/browser/win/jumplist... chrome/browser/win/jumplist.cc:310: if (!base::DeleteFile(icon_dir, true)) { On 2017/03/09 10:11:48, grt (UTC plus 1) wrote: > 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. Agreed. I have added a check here. > 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? 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.
Yes, DeleteFile is imperfect at the moment. I have it on my plate to fix this (work-in-progress here https://codereview.chromium.org/2545283002/, but I wish to add some more tests before landing since it's quite complex). I think your plan to land this to see what happens makes sense. https://codereview.chromium.org/2732313002/diff/1/chrome/browser/win/jumplist.cc File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2732313002/diff/1/chrome/browser/win/jumplist... chrome/browser/win/jumplist.cc:310: if (!base::DeleteFile(icon_dir, true)) { On 2017/03/09 23:57:36, chengx wrote: > On 2017/03/09 10:11:48, grt (UTC plus 1) wrote: > > 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. > > Agreed. I have added a check here. > > > 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? > > 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. What does it mean if some of the icons that are referenced by the old jumplist are deleted? https://codereview.chromium.org/2732313002/diff/20001/chrome/browser/win/jump... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2732313002/diff/20001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:309: if (!::PathIsDirectoryEmpty(icon_dir.value().c_str())) 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.
Description was changed from ========== Move JumplistIcon to JumplistIconOld only via renaming, not copy-n-delete In jumplist related IO operations, one step is to move folder JumplistIcon to JumplistIconOld using base::Move(). The method tries to rename the folder and copy-n-delete if renaming fails. For users with these folders corrupted and bloated already, copy-n-delete is very expensive and may lead to those icons files never deleted when e.g., copy succeeds yet delete fails. As folders JumplistIcon and JumplistIconOld are in the same disk volume, renaming should be working fine and enough for users whose jumplists are working normally. So this change won't affect those Chrome users at all. Local tests have been done on my corp machine and proved it. UMA metrics will be kept being monitored for this change. If things are changing in a negative way, this CL will be reverted. BUG=40407, 179576 ========== to ========== Move JumplistIcon to JumplistIconOld only via renaming, not copy-n-delete In jumplist related IO operations, one step is to move folder JumplistIcon to JumplistIconOld using base::Move(). The method tries to rename the folder and copy-n-delete if renaming fails. For users with these folders corrupted and bloated already, copy-n-delete is very expensive and may lead to those icons files never deleted when e.g., copy succeeds yet delete fails. As folders JumplistIcon and JumplistIconOld are in the same disk volume, renaming should be working fine and enough for users whose jumplists are working normally. So this change won't affect those Chrome users at all. Local tests have been done on my corp machine and proved it. This CL also updates the logic when deletion of folder JumplistIcon fails. Previous logic is that if base::DeleteFile() function call fails, updating jumplist is skipped. This can be potentially wrong if deletion of the content in JumplistIcon succeeds but deletion of folder alone fails. In this case, we should still continue with jumplist update, which is fixed in this CL. UMA histogram is also updated to record this change. UMA metrics will be kept being monitored for this change. If things are changing in a negative way, this CL will be reverted. BUG=40407, 179576 ==========
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Move JumplistIcon to JumplistIconOld only via renaming, not copy-n-delete In jumplist related IO operations, one step is to move folder JumplistIcon to JumplistIconOld using base::Move(). The method tries to rename the folder and copy-n-delete if renaming fails. For users with these folders corrupted and bloated already, copy-n-delete is very expensive and may lead to those icons files never deleted when e.g., copy succeeds yet delete fails. As folders JumplistIcon and JumplistIconOld are in the same disk volume, renaming should be working fine and enough for users whose jumplists are working normally. So this change won't affect those Chrome users at all. Local tests have been done on my corp machine and proved it. This CL also updates the logic when deletion of folder JumplistIcon fails. Previous logic is that if base::DeleteFile() function call fails, updating jumplist is skipped. This can be potentially wrong if deletion of the content in JumplistIcon succeeds but deletion of folder alone fails. In this case, we should still continue with jumplist update, which is fixed in this CL. UMA histogram is also updated to record this change. UMA metrics will be kept being monitored for this change. If things are changing in a negative way, this CL will be reverted. BUG=40407, 179576 ========== to ========== Move JumplistIcon to JumplistIconOld only via renaming, not copy-n-delete In jumplist related IO operations, one step is to move folder JumplistIcon to JumplistIconOld using base::Move(). The method tries to rename the folder and copy-n-delete if renaming fails. For users with these folders corrupted and bloated already, copy-n-delete is very expensive and may lead to those icons files never deleted when e.g., copy succeeds yet delete fails. As folders JumplistIcon and JumplistIconOld are in the same disk volume, renaming should be working fine and enough for users whose jumplists are working normally. So this change won't affect those Chrome users at all. Local tests have been done on my machine and proved it. This CL also updates the logic when deletion of folder JumplistIcon fails. Previous logic is that if base::DeleteFile() function call fails, updating jumplist is skipped. This can be potentially wrong if deletion of the content in JumplistIcon succeeds but deletion of folder alone fails. In this case, we should still continue with jumplist update, which is fixed in this CL. UMA histogram is also updated to record this change. UMA metrics will be kept being monitored for this change. BUG=40407, 179576 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
chengx@chromium.org changed reviewers: + isherman@chromium.org
isherman@, could you please take a look at histograms.xml? grt@, comments addressed in the new patch set. PTAL~ I had a quick look at the new base::DeleteFile() you are implementing. Honestly, I couldn't imagine it is this complex to robustly delete a file on Windows. https://codereview.chromium.org/2732313002/diff/1/chrome/browser/win/jumplist.cc File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2732313002/diff/1/chrome/browser/win/jumplist... chrome/browser/win/jumplist.cc:310: if (!base::DeleteFile(icon_dir, true)) { On 2017/03/10 09:24:06, grt (UTC plus 1) wrote: > On 2017/03/09 23:57:36, chengx wrote: > > On 2017/03/09 10:11:48, grt (UTC plus 1) wrote: > > > 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. > > > > Agreed. I have added a check here. > > > > > 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? > > > > 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. > > What does it mean if some of the icons that are referenced by the old jumplist > are deleted? 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. https://codereview.chromium.org/2732313002/diff/20001/chrome/browser/win/jump... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2732313002/diff/20001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:309: if (!::PathIsDirectoryEmpty(icon_dir.value().c_str())) On 2017/03/10 09:24:06, grt (UTC plus 1) wrote: > 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. Sure, I have updated enum FolderOperationResult and folder_operation_status accordingly.
I'd recommend renaming the metric, since you're changing its semantics.
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Hi Ilya, I have renamed the metric. PTAL, thanks!
Metrics LGTM % comments: https://codereview.chromium.org/2732313002/diff/60001/chrome/browser/win/jump... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2732313002/diff/60001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:294: "WinJumplist.DetailedFolderResultsDeleteUpdated", Optional nit: I'd suggest a simpler renaming, to "WinJumplist.DetailedFolderResults2", but either name is fine. https://codereview.chromium.org/2732313002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2732313002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:79587: + Obsolete 03/10/2017 as we are no long recording this metric. Please document what metric it was replaced by.
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Move JumplistIcon to JumplistIconOld only via renaming, not copy-n-delete In jumplist related IO operations, one step is to move folder JumplistIcon to JumplistIconOld using base::Move(). The method tries to rename the folder and copy-n-delete if renaming fails. For users with these folders corrupted and bloated already, copy-n-delete is very expensive and may lead to those icons files never deleted when e.g., copy succeeds yet delete fails. As folders JumplistIcon and JumplistIconOld are in the same disk volume, renaming should be working fine and enough for users whose jumplists are working normally. So this change won't affect those Chrome users at all. Local tests have been done on my machine and proved it. This CL also updates the logic when deletion of folder JumplistIcon fails. Previous logic is that if base::DeleteFile() function call fails, updating jumplist is skipped. This can be potentially wrong if deletion of the content in JumplistIcon succeeds but deletion of folder alone fails. In this case, we should still continue with jumplist update, which is fixed in this CL. UMA histogram is also updated to record this change. UMA metrics will be kept being monitored for this change. BUG=40407, 179576 ========== to ========== Move JumplistIcon to JumplistIconOld only via renaming, not copy-n-delete In jumplist related IO operations, one step is to move folder JumplistIcon to JumplistIconOld using base::Move(). The method tries to rename the folder and copy-n-delete if renaming fails. For users with these folders corrupted and bloated already, copy-n-delete is very expensive and may lead those icon files to be never deleted when e.g., copy succeeds yet delete fails. As folders JumplistIcon and JumplistIconOld are in the same disk volume, renaming should be working fine and enough for users whose jumplists are working normally. So this change won't affect those Chrome users at all. Local tests have been done on my machine and proved it. This CL also updates the logic when deletion of folder JumplistIcon fails. Previous logic is that if base::DeleteFile() function call fails, updating jumplist is skipped. This can be potentially wrong if deletion of the content in JumplistIcon succeeds but deletion of folder alone fails. In this case, we should still continue with jumplist update, which is fixed in this CL. UMA histogram is also updated to record this change. UMA metrics will be kept being monitored for this change. BUG=40407, 179576 ==========
isherman@, commented addressed in the new patch set. Thanks! grt@, please refer to patch set 3 for my replies to your feedback. Thanks! https://codereview.chromium.org/2732313002/diff/60001/chrome/browser/win/jump... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2732313002/diff/60001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:294: "WinJumplist.DetailedFolderResultsDeleteUpdated", On 2017/03/10 23:30:16, Ilya Sherman wrote: > Optional nit: I'd suggest a simpler renaming, to > "WinJumplist.DetailedFolderResults2", but either name is fine. I know it is a bit long, but I will stick with this name for now. https://codereview.chromium.org/2732313002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2732313002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:79587: + Obsolete 03/10/2017 as we are no long recording this metric. On 2017/03/10 23:30:16, Ilya Sherman wrote: > Please document what metric it was replaced by. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2732313002/diff/1/chrome/browser/win/jumplist.cc File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2732313002/diff/1/chrome/browser/win/jumplist... chrome/browser/win/jumplist.cc:310: if (!base::DeleteFile(icon_dir, true)) { On 2017/03/10 21:49:40, chengx wrote: > On 2017/03/10 09:24:06, grt (UTC plus 1) wrote: > > On 2017/03/09 23:57:36, chengx wrote: > > > On 2017/03/09 10:11:48, grt (UTC plus 1) wrote: > > > > 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. > > > > > > Agreed. I have added a check here. > > > > > > > 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? > > > > > > 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. > > > > What does it mean if some of the icons that are referenced by the old jumplist > > are deleted? > > 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)?" 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"? > 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. By "update" I meant "tell the shell that the sands have shifted beneath it", not "write a whole new set of icons to disk." https://codereview.chromium.org/2732313002/diff/80001/chrome/browser/win/jump... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2732313002/diff/80001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:313: } else { nit: no "else" when the "if" clause "return;"s same on line 330
The CQ bit was checked by chengx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/03/13 09:03:20, grt (UTC plus 1) wrote: > 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? Re: yes, we are taking away the images that are actively referenced. > What happens if the user pokes Chrome's icon to bring up the jumplist? Re: There will be no icons for those URL links. It'll just be background color there. Chrome won't crash and just empty images instead. I am pretty sure about this as I have manually done this for debugging purpose for a lot of times. > 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"? Re: No, I don't think so. Chrome talks to Windows once it has updated the jumplist as in https://cs.chromium.org/chromium/src/chrome/browser/win/jumplist.cc?q=jumplis... At this time, both the jumplist urls and icons have been updated. When jumplistIcon folder deletion fails yet some of the icons get delete already, the jumplist update code won't run per the current implementation. So the jumplist urls remain the same. As the some of the icons are gone, Windows will just use empty images (but using the background color somehow). I guess the code for this on Windows side could be something like 1. declare an empty image variable for each URL, 2. assign it to the icon image if it is available.
PATL~ https://codereview.chromium.org/2732313002/diff/80001/chrome/browser/win/jump... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2732313002/diff/80001/chrome/browser/win/jump... chrome/browser/win/jumplist.cc:313: } else { On 2017/03/13 09:03:20, grt (UTC plus 1) wrote: > nit: no "else" when the "if" clause "return;"s > same on line 330 Done.
lgtm given that Windows does something sane when jumplist icons disappear. thanks. https://codereview.chromium.org/2732313002/diff/100001/chrome/browser/win/jum... File chrome/browser/win/jumplist.cc (right): https://codereview.chromium.org/2732313002/diff/100001/chrome/browser/win/jum... chrome/browser/win/jumplist.cc:7: #include <Shlwapi.h> is this needed?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by chengx@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2732313002/#ps100001 (title: "Address comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1489442170336430,
"parent_rev": "039925aca6bafdd0db4044b1285311e43c6588f8", "commit_rev":
"2d05b7ac09a50214f67c7a87c875c9cd6544dce1"}
Message was sent while issue was closed.
Description was changed from ========== Move JumplistIcon to JumplistIconOld only via renaming, not copy-n-delete In jumplist related IO operations, one step is to move folder JumplistIcon to JumplistIconOld using base::Move(). The method tries to rename the folder and copy-n-delete if renaming fails. For users with these folders corrupted and bloated already, copy-n-delete is very expensive and may lead those icon files to be never deleted when e.g., copy succeeds yet delete fails. As folders JumplistIcon and JumplistIconOld are in the same disk volume, renaming should be working fine and enough for users whose jumplists are working normally. So this change won't affect those Chrome users at all. Local tests have been done on my machine and proved it. This CL also updates the logic when deletion of folder JumplistIcon fails. Previous logic is that if base::DeleteFile() function call fails, updating jumplist is skipped. This can be potentially wrong if deletion of the content in JumplistIcon succeeds but deletion of folder alone fails. In this case, we should still continue with jumplist update, which is fixed in this CL. UMA histogram is also updated to record this change. UMA metrics will be kept being monitored for this change. BUG=40407, 179576 ========== to ========== Move JumplistIcon to JumplistIconOld only via renaming, not copy-n-delete In jumplist related IO operations, one step is to move folder JumplistIcon to JumplistIconOld using base::Move(). The method tries to rename the folder and copy-n-delete if renaming fails. For users with these folders corrupted and bloated already, copy-n-delete is very expensive and may lead those icon files to be never deleted when e.g., copy succeeds yet delete fails. As folders JumplistIcon and JumplistIconOld are in the same disk volume, renaming should be working fine and enough for users whose jumplists are working normally. So this change won't affect those Chrome users at all. Local tests have been done on my machine and proved it. This CL also updates the logic when deletion of folder JumplistIcon fails. Previous logic is that if base::DeleteFile() function call fails, updating jumplist is skipped. This can be potentially wrong if deletion of the content in JumplistIcon succeeds but deletion of folder alone fails. In this case, we should still continue with jumplist update, which is fixed in this CL. UMA histogram is also updated to record this change. UMA metrics will be kept being monitored for this change. BUG=40407, 179576 Review-Url: https://codereview.chromium.org/2732313002 Cr-Commit-Position: refs/heads/master@{#456505} Committed: https://chromium.googlesource.com/chromium/src/+/2d05b7ac09a50214f67c7a87c875... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/2d05b7ac09a50214f67c7a87c875... |
