diff options
author | grt@chromium.org <grt@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2012-03-13 04:47:27 +0000 |
---|---|---|
committer | grt@chromium.org <grt@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2012-03-13 04:47:27 +0000 |
commit | 8aad6d3f1d940536528fd92ffa390ed2ff39bae7 (patch) | |
tree | b90b03e3ff3517e53558c336f70b2990ff3b907d /base/file_util_win.cc | |
parent | 2b5ce54ecefe55b03c40f3d9e143a988ea994d65 (diff) | |
download | chromium_src-8aad6d3f1d940536528fd92ffa390ed2ff39bae7.zip chromium_src-8aad6d3f1d940536528fd92ffa390ed2ff39bae7.tar.gz chromium_src-8aad6d3f1d940536528fd92ffa390ed2ff39bae7.tar.bz2 |
Fix race in file_util::ReplaceFile on Windows.
The previous implementation left a window where an empty destination file had been created yet the replace hadn't yet happened. This was causing flakes in the Chrome Frame integration tests since chrome.exe is killed shortly after startup over and over again. When the stars aligned (more often than you may think), ReplaceFile would have time to create an empty Preferences file, but didn't have time to complete the ReplaceFile. Subsequent tests would then fail because an empty prefs file is an invalid prefs file.
BUG=81479
TEST=none
Review URL: http://codereview.chromium.org/9689013
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@126334 0039d316-1c4b-4281-b951-d872f2087c98
Diffstat (limited to 'base/file_util_win.cc')
-rw-r--r-- | base/file_util_win.cc | 30 |
1 files changed, 13 insertions, 17 deletions
diff --git a/base/file_util_win.cc b/base/file_util_win.cc index db21d32..7ea1aa3 100644 --- a/base/file_util_win.cc +++ b/base/file_util_win.cc @@ -177,23 +177,19 @@ bool Move(const FilePath& from_path, const FilePath& to_path) { bool ReplaceFile(const FilePath& from_path, const FilePath& to_path) { base::ThreadRestrictions::AssertIOAllowed(); - - // Make sure that the target file exists. - HANDLE target_file = ::CreateFile( - to_path.value().c_str(), - 0, - FILE_SHARE_READ | FILE_SHARE_WRITE, - NULL, - CREATE_NEW, - FILE_ATTRIBUTE_NORMAL, - NULL); - if (target_file != INVALID_HANDLE_VALUE) - ::CloseHandle(target_file); - // When writing to a network share, we may not be able to change the ACLs. - // Ignore ACL errors then (REPLACEFILE_IGNORE_MERGE_ERRORS). - return ::ReplaceFile(to_path.value().c_str(), - from_path.value().c_str(), NULL, - REPLACEFILE_IGNORE_MERGE_ERRORS, NULL, NULL) ? true : false; + // Try a simple move first. It will only succeed when |to_path| doesn't + // already exist. + if (::MoveFile(from_path.value().c_str(), to_path.value().c_str())) + return true; + // Try the full-blown replace if the move fails, as ReplaceFile will only + // succeed when |to_path| does exist. When writing to a network share, we may + // not be able to change the ACLs. Ignore ACL errors then + // (REPLACEFILE_IGNORE_MERGE_ERRORS). + if (::ReplaceFile(to_path.value().c_str(), from_path.value().c_str(), NULL, + REPLACEFILE_IGNORE_MERGE_ERRORS, NULL, NULL)) { + return true; + } + return false; } bool CopyFile(const FilePath& from_path, const FilePath& to_path) { |