Fix race in directory creation on Windows. The...

Fix race in directory creation on Windows.  The
file_util::CreateDirectory() function is supposed to succeed if the
directory already exists, but in the previous implementation when two
processes/threads called the function at the same time for the same
path, one would see a failure.

Also, get rid of the use of SHCreateDirectoryEx function.  For one, it
is highly serializing because it calls SHChangeNotify.  The race was
actually easy to reproduce by starting two instances of Chrome both at
the same time, with a profile directory flag indicating a profile
directory that did not previously exist.

For another, SHCreateDirectoryEx would fail with ERROR_CANCELLED if
any of the path components were not visible, according to its MSDN
documentation.

BUG=none
TEST=[base_unittests.exe --gtest_filter=*CreateDirectory* ], and/or install Chrome Frame on a fresh machine and make sure the first page you navigate to has two Chrome Frame instances on it - you should get no DCHECK on failure of PathService::Override


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

[M-A Ruel, 2009-11-26 20:32:10]

barely lgtm. :)

I'd rather see a CL removing the use of this function altogether.

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

http://codereview.chromium.org/437090/diff/1/2#newcode512
base/file_util_win.cc:512: // TODO(joi): Would be good to write a replacement
for SHCreateDirectoryEx.
nit: Do you mind simplify rewriting it to not use SHCreateDirectoryEx? It looks
the saner option to me and doesn't seem that complex to implement.

http://codereview.chromium.org/437090/diff/1/2#newcode528
base/file_util_win.cc:528: if (DirectoryExists(full_path)) {
return DirectoryExists(full_path);
should be fine.

[Jói, 2009-11-26 20:46:07]

Oh man, I thought you might say that :)

OK, will do, was trying not to get sidetracked in something else but
removing the function is the right thing to do.

Cheers,
Jói


On Thu, Nov 26, 2009 at 3:32 PM,  <maruel@chromium.org> wrote:
> barely lgtm. :)
>
> I'd rather see a CL removing the use of this function altogether.
>
>
> http://codereview.chromium.org/437090/diff/1/2
> File base/file_util_win.cc (right):
>
> http://codereview.chromium.org/437090/diff/1/2#newcode512
> base/file_util_win.cc:512: // TODO(joi): Would be good to write a
> replacement for SHCreateDirectoryEx.
> nit: Do you mind simplify rewriting it to not use SHCreateDirectoryEx?
> It looks the saner option to me and doesn't seem that complex to
> implement.
>
> http://codereview.chromium.org/437090/diff/1/2#newcode528
> base/file_util_win.cc:528: if (DirectoryExists(full_path)) {
> return DirectoryExists(full_path);
> should be fine.
>
> http://codereview.chromium.org/437090
>

[Jói, 2009-11-26 21:32:08]

Please take another look.  Thanks for asking me to do the right thing,
it wasn't much work.

2009/11/26 Jói Sigurðsson <joi@chromium.org>:
> Oh man, I thought you might say that :)
>
> OK, will do, was trying not to get sidetracked in something else but
> removing the function is the right thing to do.
>
> Cheers,
> Jói
>
>
> On Thu, Nov 26, 2009 at 3:32 PM,  <maruel@chromium.org> wrote:
>> barely lgtm. :)
>>
>> I'd rather see a CL removing the use of this function altogether.
>>
>>
>> http://codereview.chromium.org/437090/diff/1/2
>> File base/file_util_win.cc (right):
>>
>> http://codereview.chromium.org/437090/diff/1/2#newcode512
>> base/file_util_win.cc:512: // TODO(joi): Would be good to write a
>> replacement for SHCreateDirectoryEx.
>> nit: Do you mind simplify rewriting it to not use SHCreateDirectoryEx?
>> It looks the saner option to me and doesn't seem that complex to
>> implement.
>>
>> http://codereview.chromium.org/437090/diff/1/2#newcode528
>> base/file_util_win.cc:528: if (DirectoryExists(full_path)) {
>> return DirectoryExists(full_path);
>> should be fine.
>>
>> http://codereview.chromium.org/437090
>>
>

[M-A Ruel, 2009-11-26 21:38:45]

I knew you were able to. lgtm