Deflake Simple Cache backend on Windows.

By taking a bit of care around opening files and deleting them, POSIX-like semantics are possible, and Simple Cache tests pass.

R=gavinp@chromium.org
BUG=None

Review URL: https://codereview.chromium.org/651063002

Cr-Commit-Position: refs/heads/master@{#301784}

8 files changed, 103 insertions, 30 deletions

diff --git a/net/disk_cache/backend_unittest.cc b/net/disk_cache/backend_unittest.cc
index e06efd9..bb313ed 100644
--- a/net/disk_cache/backend_unittest.cc
+++ b/net/disk_cache/backend_unittest.cc
@@ -3144,11 +3144,6 @@ TEST_F(DiskCacheBackendTest, ShaderCacheUpdateRankForExternalCacheHit) {
   entry->Close();
 }
 
-// The Simple Cache backend requires a few guarantees from the filesystem like
-// atomic renaming of recently open files. Those guarantees are not provided in
-// general on Windows.
-#if defined(OS_POSIX)
-
 TEST_F(DiskCacheBackendTest, SimpleCacheShutdownWithPendingCreate) {
   SetCacheType(net::APP_CACHE);
   SetSimpleCacheMode();
@@ -3486,5 +3481,3 @@ TEST_F(DiskCacheBackendTest, SimpleCacheEnumerationDestruction) {
   cache_.reset();
   // This test passes if we don't leak memory.
 }
-
-#endif  // defined(OS_POSIX)
diff --git a/net/disk_cache/entry_unittest.cc b/net/disk_cache/entry_unittest.cc
index 6081137..826b44f 100644
--- a/net/disk_cache/entry_unittest.cc
+++ b/net/disk_cache/entry_unittest.cc
@@ -2331,11 +2331,6 @@ TEST_F(DiskCacheEntryTest, KeySanityCheck) {
   DisableIntegrityCheck();
 }
 
-// The Simple Cache backend requires a few guarantees from the filesystem like
-// atomic renaming of recently open files. Those guarantees are not provided in
-// general on Windows.
-#if defined(OS_POSIX)
-
 TEST_F(DiskCacheEntryTest, SimpleCacheInternalAsyncIO) {
   SetSimpleCacheMode();
   InitCache();
@@ -4063,5 +4058,3 @@ TEST_F(DiskCacheEntryTest, SimpleCacheTruncateLargeSparseFile) {
 
   entry->Close();
 }
-
-#endif  // defined(OS_POSIX)
diff --git a/net/disk_cache/simple/simple_index_file.cc b/net/disk_cache/simple/simple_index_file.cc
index d819c9f..c6a29a2 100644
--- a/net/disk_cache/simple/simple_index_file.cc
+++ b/net/disk_cache/simple/simple_index_file.cc
@@ -6,6 +6,7 @@
 
 #include <vector>
 
+#include "base/files/file.h"
 #include "base/files/file_util.h"
 #include "base/files/memory_mapped_file.h"
 #include "base/hash.h"
@@ -22,6 +23,8 @@
 #include "net/disk_cache/simple/simple_util.h"
 #include "third_party/zlib/zlib.h"
 
+using base::File;
+
 namespace disk_cache {
 namespace {
 
@@ -66,10 +69,16 @@ void UmaRecordIndexInitMethod(IndexInitMethod method,
 }
 
 bool WritePickleFile(Pickle* pickle, const base::FilePath& file_name) {
-  int bytes_written = base::WriteFile(
-      file_name, static_cast<const char*>(pickle->data()), pickle->size());
+  File file(
+      file_name,
+      File::FLAG_CREATE_ALWAYS | File::FLAG_WRITE | File::FLAG_SHARE_DELETE);
+  if (!file.IsValid())
+    return false;
+
+  int bytes_written =
+      file.Write(0, static_cast<const char*>(pickle->data()), pickle->size());
   if (bytes_written != implicit_cast<int>(pickle->size())) {
-    base::DeleteFile(file_name, /* recursive = */ false);
+    simple_util::SimpleCacheDeleteFile(file_name);
     return false;
   }
   return true;
@@ -95,7 +104,7 @@ void ProcessEntryFile(SimpleIndex::EntrySet* entries,
     return;
   }
 
-  base::File::Info file_info;
+  File::Info file_info;
   if (!base::GetFileInfo(file_path, &file_info)) {
     LOG(ERROR) << "Could not get file info for " << file_path.value();
     return;
@@ -332,10 +341,14 @@ void SimpleIndexFile::SyncLoadFromDisk(const base::FilePath& index_filename,
                                        SimpleIndexLoadResult* out_result) {
   out_result->Reset();
 
+  File file(index_filename,
+            File::FLAG_OPEN | File::FLAG_READ | File::FLAG_SHARE_DELETE);
+  if (!file.IsValid())
+    return;
+
   base::MemoryMappedFile index_file_map;
-  if (!index_file_map.Initialize(index_filename)) {
-    LOG(WARNING) << "Could not map Simple Index file.";
-    base::DeleteFile(index_filename, false);
+  if (!index_file_map.Initialize(file.Pass())) {
+    simple_util::SimpleCacheDeleteFile(index_filename);
     return;
   }
 
@@ -346,7 +359,7 @@ void SimpleIndexFile::SyncLoadFromDisk(const base::FilePath& index_filename,
       out_result);
 
   if (!out_result->did_load)
-    base::DeleteFile(index_filename, false);
+    simple_util::SimpleCacheDeleteFile(index_filename);
 }
 
 // static
@@ -434,7 +447,7 @@ void SimpleIndexFile::SyncRestoreFromDisk(
     const base::FilePath& index_file_path,
     SimpleIndexLoadResult* out_result) {
   VLOG(1) << "Simple Cache Index is being restored from disk.";
-  base::DeleteFile(index_file_path, /* recursive = */ false);
+  simple_util::SimpleCacheDeleteFile(index_file_path);
   out_result->Reset();
   SimpleIndex::EntrySet* entries = &out_result->entries;
 
diff --git a/net/disk_cache/simple/simple_synchronous_entry.cc b/net/disk_cache/simple/simple_synchronous_entry.cc
index c50c9a3..52f8f08 100644
--- a/net/disk_cache/simple/simple_synchronous_entry.cc
+++ b/net/disk_cache/simple/simple_synchronous_entry.cc
@@ -719,7 +719,8 @@ bool SimpleSynchronousEntry::MaybeOpenFile(
   DCHECK(out_error);
 
   FilePath filename = GetFilenameFromFileIndex(file_index);
-  int flags = File::FLAG_OPEN | File::FLAG_READ | File::FLAG_WRITE;
+  int flags = File::FLAG_OPEN | File::FLAG_READ | File::FLAG_WRITE |
+              File::FLAG_SHARE_DELETE;
   files_[file_index].Initialize(filename, flags);
   *out_error = files_[file_index].error_details();
 
@@ -744,7 +745,8 @@ bool SimpleSynchronousEntry::MaybeCreateFile(
   }
 
   FilePath filename = GetFilenameFromFileIndex(file_index);
-  int flags = File::FLAG_CREATE | File::FLAG_READ | File::FLAG_WRITE;
+  int flags = File::FLAG_CREATE | File::FLAG_READ | File::FLAG_WRITE |
+              File::FLAG_SHARE_DELETE;
   files_[file_index].Initialize(filename, flags);
   *out_error = files_[file_index].error_details();
 
@@ -1136,7 +1138,7 @@ bool SimpleSynchronousEntry::DeleteFileForEntryHash(
     const int file_index) {
   FilePath to_delete = path.AppendASCII(
       GetFilenameFromEntryHashAndFileIndex(entry_hash, file_index));
-  return base::DeleteFile(to_delete, false);
+  return simple_util::SimpleCacheDeleteFile(to_delete);
 }
 
 // static
@@ -1150,7 +1152,7 @@ bool SimpleSynchronousEntry::DeleteFilesForEntryHash(
   }
   FilePath to_delete = path.AppendASCII(
       GetSparseFilenameFromEntryHash(entry_hash));
-  base::DeleteFile(to_delete, false);
+  simple_util::SimpleCacheDeleteFile(to_delete);
   return result;
 }
 
@@ -1181,7 +1183,8 @@ bool SimpleSynchronousEntry::OpenSparseFileIfExists(
 
   FilePath filename = path_.AppendASCII(
       GetSparseFilenameFromEntryHash(entry_hash_));
-  int flags = File::FLAG_OPEN | File::FLAG_READ | File::FLAG_WRITE;
+  int flags = File::FLAG_OPEN | File::FLAG_READ | File::FLAG_WRITE |
+              File::FLAG_SHARE_DELETE;
   sparse_file_.Initialize(filename, flags);
   if (sparse_file_.IsValid())
     return ScanSparseFile(out_sparse_data_size);
@@ -1194,7 +1197,8 @@ bool SimpleSynchronousEntry::CreateSparseFile() {
 
   FilePath filename = path_.AppendASCII(
       GetSparseFilenameFromEntryHash(entry_hash_));
-  int flags = File::FLAG_CREATE | File::FLAG_READ | File::FLAG_WRITE;
+  int flags = File::FLAG_CREATE | File::FLAG_READ | File::FLAG_WRITE |
+              File::FLAG_SHARE_DELETE;
   sparse_file_.Initialize(filename, flags);
   if (!sparse_file_.IsValid())
     return false;
diff --git a/net/disk_cache/simple/simple_util.h b/net/disk_cache/simple/simple_util.h
index b6ca85b..24bfa0b 100644
--- a/net/disk_cache/simple/simple_util.h
+++ b/net/disk_cache/simple/simple_util.h
@@ -69,7 +69,14 @@ NET_EXPORT_PRIVATE int GetFileIndexFromStreamIndex(int stream_index);
 // functions in file.h, the time resolution is milliseconds.
 NET_EXPORT_PRIVATE bool GetMTime(const base::FilePath& path,
                                  base::Time* out_mtime);
-}  // namespace simple_backend
+
+// Deletes a file, insuring POSIX semantics. Provided that all open handles to
+// this file were opened with File::FLAG_SHARE_DELETE, it is possible to delete
+// an open file and continue to use that file. After deleting an open file, it
+// is possible to immediately create a new file with the same name.
+NET_EXPORT_PRIVATE bool SimpleCacheDeleteFile(const base::FilePath& path);
+
+}  // namespace simple_util
 
 }  // namespace disk_cache
 
diff --git a/net/disk_cache/simple/simple_util_posix.cc b/net/disk_cache/simple/simple_util_posix.cc
new file mode 100644
index 0000000..e8bb0a3
--- /dev/null
+++ b/net/disk_cache/simple/simple_util_posix.cc
@@ -0,0 +1,17 @@
+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "net/disk_cache/simple/simple_util.h"
+
+#include "base/files/file_util.h"
+
+namespace disk_cache {
+namespace simple_util {
+
+bool SimpleCacheDeleteFile(const base::FilePath& path) {
+  return base::DeleteFile(path, false);
+}
+
+}  // namespace simple_util
+}  // namespace disk_cache
diff --git a/net/disk_cache/simple/simple_util_win.cc b/net/disk_cache/simple/simple_util_win.cc
new file mode 100644
index 0000000..d0d191d
--- /dev/null
+++ b/net/disk_cache/simple/simple_util_win.cc
@@ -0,0 +1,44 @@
+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "net/disk_cache/simple/simple_util.h"
+
+#include <windows.h>
+
+#include "base/files/file_util.h"
+#include "base/format_macros.h"
+#include "base/rand_util.h"
+#include "base/strings/stringprintf.h"
+#include "net/disk_cache/cache_util.h"
+
+namespace disk_cache {
+namespace simple_util {
+
+bool SimpleCacheDeleteFile(const base::FilePath& path) {
+  // Even if a file was opened with FLAG_SHARE_DELETE, it is not possible to
+  // create a new file with the same name until the original file is actually
+  // deleted. To allow new files to be created with the new name right away,
+  // the file is renamed before it is deleted.
+
+  // Why a random name? Because if the name was derived from our original name,
+  // then churn on a particular cache entry could cause flakey behaviour.
+
+  // TODO(gaivnp): Ensure these "todelete_" files are cleaned up on periodic
+  // directory sweeps.
+  const base::FilePath rename_target =
+      path.DirName().AppendASCII(base::StringPrintf("todelete_%016" PRIx64,
+                                                    base::RandUint64()));
+
+  bool rename_succeeded = !!MoveFile(path.value().c_str(),
+                                     rename_target.value().c_str());
+  if (rename_succeeded)
+    return DeleteCacheFile(rename_target);
+
+  // The rename did not succeed. The fallback behaviour is to delete the file in
+  // place, which might cause some flake.
+  return DeleteCacheFile(path);
+}
+
+}  // namespace simple_util
+}  // namespace disk_cache
diff --git a/net/net.gypi b/net/net.gypi
index 01b0d56..b19ce05 100644
--- a/net/net.gypi
+++ b/net/net.gypi
@@ -481,6 +481,8 @@
       'disk_cache/simple/simple_synchronous_entry.cc',
       'disk_cache/simple/simple_synchronous_entry.h',
       'disk_cache/simple/simple_util.cc',
+      'disk_cache/simple/simple_util_posix.cc',
+      'disk_cache/simple/simple_util_win.cc',
       'disk_cache/simple/simple_util.h',
       'disk_cache/simple/simple_version_upgrade.cc',
       'disk_cache/simple/simple_version_upgrade.h',