Windows: Make file_util::Delete("c:\\foo_dir", false) work correctly. Add more unit tests for Delete

Windows: Make file_util::Delete("c:\\foo_dir", false) work correctly. Add more unit tests for Delete.

BUG=42374
TEST=included

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=46633

[Scott Hess - ex-Googler, 2010-04-23 05:05:10]

drive-by

http://codereview.chromium.org/1763008/diff/1/2
File base/file_util_win.cc (right):

http://codereview.chromium.org/1763008/diff/1/2#newcode83
base/file_util_win.cc:83: }
The final return in this code makes it seem like file_util::Delete() should be
considered a success if the file does not exist.  But the above doesn't support
that interpretation.  The POSIX version supports that interpretation.  It would
be nice to bring them into alignment.

[That said, POSIX has a race condition if someone else deletes the path after
the stat() and before the unlink() or rmdir().]

[darin (slow to review), 2010-04-23 06:50:40]

http://codereview.chromium.org/1763008/diff/1/2
File base/file_util_win.cc (right):

http://codereview.chromium.org/1763008/diff/1/2#newcode83
base/file_util_win.cc:83: }
On 2010/04/23 05:05:11, shess wrote:
> The final return in this code makes it seem like file_util::Delete() should be
> considered a success if the file does not exist.  But the above doesn't
support
> that interpretation.  The POSIX version supports that interpretation.  It
would
> be nice to bring them into alignment.

+1 for making ports behave similarly.  there should also be a corresponding unit
test :)


> 
> [That said, POSIX has a race condition if someone else deletes the path after
> the stat() and before the unlink() or rmdir().]

[Lei Zhang, 2010-04-23 06:53:22]

I am doing a bit of TDD right now. It looks I have some reviewers lined up
already. I'll add erikkay too.

[Lei Zhang, 2010-04-23 08:06:18]

Please take a look. It's getting a bit late, so read carefully.

[Scott Hess - ex-Googler, 2010-04-23 16:17:16]

LGTM, I think.

http://codereview.chromium.org/1763008/diff/6001/7002
File base/file_util_unittest.cc (right):

http://codereview.chromium.org/1763008/diff/6001/7002#newcode472
base/file_util_unittest.cc:472: // Create a subdirectory and put a file and two
directories insode.
s/insode/inside/

http://codereview.chromium.org/1763008/diff/6001/7002#newcode502
base/file_util_unittest.cc:502: // Create a subdirectory and put a file and two
directories insode.
s/insode/inside/

[Lei Zhang, 2010-04-23 23:07:23]

Typos fixed in patch set 3. The Windows try job for patch set 2 actually failed.
[1]

It turns out a bunch of installer unit tests (all copy and paste of each other)
did Delete(dir, false) and relied on the bad behavior I am trying to fix! So I
flipped the delte recursion flag in all those unit tests.

[1]
http://build.chromium.org/buildbot/try-server/builders/win/builds/29080/steps...

[Scott Hess - ex-Googler, 2010-04-24 15:34:15]

Still LGTM.

Even though the Windows trybot's redness all looks grd related, it would still
be nice to get that green before submitting.  Just in case...

On 2010/04/23 23:07:23, Lei Zhang wrote:
> Typos fixed in patch set 3. The Windows try job for patch set 2 actually
failed.
> [1]
> 
> It turns out a bunch of installer unit tests (all copy and paste of each
other)
> did Delete(dir, false) and relied on the bad behavior I am trying to fix! So I
> flipped the delte recursion flag in all those unit tests.
> 
> [1]
>
http://build.chromium.org/buildbot/try-server/builders/win/builds/29080/steps...

[Lei Zhang, 2010-04-28 00:29:23]

On 2010/04/24 15:34:15, shess wrote:
> Still LGTM.
> 
> Even though the Windows trybot's redness all looks grd related, it would still
> be nice to get that green before submitting.  Just in case...

Just in case DeleteNonExistent fails half the time on the bots:
http://build.chromium.org/buildbot/try-server/builders/win/builds/29310/
http://build.chromium.org/buildbot/try-server/builders/win/builds/29473

I can't reproduce this locally. I'm going to send a few more try jobs with debug
logging and hopefully figure this out.

[Scott Hess - ex-Googler, 2010-04-28 03:09:43]

OK, so I noticed something like this when I was writing some code that
did a delete, and sometimes the file didn't exist.  I never figured
out quite where it was coming from, because I looked at other
file_util::Delete() calls and they were all protected by an existence
check.  I'm going to bet that there is some subtle difference on some
trybots.

-scott


On Tue, Apr 27, 2010 at 5:29 PM,  <thestig@chromium.org> wrote:
> On 2010/04/24 15:34:15, shess wrote:
>>
>> Still LGTM.
>
>> Even though the Windows trybot's redness all looks grd related, it would
>> still
>> be nice to get that green before submitting.  Just in case...
>
> Just in case DeleteNonExistent fails half the time on the bots:
> http://build.chromium.org/buildbot/try-server/builders/win/builds/29310/
> http://build.chromium.org/buildbot/try-server/builders/win/builds/29473
>
> I can't reproduce this locally. I'm going to send a few more try jobs with
> debug
> logging and hopefully figure this out.
>
> http://codereview.chromium.org/1763008/show
>

[Lei Zhang, 2010-04-28 08:28:33]

On 2010/04/28 03:09:43, shess wrote:
> OK, so I noticed something like this when I was writing some code that
> did a delete, and sometimes the file didn't exist.  I never figured
> out quite where it was coming from, because I looked at other
> file_util::Delete() calls and they were all protected by an existence
> check.  I'm going to bet that there is some subtle difference on some
> trybots.

Took a few try jobs to hit it again. I got 1026 from ShFileOperation. MSDN says
it's basically file not found.
http://msdn.microsoft.com/en-us/library/bb762164(VS.85).aspx

[Scott Hess - ex-Googler, 2010-04-28 21:36:26]

LGTM.

I want to say to put the 0x402 as a kUnknownError constant, with the comment on
that.  But I'm having troubles making a cogent argument about how that helps the
world, given how horrible this is in the first place.

-scott

On 2010/04/28 08:28:33, Lei Zhang wrote:
> On 2010/04/28 03:09:43, shess wrote:
> > OK, so I noticed something like this when I was writing some code that
> > did a delete, and sometimes the file didn't exist.  I never figured
> > out quite where it was coming from, because I looked at other
> > file_util::Delete() calls and they were all protected by an existence
> > check.  I'm going to bet that there is some subtle difference on some
> > trybots.
> 
> Took a few try jobs to hit it again. I got 1026 from ShFileOperation. MSDN
says
> it's basically file not found.
> http://msdn.microsoft.com/en-us/library/bb762164%28VS.85%29.aspx

[Scott Hess - ex-Googler, 2010-04-28 21:42:25]

BTW: http://www.pc-library.com/errors/error-code/1026-0x402/

Makes it sound like this doesn't always mean that the file doesn't exist.  I
don't know if it's worthwhile to put one last !Exists() check in?

Earlier I was thinking of suggesting that the existence check should be pulled
out, and the code should just return immediately if !Exists.  But then I noticed
that the code would just exercise the default case, which seemed pretty
reasonable to me.  I don't feel all that strongly about this, but now that it's
getting a little more gnarly, I figure I should throw it out there.

-scott


On 2010/04/28 21:36:26, shess wrote:
> LGTM.
> 
> I want to say to put the 0x402 as a kUnknownError constant, with the comment
on
> that.  But I'm having troubles making a cogent argument about how that helps
the
> world, given how horrible this is in the first place.
> 
> -scott
> 
> On 2010/04/28 08:28:33, Lei Zhang wrote:
> > On 2010/04/28 03:09:43, shess wrote:
> > > OK, so I noticed something like this when I was writing some code that
> > > did a delete, and sometimes the file didn't exist.  I never figured
> > > out quite where it was coming from, because I looked at other
> > > file_util::Delete() calls and they were all protected by an existence
> > > check.  I'm going to bet that there is some subtle difference on some
> > > trybots.
> > 
> > Took a few try jobs to hit it again. I got 1026 from ShFileOperation. MSDN
> says
> > it's basically file not found.
> > http://msdn.microsoft.com/en-us/library/bb762164%2528VS.85%2529.aspx

[Lei Zhang, 2010-04-30 00:38:25]

Ok, patch set 5 tries something different: Use file_util::PathExists to check
for existence first. Of course, we can only do that when the path does not
contain wildcard characters. AFAIK, that's only '*' and '?' on Windows, right?

On 2010/04/28 21:42:25, shess wrote:
> BTW: http://www.pc-library.com/errors/error-code/1026-0x402/
> 
> Makes it sound like this doesn't always mean that the file doesn't exist.  I
> don't know if it's worthwhile to put one last !Exists() check in?
> 
> Earlier I was thinking of suggesting that the existence check should be pulled
> out, and the code should just return immediately if !Exists.  But then I
noticed
> that the code would just exercise the default case, which seemed pretty
> reasonable to me.  I don't feel all that strongly about this, but now that
it's
> getting a little more gnarly, I figure I should throw it out there.

[Scott Hess - ex-Googler, 2010-04-30 13:50:48]

Crap.  I like the earlier solution with the magic constant better, at
least we knew for certain that things were already failing.  My worry
was that since 0x402 seems to imply "Something bad happened", maybe it
allowed for cases where the file DID exist and wasn't actually deleted
for some reason.

Does anyone in Chrome actually use wildcards, since it's not supported on POSIX?

Replacing the last line of the function with:
  return !PathExists(path);
be reasonable?  Hmm, no, because of the wildcard problem.

OK.  I'm leaning towards magic constant, and hope for the best.  We
can probably assume that any bad cases let through by 0x402 will
anyhow cause all sorts of non-recoverable stuff to happen.  I just
wish it was clearer why we get this inconsistently.  A race condition
with the parent directory being deleted?  Trybot flakiness?

-scott


On Thu, Apr 29, 2010 at 5:38 PM,  <thestig@chromium.org> wrote:
> Ok, patch set 5 tries something different: Use file_util::PathExists to
> check
> for existence first. Of course, we can only do that when the path does not
> contain wildcard characters. AFAIK, that's only '*' and '?' on Windows,
> right?
>
> On 2010/04/28 21:42:25, shess wrote:
>>
>> BTW: http://www.pc-library.com/errors/error-code/1026-0x402/
>
>> Makes it sound like this doesn't always mean that the file doesn't exist.
>>  I
>> don't know if it's worthwhile to put one last !Exists() check in?
>
>> Earlier I was thinking of suggesting that the existence check should be
>> pulled
>> out, and the code should just return immediately if !Exists.  But then I
>
> noticed
>>
>> that the code would just exercise the default case, which seemed pretty
>> reasonable to me.  I don't feel all that strongly about this, but now that
>
> it's
>>
>> getting a little more gnarly, I figure I should throw it out there.
>
>
> http://codereview.chromium.org/1763008/show
>

[Lei Zhang, 2010-05-06 00:56:43]

On 2010/04/30 13:50:48, shess wrote:
> Crap.  I like the earlier solution with the magic constant better, at
> least we knew for certain that things were already failing.  My worry
> was that since 0x402 seems to imply "Something bad happened", maybe it
> allowed for cases where the file DID exist and wasn't actually deleted
> for some reason.
> 
> Does anyone in Chrome actually use wildcards, since it's not supported on
POSIX?
> 
> Replacing the last line of the function with:
>   return !PathExists(path);
> be reasonable?  Hmm, no, because of the wildcard problem.
> 
> OK.  I'm leaning towards magic constant, and hope for the best.  We
> can probably assume that any bad cases let through by 0x402 will
> anyhow cause all sorts of non-recoverable stuff to happen.  I just
> wish it was clearer why we get this inconsistently.  A race condition
> with the parent directory being deleted?  Trybot flakiness?

On second thought, I think patch set 4 is better in the long run. Overtime as
people move away from XP, the 0x402 will stop occurring. And yes, do we have the
win32 installer utils which pass in wildcards.  I think you've LGTMed this CL
several times already, so I will go ahead and commit patch set 4 tomorrow
morning unless I hear objections.

[Scott Hess - ex-Googler, 2010-05-06 05:01:48]

AFAICT patch 4 is "Check for an essentially undocumented error code",
in which case Still LGTM.

On Wed, May 5, 2010 at 5:56 PM,  <thestig@chromium.org> wrote:
> On 2010/04/30 13:50:48, shess wrote:
>>
>> Crap.  I like the earlier solution with the magic constant better, at
>> least we knew for certain that things were already failing.  My worry
>> was that since 0x402 seems to imply "Something bad happened", maybe it
>> allowed for cases where the file DID exist and wasn't actually deleted
>> for some reason.
>
>> Does anyone in Chrome actually use wildcards, since it's not supported on
>
> POSIX?
>
>> Replacing the last line of the function with:
>>   return !PathExists(path);
>> be reasonable?  Hmm, no, because of the wildcard problem.
>
>> OK.  I'm leaning towards magic constant, and hope for the best.  We
>> can probably assume that any bad cases let through by 0x402 will
>> anyhow cause all sorts of non-recoverable stuff to happen.  I just
>> wish it was clearer why we get this inconsistently.  A race condition
>> with the parent directory being deleted?  Trybot flakiness?
>
> On second thought, I think patch set 4 is better in the long run. Overtime
> as
> people move away from XP, the 0x402 will stop occurring. And yes, do we have
> the
> win32 installer utils which pass in wildcards.  I think you've LGTMed this
> CL
> several times already, so I will go ahead and commit patch set 4 tomorrow
> morning unless I hear objections.
>
> http://codereview.chromium.org/1763008/show
>