Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2545283002:
A robust base::DeleteFile.
|
Created:
4 years ago by grt (UTC plus 2) Modified:
3 years, 9 months ago CC:
chromium-reviews, vmpstr+watch_chromium.org, Scott Hess - ex-Googler, jschuh Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionA robust base::DeleteFile.
Win32's DeleteFile marks a file for deletion, but makes no guarantees
that it disappears. This makes recursive deletes flaky as well as
delete-create or delete-move pairs. This flakines leads to install and
update failures, especially when scanners are active on the
machine. This CL contains a robust file and directory deletion strategy
that works as one would expect (delete means delete).
Additionally, this CL removes wildcard support from the Windows
implementation of base::DeleteFile, resolving a long-standing TODO.
BUG=571906, 674135
Patch Set 1 #Patch Set 2 : clang fix; simplification #
Total comments: 17
Patch Set 3 : shess and jschuh comments #Patch Set 4 : search for temp #Patch Set 5 : cl format and logging to find failure on bot #
Total comments: 24
Patch Set 6 : even better #Patch Set 7 : with metrics and global retry #
Total comments: 4
Patch Set 8 : comment tweaks #Patch Set 9 : sync to position 437832 #
Total comments: 15
Patch Set 10 : test fixes #
Depends on Patchset: Messages
Total messages: 77 (46 generated)
The CQ bit was checked by grt@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...
grt@chromium.org changed reviewers: + wfh@chromium.org
Hi Will. Do you have time to put some eyes on this? I was motivated to make this change after digging into install failures on new machines with a certain piece of A/V software pre-installed. I believe this to be one step on the path to decreasing our install/update failure rate. Feel free to direct to another Win reviewer if you think of someone even better equipped. Thanks. P.S. I've only verified that base_unittests passes locally, so the current dry run may be a bit bloody. :-)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by grt@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...
grt@chromium.org changed reviewers: + shess@google.com - wfh@chromium.org
+cc shess and jschuh https://codereview.chromium.org/2545283002/diff/20001/base/files/file_util_wi... File base/files/file_util_win.cc (right): https://codereview.chromium.org/2545283002/diff/20001/base/files/file_util_wi... base/files/file_util_win.cc:212: !IsMountPointOrSymlink(to_delete, basic_info) && !DeleteContents(path)) { i wonder if this is overkill. a directory with a reparse point cannot contain files, so perhaps it's correct enough to not recurse if |to_delete| is a directory and has a reparse point. thoughts?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
jschuh@chromium.org changed reviewers: + jschuh@chromium.org
Sadly, I'm not entirely surprised it's this complicated. https://codereview.chromium.org/2545283002/diff/20001/base/files/file_util_wi... File base/files/file_util_win.cc (right): https://codereview.chromium.org/2545283002/diff/20001/base/files/file_util_wi... base/files/file_util_win.cc:212: !IsMountPointOrSymlink(to_delete, basic_info) && !DeleteContents(path)) { On 2016/12/04 08:52:06, grt (UTC plus 1) wrote: > i wonder if this is overkill. a directory with a reparse point cannot contain > files, so perhaps it's correct enough to not recurse if |to_delete| is a > directory and has a reparse point. thoughts? For IsMountPointOrSymlink()? Doesn't seem like overkill to me. https://codereview.chromium.org/2545283002/diff/20001/base/files/file_util_wi... base/files/file_util_win.cc:304: bool TreeDeleter::MoveToParent(const FilePath& path, Is there a better place to move to than the parent? Could we first check if one of the temp resolutions is on the same drive, and if so use that as the target first? My thought is to manually look for a temp by checking GetTempPath()'s result, %SystemRoot%\Temp, %USERPROFILE%\AppData\Local\Temp, and ?:\Temp for the target drive. Any of those just seems like a better location than the parent.
https://codereview.chromium.org/2545283002/diff/20001/base/files/file_util_wi... File base/files/file_util_win.cc (right): https://codereview.chromium.org/2545283002/diff/20001/base/files/file_util_wi... base/files/file_util_win.cc:212: !IsMountPointOrSymlink(to_delete, basic_info) && !DeleteContents(path)) { On 2016/12/04 14:10:46, jschuh (very slow) wrote: > On 2016/12/04 08:52:06, grt (UTC plus 1) wrote: > > i wonder if this is overkill. a directory with a reparse point cannot contain > > files, so perhaps it's correct enough to not recurse if |to_delete| is a > > directory and has a reparse point. thoughts? > > For IsMountPointOrSymlink()? Doesn't seem like overkill to me. I'm not too familiar with the other uses for reparse points. https://msdn.microsoft.com/en-us/library/dd541667.aspx lists the possibilities as defined by MS, but 3rd parties can define their own. Since MSDN says that a directory with a reparse point may not contain files, it seems to be reasonably conservative to never recurse into them. Or are there cases where it really is the right thing? Of note: Win32's DeleteFile operates on links rather than targets and Win32's RemoveDirectory removes mounts/junctions even if the target is not empty.
https://codereview.chromium.org/2545283002/diff/20001/base/files/file_util_wi... File base/files/file_util_win.cc (right): https://codereview.chromium.org/2545283002/diff/20001/base/files/file_util_wi... base/files/file_util_win.cc:304: bool TreeDeleter::MoveToParent(const FilePath& path, On 2016/12/04 14:10:46, jschuh (very slow) wrote: > Is there a better place to move to than the parent? Could we first check if one > of the temp resolutions is on the same drive, and if so use that as the target > first? My thought is to manually look for a temp by checking GetTempPath()'s > result, %SystemRoot%\Temp, %USERPROFILE%\AppData\Local\Temp, and ?:\Temp for the > target drive. Any of those just seems like a better location than the parent. I had similar misgivings about doing this, and I'm glad you brought it up. I chose the parent because it's less work and because Niall mentions that this is his strategy in his "Racing the Filesystem" talk. I do like that using some temp-ish dir has the benefit that anything left behind should eventually be reaped by another force. Do you think it's worth the added complexity?
Description was changed from ========== A robust base::DeleteFile. Win32's DeleteFile marks a file for deletion, but makes no guarantees that it disappears. This makes recursive deletes flaky as well as delete-create or delete-move pairs. This flakines leads to install and update failures, especially when scanners are active on the machine. This CL contains a robust file and directory deletion strategy that works as one would expect (delete means delete). Additionally, this CL removes wildcard support from the Windows implementation of base::DeleteFile, resolving a long-standing TODO. BUG=571906 ========== to ========== A robust base::DeleteFile. Win32's DeleteFile marks a file for deletion, but makes no guarantees that it disappears. This makes recursive deletes flaky as well as delete-create or delete-move pairs. This flakines leads to install and update failures, especially when scanners are active on the machine. This CL contains a robust file and directory deletion strategy that works as one would expect (delete means delete). Additionally, this CL removes wildcard support from the Windows implementation of base::DeleteFile, resolving a long-standing TODO. BUG=571906 ==========
grt@chromium.org changed reviewers: + wfh@chromium.org - shess@google.com
+wfh again
shess@chromium.org changed reviewers: + shess@chromium.org
Something I didn't get from the talk was whether the dance of moving the file somewhere else and setting delete-on-close was required, or just a way to address lagging deletes? If the latter, then would it make sense to make a first pass deleting the files, and then make a second pass moving away all of the files for which the delete didn't take? An upside of always following the full-on procedure is that using it as the common case means it's well-exercised. But wouldn't it also possibly cause a lot of work in the filesystem, because the OS can't rely on your code to complete the job? One middle ground between having a delete-only pass and a move-delete pass would be something which makes repeated passes, with each pass aiming to delete while flattening (shifting everything up a level). Hopefully the time it takes to do a complete pass would give the filesystem time to process all of those deletes, minimizing time spent busy-deleting. [Also, did I mention that this is wonderful?] https://codereview.chromium.org/2545283002/diff/20001/base/files/file_util_wi... File base/files/file_util_win.cc (right): https://codereview.chromium.org/2545283002/diff/20001/base/files/file_util_wi... base/files/file_util_win.cc:220: if (read_only_set) { Maybe split this function into a helper, so that you don't lose failures (or maybe I misunderstand why DoDelete failure warrants this reset, but the return-false just above that doesn't). [Hmm. Maybe because DoDelete is taking over to_delete?] https://codereview.chromium.org/2545283002/diff/20001/base/files/file_util_wi... base/files/file_util_win.cc:304: bool TreeDeleter::MoveToParent(const FilePath& path, On 2016/12/04 21:35:46, grt (UTC plus 1) wrote: > On 2016/12/04 14:10:46, jschuh (very slow) wrote: > > Is there a better place to move to than the parent? Could we first check if > one > > of the temp resolutions is on the same drive, and if so use that as the target > > first? My thought is to manually look for a temp by checking GetTempPath()'s > > result, %SystemRoot%\Temp, %USERPROFILE%\AppData\Local\Temp, and ?:\Temp for > the > > target drive. Any of those just seems like a better location than the parent. > > I had similar misgivings about doing this, and I'm glad you brought it up. I > chose the parent because it's less work and because Niall mentions that this is > his strategy in his "Racing the Filesystem" talk. I do like that using some > temp-ish dir has the benefit that anything left behind should eventually be > reaped by another force. Do you think it's worth the added complexity? In most cases in our code, there probably is going to be an obvious place where you could dump deleted items temporarily. At worst, most cases will have a clear root which encloses all other operations. Maybe rather than trying to make this code smart enough to make intelligent guesses, it should instead expose an API that allows the landing directory to be specified directly. Alternately, since I see there is retry logic already, maybe recursive delete could progress by renaming everything to the root, and only renaming slow items out of the root. https://codereview.chromium.org/2545283002/diff/20001/base/files/file_util_wi... base/files/file_util_win.cc:335: .append(".tmp"))); IMHO adding .tmp is just putting lipstick on the pig.
On 2016/12/06 08:15:56, Scott Hess wrote: > Something I didn't get from the talk was whether the dance of moving the file > somewhere else and setting delete-on-close was required, or just a way to > address lagging deletes? If the latter, then would it make sense to make a > first pass deleting the files, and then make a second pass moving away all of > the files for which the delete didn't take? Interesting idea. I wonder if the following would be reliable: - open file - paint for deletion - close file - attempt to re-open file - on success, move out of the way and re-close The downsides I see are that close-open is racy and that i'm not 100% confident that a failure to re-open means that the parent directory can be deleted right away. In one trace I have, I see activity by the System proc on a file a full 1.2 seconds after it was deleted. I don't know if opening that file would have succeeded. Actually, that might be a red herring. I see another case of System accessing a file long after it was deleted, yet the containing dir for that particular file was deleted. I'm scratching my head over that one. James: do you have any thoughts on this? > An upside of always following the full-on procedure is that using it as the > common case means it's well-exercised. But wouldn't it also possibly cause a > lot of work in the filesystem, because the OS can't rely on your code to > complete the job? Yes, there is extra load on the FS due to the move. > One middle ground between having a delete-only pass and a move-delete pass would > be something which makes repeated passes, with each pass aiming to delete while > flattening (shifting everything up a level). Hopefully the time it takes to do > a complete pass would give the filesystem time to process all of those deletes, > minimizing time spent busy-deleting. I'm concerned about the case where we think we've deleted the contents of a directory (because we've marked all files as delete-on-close and enumerating doesn't show anything) but deleting the directory still returns ERROR_DIR_NOT_EMPTY because one of the deletes is taking a while. At that point I think we'd have no choice but to keep retrying until we can delete it. I don't know if this is actually possible (I'm not sure where such guarantees would be documented), but I think it's avoided by using the always-move strategy. > [Also, did I mention that this is wonderful?] Thanks.
I feel like I'm entering the scary door with this CL.
https://codereview.chromium.org/2545283002/diff/20001/base/files/file_util_wi... File base/files/file_util_win.cc (right): https://codereview.chromium.org/2545283002/diff/20001/base/files/file_util_wi... base/files/file_util_win.cc:212: !IsMountPointOrSymlink(to_delete, basic_info) && !DeleteContents(path)) { On 2016/12/04 21:31:48, grt (UTC plus 1) wrote: > On 2016/12/04 14:10:46, jschuh (very slow) wrote: > > On 2016/12/04 08:52:06, grt (UTC plus 1) wrote: > > > i wonder if this is overkill. a directory with a reparse point cannot > contain > > > files, so perhaps it's correct enough to not recurse if |to_delete| is a > > > directory and has a reparse point. thoughts? > > > > For IsMountPointOrSymlink()? Doesn't seem like overkill to me. > > I'm not too familiar with the other uses for reparse points. > https://msdn.microsoft.com/en-us/library/dd541667.aspx lists the possibilities > as defined by MS, but 3rd parties can define their own. Since MSDN says that a > directory with a reparse point may not contain files, it seems to be reasonably > conservative to never recurse into them. Or are there cases where it really is > the right thing? Of note: Win32's DeleteFile operates on links rather than > targets and Win32's RemoveDirectory removes mounts/junctions even if the target > is not empty. Ah, I see. I think it would probably be safe enough to test just for FILE_ATTRIBUTE_REPARSE_POINT, because recursively deleting where we shouldn't is probably worse than not doing it. https://codereview.chromium.org/2545283002/diff/20001/base/files/file_util_wi... base/files/file_util_win.cc:304: bool TreeDeleter::MoveToParent(const FilePath& path, On 2016/12/04 21:35:46, grt (UTC plus 1) wrote: > On 2016/12/04 14:10:46, jschuh (very slow) wrote: > > Is there a better place to move to than the parent? Could we first check if > one > > of the temp resolutions is on the same drive, and if so use that as the target > > first? My thought is to manually look for a temp by checking GetTempPath()'s > > result, %SystemRoot%\Temp, %USERPROFILE%\AppData\Local\Temp, and ?:\Temp for > the > > target drive. Any of those just seems like a better location than the parent. > > I had similar misgivings about doing this, and I'm glad you brought it up. I > chose the parent because it's less work and because Niall mentions that this is > his strategy in his "Racing the Filesystem" talk. I do like that using some > temp-ish dir has the benefit that anything left behind should eventually be > reaped by another force. Do you think it's worth the added complexity? I just worry about accumulating a bunch of files in an odd location. But I trust your judgement if you don't think it's worth the complexity. https://codereview.chromium.org/2545283002/diff/20001/base/files/file_util_wi... base/files/file_util_win.cc:335: .append(".tmp"))); On 2016/12/06 08:15:56, Scott Hess wrote: > IMHO adding .tmp is just putting lipstick on the pig. I disagree. I think this is a good indicator if someone stumbles across this file after it should have been deleted.
Some comments addressed and some questions raised. https://codereview.chromium.org/2545283002/diff/20001/base/files/file_util_wi... File base/files/file_util_win.cc (right): https://codereview.chromium.org/2545283002/diff/20001/base/files/file_util_wi... base/files/file_util_win.cc:212: !IsMountPointOrSymlink(to_delete, basic_info) && !DeleteContents(path)) { On 2016/12/06 22:16:34, jschuh (very slow) wrote: > On 2016/12/04 21:31:48, grt (UTC plus 1) wrote: > > On 2016/12/04 14:10:46, jschuh (very slow) wrote: > > > On 2016/12/04 08:52:06, grt (UTC plus 1) wrote: > > > > i wonder if this is overkill. a directory with a reparse point cannot > > contain > > > > files, so perhaps it's correct enough to not recurse if |to_delete| is a > > > > directory and has a reparse point. thoughts? > > > > > > For IsMountPointOrSymlink()? Doesn't seem like overkill to me. > > > > I'm not too familiar with the other uses for reparse points. > > https://msdn.microsoft.com/en-us/library/dd541667.aspx lists the possibilities > > as defined by MS, but 3rd parties can define their own. Since MSDN says that a > > directory with a reparse point may not contain files, it seems to be > reasonably > > conservative to never recurse into them. Or are there cases where it really is > > the right thing? Of note: Win32's DeleteFile operates on links rather than > > targets and Win32's RemoveDirectory removes mounts/junctions even if the > target > > is not empty. > > Ah, I see. I think it would probably be safe enough to test just for > FILE_ATTRIBUTE_REPARSE_POINT, because recursively deleting where we shouldn't is > probably worse than not doing it. Done. https://codereview.chromium.org/2545283002/diff/20001/base/files/file_util_wi... base/files/file_util_win.cc:220: if (read_only_set) { On 2016/12/06 08:15:56, Scott Hess wrote: > Maybe split this function into a helper, so that you don't lose failures (or > maybe I misunderstand why DoDelete failure warrants this reset, but the > return-false just above that doesn't). > > [Hmm. Maybe because DoDelete is taking over to_delete?] Nice catch! Indeed, the block above belongs in DoDelete so that the directory is set back to read-only on failure. https://codereview.chromium.org/2545283002/diff/20001/base/files/file_util_wi... base/files/file_util_win.cc:258: } while (items_failed && ++retries < 5); wdyt of a short sleep here to give the system time to process whatever is in the directory? maybe only a single retry with a short sleep? https://codereview.chromium.org/2545283002/diff/20001/base/files/file_util_wi... base/files/file_util_win.cc:304: bool TreeDeleter::MoveToParent(const FilePath& path, On 2016/12/06 22:16:34, jschuh (very slow) wrote: > On 2016/12/04 21:35:46, grt (UTC plus 1) wrote: > > On 2016/12/04 14:10:46, jschuh (very slow) wrote: > > > Is there a better place to move to than the parent? Could we first check if > > one > > > of the temp resolutions is on the same drive, and if so use that as the > target > > > first? My thought is to manually look for a temp by checking GetTempPath()'s > > > result, %SystemRoot%\Temp, %USERPROFILE%\AppData\Local\Temp, and ?:\Temp for > > the > > > target drive. Any of those just seems like a better location than the > parent. > > > > I had similar misgivings about doing this, and I'm glad you brought it up. I > > chose the parent because it's less work and because Niall mentions that this > is > > his strategy in his "Racing the Filesystem" talk. I do like that using some > > temp-ish dir has the benefit that anything left behind should eventually be > > reaped by another force. Do you think it's worth the added complexity? > > I just worry about accumulating a bunch of files in an odd location. But I trust > your judgement if you don't think it's worth the complexity. The next two things on my plate are: - Survey current callers to see if most/all have a logical directory that should be the destination. There are more than a few. :-( - Try out a "search for TEMP". I'm convincing myself that TEMP is better if it's on the same volume. If it's not (and X;\Temp doesn't exist), then moving up the tree may be okay. Maybe most deletes outside of the installer are in the user data dir somewhere. - Another thought: for everything on the same volume as the user data dir, move into a temp dir in UDD. We could have a low-priority purge pass after browser launch to clean up stragglers. I would love it if we could make base::DeleteFile "just work", but maybe we will need either extra Win-specific APIs to set a temp location or make a cleanup pass. https://codereview.chromium.org/2545283002/diff/20001/base/files/file_util_wi... base/files/file_util_win.cc:335: .append(".tmp"))); On 2016/12/06 22:16:34, jschuh (very slow) wrote: > On 2016/12/06 08:15:56, Scott Hess wrote: > > IMHO adding .tmp is just putting lipstick on the pig. > > I disagree. I think this is a good indicator if someone stumbles across this > file after it should have been deleted. This was my thinking exactly.
> James: do you have any thoughts on this? It's likely that even when the file is nominally deleted the file still exists in the kernel. The only requirement AFAIK is that there are open handles to the file, the kernel can keep a reference to a FILE object without needing a handle and so the file can be removed from the directory but still exists to be written to. It's also possible the writes you see are actually the cache manager which is why typically you don't show the System process in Process Monitor. I think this best effort is about all we can do, unfortunately not only does NTOS have weird locking but it also has a very annoying behaviour when files are mapped as sections. In that case if you try and mark a file for deletion which has been mapped you'll be refused. However you can mark the file to be deleted before it's mapped, the only problem here is that if all the handles are closed to the file _before_ the file is unmapped the kernel straight up ignores the delete, and it seems to treat it as one shot so if it fails due to it still being mapped it will never try it again. This might explain cases where you mark a file as deleted but in fact it's never deleted if say AV came in at the last minute opened the handle and mapped it for inspection.
On 2016/12/07 10:46:10, forshaw wrote: > > James: do you have any thoughts on this? > > It's likely that even when the file is nominally deleted the file still exists > in the kernel. The only requirement AFAIK is that there are open handles to the > file, the kernel can keep a reference to a FILE object without needing a handle > and so the file can be removed from the directory but still exists to be written > to. It's also possible the writes you see are actually the cache manager which > is why typically you don't show the System process in Process Monitor. > > I think this best effort is about all we can do, unfortunately not only does > NTOS have weird locking but it also has a very annoying behaviour when files are > mapped as sections. In that case if you try and mark a file for deletion which > has been mapped you'll be refused. However you can mark the file to be deleted > before it's mapped, the only problem here is that if all the handles are closed > to the file _before_ the file is unmapped the kernel straight up ignores the > delete, and it seems to treat it as one shot so if it fails due to it still > being mapped it will never try it again. This might explain cases where you mark > a file as deleted but in fact it's never deleted if say AV came in at the last > minute opened the handle and mapped it for inspection. Thanks, James. By "this best effort", do you mean the currently-implemented approach, or the one I suggested in https://codereview.chromium.org/2545283002/#msg22 (which I think is more risky, but I'm not certain)?
The CQ bit was checked by grt@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...
https://codereview.chromium.org/2545283002/diff/20001/base/files/file_util_wi... File base/files/file_util_win.cc (right): https://codereview.chromium.org/2545283002/diff/20001/base/files/file_util_wi... base/files/file_util_win.cc:304: bool TreeDeleter::MoveToParent(const FilePath& path, On 2016/12/07 08:34:39, grt (UTC plus 1) wrote: > On 2016/12/06 22:16:34, jschuh (very slow) wrote: > > On 2016/12/04 21:35:46, grt (UTC plus 1) wrote: > > > On 2016/12/04 14:10:46, jschuh (very slow) wrote: > > > > Is there a better place to move to than the parent? Could we first check > if > > > one > > > > of the temp resolutions is on the same drive, and if so use that as the > > target > > > > first? My thought is to manually look for a temp by checking > GetTempPath()'s > > > > result, %SystemRoot%\Temp, %USERPROFILE%\AppData\Local\Temp, and ?:\Temp > for > > > the > > > > target drive. Any of those just seems like a better location than the > > parent. > > > > > > I had similar misgivings about doing this, and I'm glad you brought it up. I > > > chose the parent because it's less work and because Niall mentions that this > > is > > > his strategy in his "Racing the Filesystem" talk. I do like that using some > > > temp-ish dir has the benefit that anything left behind should eventually be > > > reaped by another force. Do you think it's worth the added complexity? > > > > I just worry about accumulating a bunch of files in an odd location. But I > trust > > your judgement if you don't think it's worth the complexity. > > The next two things on my plate are: > - Survey current callers to see if most/all have a logical directory that should > be the destination. There are more than a few. :-( > - Try out a "search for TEMP". I'm convincing myself that TEMP is better if it's > on the same volume. If it's not (and X;\Temp doesn't exist), then moving up the > tree may be okay. Maybe most deletes outside of the installer are in the user > data dir somewhere. I've added this. PTAL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by grt@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...
siggi@chromium.org changed reviewers: + siggi@chromium.org
Didn't review in great detail, but looks sane to me. https://codereview.chromium.org/2545283002/diff/80001/base/files/file_util_wi... File base/files/file_util_win.cc (right): https://codereview.chromium.org/2545283002/diff/80001/base/files/file_util_wi... base/files/file_util_win.cc:46: // directory deletion strategy to work with Windows' pecurlarity around file nit: pecurlarity? https://codereview.chromium.org/2545283002/diff/80001/base/files/file_util_wi... base/files/file_util_win.cc:55: // - Open the item to be deleted. This sounds like a single post-order traversal on the directory hierarchy. As we discussed on chat, by doing two passes you can pre-flight by doing OpenFile to test that all the files and dirs grant DELETE and/or not open without FILE_SHARE_DELETE. This would allow you more of an all-or-nothing success or failure. I don't know whether that's desirable here, and it's another metric ****ton of work. https://codereview.chromium.org/2545283002/diff/80001/base/files/file_util_wi... base/files/file_util_win.cc:58: // plain old dirctory, not a mount point or a symlinks), recursively delete nit: dirctory https://codereview.chromium.org/2545283002/diff/80001/base/files/file_util_wi... base/files/file_util_win.cc:80: class TreeDeleter { Maybe you'd want to expose this class for more nuanced users, like the installer, if only for the ability to get detailed metrics on how it did? Alternatively you could expose a DeleteFileWithCallback that'd allow the installer to call in here and still have its metrics cake? https://codereview.chromium.org/2545283002/diff/80001/base/files/file_util_wi... base/files/file_util_win.cc:188: temp_dir_path_ = temp_path; Do you also need to test whether you can create or write to the temp directory? https://codereview.chromium.org/2545283002/diff/80001/base/files/file_util_wi... base/files/file_util_win.cc:223: ChooseTempDirIfNotChosen(path, to_delete); nit: EnsureTempDirChosen? https://codereview.chromium.org/2545283002/diff/80001/base/files/file_util_wi... base/files/file_util_win.cc:283: // explorer.exe) observing the temp directory have a greater liklihood of nit: likelihood? https://codereview.chromium.org/2545283002/diff/80001/base/files/file_util_wi... base/files/file_util_win.cc:323: // the temp directory before deletion. ah - oki - so failure to move to the temp dir is not fatal, or is it? https://codereview.chromium.org/2545283002/diff/80001/base/files/file_util_wi... base/files/file_util_win.cc:378: // This is highly unexpected since the file was to be deleted moments ago. I could see this happen if there are things on the system watching all moves to scan all the things for your "security". I wonder if AVs back off on scanning .tmp files in %TMP% dirs, as a rule.
Didn't review in great detail, but looks sane to me.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
few questions https://codereview.chromium.org/2545283002/diff/80001/base/files/file_util_wi... File base/files/file_util_win.cc (right): https://codereview.chromium.org/2545283002/diff/80001/base/files/file_util_wi... base/files/file_util_win.cc:45: // The workhorse of base::DeleteFile. This class implements a robust file and how much of this comment should be exposed via the main base::DeleteFile header file (file_util.h)? are partial successes exposed up via that interface? https://codereview.chromium.org/2545283002/diff/80001/base/files/file_util_wi... base/files/file_util_win.cc:63: // - Move the item up one directory (with a random name). should the public header file describe that one of the constraints of using this function is that the caller has write access to the parent directory? Are checks made before starting any of these operations that this is the case?
The CQ bit was checked by grt@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 checked by grt@chromium.org to run a CQ dry run
Patchset #6 (id:100001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL https://codereview.chromium.org/2545283002/diff/80001/base/files/file_util_wi... File base/files/file_util_win.cc (right): https://codereview.chromium.org/2545283002/diff/80001/base/files/file_util_wi... base/files/file_util_win.cc:45: // The workhorse of base::DeleteFile. This class implements a robust file and On 2016/12/07 19:47:43, Will Harris wrote: > how much of this comment should be exposed via the main base::DeleteFile header > file (file_util.h)? Updated! > are partial successes exposed up via that interface? I documented what I think is necessary for callers to know. Let me know if you think more is needed. Thanks! https://codereview.chromium.org/2545283002/diff/80001/base/files/file_util_wi... base/files/file_util_win.cc:46: // directory deletion strategy to work with Windows' pecurlarity around file On 2016/12/07 16:29:18, Sigurður Ásgeirsson wrote: > nit: pecurlarity? what, you don't have that word in Icelandic? :-) https://codereview.chromium.org/2545283002/diff/80001/base/files/file_util_wi... base/files/file_util_win.cc:55: // - Open the item to be deleted. On 2016/12/07 16:29:19, Sigurður Ásgeirsson wrote: > This sounds like a single post-order traversal on the directory hierarchy. > > As we discussed on chat, by doing two passes you can pre-flight by doing > OpenFile to test that all the files and dirs grant DELETE and/or not open > without FILE_SHARE_DELETE. This would allow you more of an all-or-nothing > success or failure. > I don't know whether that's desirable here, and it's another metric ****ton of > work. From the caller's point of view, I'm not sure what the difference is between "bail on first error" (leaving things in a completely unknown state on failure) vs. "best effort" (leaving things in a completely unknown state on failure) aside from the latter potentially freeing up more disk space. I'd rather have the disk space. :-) https://codereview.chromium.org/2545283002/diff/80001/base/files/file_util_wi... base/files/file_util_win.cc:58: // plain old dirctory, not a mount point or a symlinks), recursively delete On 2016/12/07 16:29:18, Sigurður Ásgeirsson wrote: > nit: dirctory Done. https://codereview.chromium.org/2545283002/diff/80001/base/files/file_util_wi... base/files/file_util_win.cc:63: // - Move the item up one directory (with a random name). On 2016/12/07 19:47:43, Will Harris wrote: > should the public header file describe that one of the constraints of using this > function is that the caller has write access to the parent directory? It's not a constraint. If the caller has write access to a temp dir or the parent, the item(s) will be moved. If not, they will be deleted in place. > Are checks made before starting any of these operations that this is the case? No. The moves will fail (in MoveToTemp) but the deletes will proceed. It does makes sense, though, for the temp probing code to check for writability so that if X:\Temp can't be written to it tries the parent dir and fallsback to using the item's own dir as a last resort. Thanks. https://codereview.chromium.org/2545283002/diff/80001/base/files/file_util_wi... base/files/file_util_win.cc:80: class TreeDeleter { On 2016/12/07 16:29:18, Sigurður Ásgeirsson wrote: > Maybe you'd want to expose this class for more nuanced users, like the > installer, if only for the ability to get detailed metrics on how it did? > > Alternatively you could expose a DeleteFileWithCallback that'd allow the > installer to call in here and still have its metrics cake? I was mulling over metrics options. My inclination is to keep this class private since it's all implementation-detail-goop. Adding a callback for metrics is a nice idea. Do you think it's nonsensical to put some histograms in the impl itself for now and read canary data before deciding if it's worth the effort to expose fine-grained metrics to callers? https://codereview.chromium.org/2545283002/diff/80001/base/files/file_util_wi... base/files/file_util_win.cc:164: FILE_ID_INFO item_id_info = {}; turns out this wasn't supported on Win7. https://codereview.chromium.org/2545283002/diff/80001/base/files/file_util_wi... base/files/file_util_win.cc:188: temp_dir_path_ = temp_path; On 2016/12/07 16:29:18, Sigurður Ásgeirsson wrote: > Do you also need to test whether you can create or write to the temp directory? Hmm. Yeah, that makes sense. If X:\Temp can't be written to, it's better to try the parent dir and fallback to using the item's own dir as a last resort. Thanks. https://codereview.chromium.org/2545283002/diff/80001/base/files/file_util_wi... base/files/file_util_win.cc:223: ChooseTempDirIfNotChosen(path, to_delete); On 2016/12/07 16:29:18, Sigurður Ásgeirsson wrote: > nit: EnsureTempDirChosen? Resolved the odd name otherwise. https://codereview.chromium.org/2545283002/diff/80001/base/files/file_util_wi... base/files/file_util_win.cc:283: // explorer.exe) observing the temp directory have a greater liklihood of On 2016/12/07 16:29:18, Sigurður Ásgeirsson wrote: > nit: likelihood? Done. (BTW: Damn, you're good. :-) ) https://codereview.chromium.org/2545283002/diff/80001/base/files/file_util_wi... base/files/file_util_win.cc:323: // the temp directory before deletion. On 2016/12/07 16:29:18, Sigurður Ásgeirsson wrote: > ah - oki - so failure to move to the temp dir is not fatal, or is it? Not fatal. I've improved things a bit so that the rename is still likely to happen if nothing other than the root directory being deleted could be written to. https://codereview.chromium.org/2545283002/diff/80001/base/files/file_util_wi... base/files/file_util_win.cc:378: // This is highly unexpected since the file was to be deleted moments ago. On 2016/12/07 16:29:19, Sigurður Ásgeirsson wrote: > I could see this happen if there are things on the system watching all moves to > scan all the things for your "security". I wonder if AVs back off on scanning > .tmp files in %TMP% dirs, as a rule. We're doing this on a handle that we opened with DELETE access. A scanner that opened the same file would have to have used SHARE_DELETE, so I'm not sure how they could interfere unless they were doing crazycrazy in the kernel. Or am I forgetting about something again?
On 2016/12/07 20:56:06, grt (UTC plus 1) wrote: > PTAL > > https://codereview.chromium.org/2545283002/diff/80001/base/files/file_util_wi... > File base/files/file_util_win.cc (right): > > https://codereview.chromium.org/2545283002/diff/80001/base/files/file_util_wi... > base/files/file_util_win.cc:45: // The workhorse of base::DeleteFile. This class > implements a robust file and > On 2016/12/07 19:47:43, Will Harris wrote: > > how much of this comment should be exposed via the main base::DeleteFile > header > > file (file_util.h)? > > Updated! > > > are partial successes exposed up via that interface? > > I documented what I think is necessary for callers to know. Let me know if you > think more is needed. Thanks! > > https://codereview.chromium.org/2545283002/diff/80001/base/files/file_util_wi... > base/files/file_util_win.cc:46: // directory deletion strategy to work with > Windows' pecurlarity around file > On 2016/12/07 16:29:18, Sigurður Ásgeirsson wrote: > > nit: pecurlarity? > > what, you don't have that word in Icelandic? :-) > > https://codereview.chromium.org/2545283002/diff/80001/base/files/file_util_wi... > base/files/file_util_win.cc:55: // - Open the item to be deleted. > On 2016/12/07 16:29:19, Sigurður Ásgeirsson wrote: > > This sounds like a single post-order traversal on the directory hierarchy. > > > > As we discussed on chat, by doing two passes you can pre-flight by doing > > OpenFile to test that all the files and dirs grant DELETE and/or not open > > without FILE_SHARE_DELETE. This would allow you more of an all-or-nothing > > success or failure. > > I don't know whether that's desirable here, and it's another metric ****ton of > > work. > > From the caller's point of view, I'm not sure what the difference is between > "bail on first error" (leaving things in a completely unknown state on failure) > vs. "best effort" (leaving things in a completely unknown state on failure) > aside from the latter potentially freeing up more disk space. I'd rather have > the disk space. :-) > > https://codereview.chromium.org/2545283002/diff/80001/base/files/file_util_wi... > base/files/file_util_win.cc:58: // plain old dirctory, not a mount point or a > symlinks), recursively delete > On 2016/12/07 16:29:18, Sigurður Ásgeirsson wrote: > > nit: dirctory > > Done. > > https://codereview.chromium.org/2545283002/diff/80001/base/files/file_util_wi... > base/files/file_util_win.cc:63: // - Move the item up one directory (with a > random name). > On 2016/12/07 19:47:43, Will Harris wrote: > > should the public header file describe that one of the constraints of using > this > > function is that the caller has write access to the parent directory? > > It's not a constraint. If the caller has write access to a temp dir or the > parent, the item(s) will be moved. If not, they will be deleted in place. > > > Are checks made before starting any of these operations that this is the case? > > No. The moves will fail (in MoveToTemp) but the deletes will proceed. > > It does makes sense, though, for the temp probing code to check for writability > so that if X:\Temp can't be written to it tries the parent dir and fallsback to > using the item's own dir as a last resort. Thanks. > > https://codereview.chromium.org/2545283002/diff/80001/base/files/file_util_wi... > base/files/file_util_win.cc:80: class TreeDeleter { > On 2016/12/07 16:29:18, Sigurður Ásgeirsson wrote: > > Maybe you'd want to expose this class for more nuanced users, like the > > installer, if only for the ability to get detailed metrics on how it did? > > > > Alternatively you could expose a DeleteFileWithCallback that'd allow the > > installer to call in here and still have its metrics cake? > > I was mulling over metrics options. My inclination is to keep this class private > since it's all implementation-detail-goop. Adding a callback for metrics is a > nice idea. Do you think it's nonsensical to put some histograms in the impl > itself for now and read canary data before deciding if it's worth the effort to > expose fine-grained metrics to callers? > > https://codereview.chromium.org/2545283002/diff/80001/base/files/file_util_wi... > base/files/file_util_win.cc:164: FILE_ID_INFO item_id_info = {}; > turns out this wasn't supported on Win7. > > https://codereview.chromium.org/2545283002/diff/80001/base/files/file_util_wi... > base/files/file_util_win.cc:188: temp_dir_path_ = temp_path; > On 2016/12/07 16:29:18, Sigurður Ásgeirsson wrote: > > Do you also need to test whether you can create or write to the temp > directory? > > Hmm. Yeah, that makes sense. If X:\Temp can't be written to, it's better to try > the parent dir and fallback to using the item's own dir as a last resort. > Thanks. > > https://codereview.chromium.org/2545283002/diff/80001/base/files/file_util_wi... > base/files/file_util_win.cc:223: ChooseTempDirIfNotChosen(path, to_delete); > On 2016/12/07 16:29:18, Sigurður Ásgeirsson wrote: > > nit: EnsureTempDirChosen? > > Resolved the odd name otherwise. > > https://codereview.chromium.org/2545283002/diff/80001/base/files/file_util_wi... > base/files/file_util_win.cc:283: // explorer.exe) observing the temp directory > have a greater liklihood of > On 2016/12/07 16:29:18, Sigurður Ásgeirsson wrote: > > nit: likelihood? > > Done. > > (BTW: Damn, you're good. :-) ) > > https://codereview.chromium.org/2545283002/diff/80001/base/files/file_util_wi... > base/files/file_util_win.cc:323: // the temp directory before deletion. > On 2016/12/07 16:29:18, Sigurður Ásgeirsson wrote: > > ah - oki - so failure to move to the temp dir is not fatal, or is it? > > Not fatal. I've improved things a bit so that the rename is still likely to > happen if nothing other than the root directory being deleted could be written > to. > > https://codereview.chromium.org/2545283002/diff/80001/base/files/file_util_wi... > base/files/file_util_win.cc:378: // This is highly unexpected since the file was > to be deleted moments ago. > On 2016/12/07 16:29:19, Sigurður Ásgeirsson wrote: > > I could see this happen if there are things on the system watching all moves > to > > scan all the things for your "security". I wonder if AVs back off on scanning > > .tmp files in %TMP% dirs, as a rule. > > We're doing this on a handle that we opened with DELETE access. A scanner that > opened the same file would have to have used SHARE_DELETE, so I'm not sure how > they could interfere unless they were doing crazycrazy in the kernel. Or am I > forgetting about something again? I second siggi's thoughts on exposing more of the result rather than just a bool otherwise, or in addition, regardless, lgtm.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #7 (id:140001) has been deleted
The CQ bit was checked by grt@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...
PTAL. I've added a way for callers to get metrics, and have rejiggered the retry logic. Rather than retrying individual items within a directory during a recursive delete, it now retires the operation globally until either success or no progress after four retries. I think this is better in that it should limit the total number of retries and it should allow a bit more space for other procs and the OS to get out of the way. I plan to add some UMA in the installer making use of what's exposed here. WDYT?
Sadly I don't have time to moar review till Monday, so don't hold this up for me :). https://codereview.chromium.org/2545283002/diff/80001/base/files/file_util_wi... File base/files/file_util_win.cc (right): https://codereview.chromium.org/2545283002/diff/80001/base/files/file_util_wi... base/files/file_util_win.cc:378: // This is highly unexpected since the file was to be deleted moments ago. On 2016/12/07 20:56:06, grt (UTC plus 1) wrote: > On 2016/12/07 16:29:19, Sigurður Ásgeirsson wrote: > > I could see this happen if there are things on the system watching all moves > to > > scan all the things for your "security". I wonder if AVs back off on scanning > > .tmp files in %TMP% dirs, as a rule. > > We're doing this on a handle that we opened with DELETE access. A scanner that > opened the same file would have to have used SHARE_DELETE, so I'm not sure how > they could interfere unless they were doing crazycrazy in the kernel. Or am I > forgetting about something again? Use the source, (young) Padawan <https://github.com/Microsoft/Windows-driver-samples/blob/master/filesys/fastf...>. Looks like the file can become non-deletable if someone maps it in the interrim, there's a smattering of other failure conditions.
/exit stage left to do my chores. https://codereview.chromium.org/2545283002/diff/160001/base/files/file_util.h File base/files/file_util.h (right): https://codereview.chromium.org/2545283002/diff/160001/base/files/file_util.h... base/files/file_util.h:64: // possible will always be deleted. Returns true if |path| was deleted or did maybe some words that this is constrained to a single volume? https://codereview.chromium.org/2545283002/diff/160001/base/files/file_util.h... base/files/file_util.h:75: struct DeleteFileMetrics { nice and simple, I like it.
/exit stage left to do my chores.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
On 2016/12/08 14:29:26, Sigurður Ásgeirsson wrote: > Sadly I don't have time to moar review till Monday, so don't hold this up for me > :). > > https://codereview.chromium.org/2545283002/diff/80001/base/files/file_util_wi... > File base/files/file_util_win.cc (right): > > https://codereview.chromium.org/2545283002/diff/80001/base/files/file_util_wi... > base/files/file_util_win.cc:378: // This is highly unexpected since the file was > to be deleted moments ago. > On 2016/12/07 20:56:06, grt (UTC plus 1) wrote: > > On 2016/12/07 16:29:19, Sigurður Ásgeirsson wrote: > > > I could see this happen if there are things on the system watching all moves > > to > > > scan all the things for your "security". I wonder if AVs back off on > scanning > > > .tmp files in %TMP% dirs, as a rule. > > > > We're doing this on a handle that we opened with DELETE access. A scanner that > > opened the same file would have to have used SHARE_DELETE, so I'm not sure how > > they could interfere unless they were doing crazycrazy in the kernel. Or am I > > forgetting about something again? > > Use the source, (young) Padawan > <https://github.com/Microsoft/Windows-driver-samples/blob/master/filesys/fastf...>. > Looks like the file can become non-deletable if someone maps it in the interrim, > there's a smattering of other failure conditions. Nice! Thanks for the pointer.
The CQ bit was checked by grt@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...
https://codereview.chromium.org/2545283002/diff/160001/base/files/file_util.h File base/files/file_util.h (right): https://codereview.chromium.org/2545283002/diff/160001/base/files/file_util.h... base/files/file_util.h:64: // possible will always be deleted. Returns true if |path| was deleted or did On 2016/12/08 14:36:46, Sigurður Ásgeirsson wrote: > maybe some words that this is constrained to a single volume? Done. https://codereview.chromium.org/2545283002/diff/160001/base/files/file_util.h... base/files/file_util.h:75: struct DeleteFileMetrics { On 2016/12/08 14:36:46, Sigurður Ásgeirsson wrote: > nice and simple, I like it. Thanks.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
grt@chromium.org changed reviewers: + thakis@chromium.org
+thakis for base/OWNERS review. please review the CL this depends on, too (base32 in base). thanks!
The CQ bit was checked by grt@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: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
I just stumbled upon DatabaseTracker::DeleteOrigin, which can be simplified once this lands -- it has its own "move files into a temp directory so that a delete succeeds". That should be unnecessary with this robust delete since it takes care of that automagically.
The main code for this is very readable, thanks! It's pretty scary that this much code is needed to delete a file though. I'm no Windows expert, so I'll take your word for it and I'll assume wfh reviewed this "for windows" already. Did you do any principled testing to arrive on the set of techniques you're using? Do you have some test driver that sets up a bunch of volumes (some of them low on disk space, say) with a bunch of directories, which then stats a few processes walking those trees, and then try to delete dirs in there using this code with both approaches? And then each technique increases success percentage somehow? Also, TreeDeleter makes file_util > 30% larger; maybe it should be in its own file? https://codereview.chromium.org/2545283002/diff/200001/base/files/file_util.h File base/files/file_util.h (right): https://codereview.chromium.org/2545283002/diff/200001/base/files/file_util.h... base/files/file_util.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. should BUG= include 674135 too? https://codereview.chromium.org/2545283002/diff/200001/base/files/file_util.h... base/files/file_util.h:58: // Deletes the given path. If |path| names a directory and |recursive| is false, should BUG= include 674135 too? https://codereview.chromium.org/2545283002/diff/200001/base/files/file_util_u... File base/files/file_util_unittest.cc (right): https://codereview.chromium.org/2545283002/diff/200001/base/files/file_util_u... base/files/file_util_unittest.cc:200: ASSERT_NE(INVALID_FILE_ATTRIBUTES, attrs); I thought ASSERT doesn't do the right thing in helper functions (it returns from the current frame but the calling frame will just continue) https://codereview.chromium.org/2545283002/diff/200001/base/files/file_util_u... base/files/file_util_unittest.cc:711: // Create a read-only file nit: style guide says comments should be sentences ending with punctuation (also elsewhere) https://codereview.chromium.org/2545283002/diff/200001/base/files/file_util_w... File base/files/file_util_win.cc (right): https://codereview.chromium.org/2545283002/diff/200001/base/files/file_util_w... base/files/file_util_win.cc:179: return Base32Encode(StringPiece(&random[0], sizeof(random))).append(".tmp"); what's wrong with win32's GetTempFileName()? https://codereview.chromium.org/2545283002/diff/200001/base/files/file_util_w... base/files/file_util_win.cc:265: // Try the parent dir of |path|. xxx https://codereview.chromium.org/2545283002/diff/200001/base/files/file_util_w... base/files/file_util_win.cc:469: // a final attempt to mark the file for deletion. Why would this succeed now?
https://codereview.chromium.org/2545283002/diff/200001/base/files/file_util_w... File base/files/file_util_win.cc (right): https://codereview.chromium.org/2545283002/diff/200001/base/files/file_util_w... base/files/file_util_win.cc:265: // Try the parent dir of |path|. On 2017/01/10 18:31:56, Nico wrote: > xxx Sorry, meant to edit this one before hitting publish. If you put the temp dir into the parent directory, the `enumer` for the parent won't have the new temp file you're creating, will it?
Description was changed from ========== A robust base::DeleteFile. Win32's DeleteFile marks a file for deletion, but makes no guarantees that it disappears. This makes recursive deletes flaky as well as delete-create or delete-move pairs. This flakines leads to install and update failures, especially when scanners are active on the machine. This CL contains a robust file and directory deletion strategy that works as one would expect (delete means delete). Additionally, this CL removes wildcard support from the Windows implementation of base::DeleteFile, resolving a long-standing TODO. BUG=571906 ========== to ========== A robust base::DeleteFile. Win32's DeleteFile marks a file for deletion, but makes no guarantees that it disappears. This makes recursive deletes flaky as well as delete-create or delete-move pairs. This flakines leads to install and update failures, especially when scanners are active on the machine. This CL contains a robust file and directory deletion strategy that works as one would expect (delete means delete). Additionally, this CL removes wildcard support from the Windows implementation of base::DeleteFile, resolving a long-standing TODO. BUG=571906,674135 ==========
Thanks. I was hoping to get sign-off on this approach before doing some investing more time in tests. I have built a mini_installer with this change and asked two users with consistent repros for https://crbug.com/599084 to try it out. It worked like a charm. My next steps are: - Rebase on top of https://codereview.chromium.org/2567383002/. - Move the impl into its own file as per your request. - Figure out a sane way to get metrics to see how it's working in the field. - Build some kind of psycho stress test. https://codereview.chromium.org/2545283002/diff/200001/base/files/file_util.h File base/files/file_util.h (right): https://codereview.chromium.org/2545283002/diff/200001/base/files/file_util.h... base/files/file_util.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2017/01/10 18:31:55, Nico wrote: > should BUG= include 674135 too? May as well since a comment in that issue references this CR. Thanks. https://codereview.chromium.org/2545283002/diff/200001/base/files/file_util_u... File base/files/file_util_unittest.cc (right): https://codereview.chromium.org/2545283002/diff/200001/base/files/file_util_u... base/files/file_util_unittest.cc:200: ASSERT_NE(INVALID_FILE_ATTRIBUTES, attrs); On 2017/01/10 18:31:55, Nico wrote: > I thought ASSERT doesn't do the right thing in helper functions (it returns from > the current frame but the calling frame will just continue) Indeed! I've added ASSERT_NO_FATAL_FAILURE where relevant. Thanks. https://codereview.chromium.org/2545283002/diff/200001/base/files/file_util_u... base/files/file_util_unittest.cc:711: // Create a read-only file On 2017/01/10 18:31:55, Nico wrote: > nit: style guide says comments should be sentences ending with punctuation (also > elsewhere) Done. https://codereview.chromium.org/2545283002/diff/200001/base/files/file_util_w... File base/files/file_util_win.cc (right): https://codereview.chromium.org/2545283002/diff/200001/base/files/file_util_w... base/files/file_util_win.cc:179: return Base32Encode(StringPiece(&random[0], sizeof(random))).append(".tmp"); On 2017/01/10 18:31:56, Nico wrote: > what's wrong with win32's GetTempFileName()? I think it's undesirable here for a few reasons: - If you want it to guarantee a unique filename, the documentation says it's inefficient in directories containing lots of files. Temp dirs often have lots of files, so this seems bad. - If you want it to guarantee a unique filename, it creates a file with that name and leaves it there for you. This file is then in the way of the file we want to move, so we'd have to delete it or rename it, which kinda puts us back to where we started. :-) - If you don't want it to guarantee a unique filename, you have to call it repeatedly in case of collision, which is no better than the roll-your-own I've created here. https://codereview.chromium.org/2545283002/diff/200001/base/files/file_util_w... base/files/file_util_win.cc:265: // Try the parent dir of |path|. On 2017/01/10 18:39:51, Nico wrote: > On 2017/01/10 18:31:56, Nico wrote: > > xxx > > Sorry, meant to edit this one before hitting publish. If you put the temp dir > into the parent directory, the `enumer` for the parent won't have the new temp > file you're creating, will it? No, the enumerator used in DeleteContents will never see items that are moved into either the parent dir of |path| or the dir of |path|. ChooseTempDir is called only once with the path given to RobustDelete. In the case where RobustDelete is called, |path| names a directory, and |recursive| is true, enumeration is done within the directory |path| (and its children) and in the directory containing |path|. https://codereview.chromium.org/2545283002/diff/200001/base/files/file_util_w... base/files/file_util_win.cc:469: // a final attempt to mark the file for deletion. On 2017/01/10 18:31:56, Nico wrote: > Why would this succeed now? Races. It's possible that another proc had mapped the file into memory (https://github.com/Microsoft/Windows-driver-samples/blob/master/filesys/fastf...), like, I dunno, an AV scanner...
Yes, overall approach looks good to me. I think having some way (e.g. the psycho
test suite you mention) to evaluate future changes ("adding another attempt at
deletion here increases test success rate by 5%") to this code is important
though.
https://codereview.chromium.org/2545283002/diff/200001/base/files/file_util_w...
File base/files/file_util_win.cc (right):
https://codereview.chromium.org/2545283002/diff/200001/base/files/file_util_w...
base/files/file_util_win.cc:179: return Base32Encode(StringPiece(&random[0],
sizeof(random))).append(".tmp");
On 2017/01/12 15:07:03, grt (UTC plus 1) wrote:
> On 2017/01/10 18:31:56, Nico wrote:
> > what's wrong with win32's GetTempFileName()?
>
> I think it's undesirable here for a few reasons:
> - If you want it to guarantee a unique filename, the documentation says it's
> inefficient in directories containing lots of files. Temp dirs often have lots
> of files, so this seems bad.
> - If you want it to guarantee a unique filename, it creates a file with that
> name and leaves it there for you. This file is then in the way of the file we
> want to move, so we'd have to delete it or rename it, which kinda puts us back
> to where we started. :-)
(This is to protect against time of check to time of use attacks – see mktemp
references on https://en.wikipedia.org/wiki/Time_of_check_to_time_of_use . For
file deletion, I believe you're right that this gets in the way instead of
helping, but I'm also not very creative in coming up with exploits for this type
of problem :-) )
> - If you don't want it to guarantee a unique filename, you have to call it
> repeatedly in case of collision, which is no better than the roll-your-own
I've
> created here.
Well, it's roughly the same code except that we don't need the base64 code in
base, right? (Maybe I'm missing something.)
shess@chromium.org changed reviewers: - shess@chromium.org |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
