Disk cache: Change the way we delete files.

Now we always share delete and request delete access when we
open the files, and don't close the handle before deleting a
file. The result is that the delete operation should not be
prevented by someone opening the file right after we close
our handle.

The downside is that anybody attempting to open one of our
files while the browser is running will have to also share
delete or their request will fail. On the other hand, we
don't really want other people modifying the files at the
same time, so the change may be for good.

The caveat is that I think most of the failures are result
of improper AV behavior, so we have to see what happens.

BUG=16723
TEST=net_unittests


Review URL: http://codereview.chromium.org/3475025

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@60989 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 75 insertions, 16 deletions

diff --git a/net/disk_cache/backend_unittest.cc b/net/disk_cache/backend_unittest.cc
index 43b59e8..adfc95c 100644
--- a/net/disk_cache/backend_unittest.cc
+++ b/net/disk_cache/backend_unittest.cc
@@ -12,6 +12,7 @@
 #include "net/base/net_errors.h"
 #include "net/base/test_completion_callback.h"
 #include "net/disk_cache/backend_impl.h"
+#include "net/disk_cache/cache_util.h"
 #include "net/disk_cache/disk_cache_test_base.h"
 #include "net/disk_cache/disk_cache_test_util.h"
 #include "net/disk_cache/histogram_macros.h"
@@ -1889,3 +1890,44 @@ TEST_F(DiskCacheBackendTest, TotalBuffersSize2) {
 
   EXPECT_FALSE(cache_impl_->IsAllocAllowed(0, kOneMB));
 }
+
+// Tests that sharing of external files works and we are able to delete the
+// files when we need to.
+TEST_F(DiskCacheBackendTest, FileSharing) {
+  SetDirectMode();
+  InitCache();
+
+  disk_cache::Addr address(0x80000001);
+  ASSERT_TRUE(cache_impl_->CreateExternalFile(&address));
+  FilePath name = cache_impl_->GetFileName(address);
+
+  scoped_refptr<disk_cache::File> file(new disk_cache::File(false));
+  file->Init(name);
+
+#if defined(OS_WIN)
+  DWORD sharing = FILE_SHARE_READ | FILE_SHARE_WRITE;
+  DWORD access = GENERIC_READ | GENERIC_WRITE;
+  ScopedHandle file2(CreateFile(name.value().c_str(), access, sharing, NULL,
+                                OPEN_EXISTING, 0, NULL));
+  EXPECT_FALSE(file2.IsValid());
+
+  sharing |= FILE_SHARE_DELETE;
+  file2.Set(CreateFile(name.value().c_str(), access, sharing, NULL,
+                       OPEN_EXISTING, 0, NULL));
+  EXPECT_TRUE(file2.IsValid());
+#endif
+
+  EXPECT_TRUE(file_util::Delete(name, false));
+
+  // We should be able to use the file.
+  const int kSize = 200;
+  char buffer1[kSize];
+  char buffer2[kSize];
+  memset(buffer1, 't', kSize);
+  memset(buffer2, 0, kSize);
+  EXPECT_TRUE(file->Write(buffer1, kSize, 0));
+  EXPECT_TRUE(file->Read(buffer2, kSize, 0));
+  EXPECT_EQ(0, memcmp(buffer1, buffer2, kSize));
+
+  EXPECT_TRUE(disk_cache::DeleteCacheFile(name));
+}
diff --git a/net/disk_cache/block_files.cc b/net/disk_cache/block_files.cc
index 7936b8f..a0fc6b6 100644
--- a/net/disk_cache/block_files.cc
+++ b/net/disk_cache/block_files.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
+// Copyright (c) 2006-2010 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.
 
@@ -466,10 +466,15 @@ void BlockFiles::RemoveEmptyFile(FileType block_type) {
       int file_index = header->next_file;
       header->next_file = next_header->next_file;
       DCHECK(block_files_.size() >= static_cast<unsigned int>(file_index));
+
+      // We get a new handle to the file and release the old one so that the
+      // file gets unmmaped... so we can delete it.
+      FilePath name = Name(file_index);
+      scoped_refptr<File> this_file(new File(false));
+      this_file->Init(name);
       block_files_[file_index]->Release();
       block_files_[file_index] = NULL;
 
-      FilePath name = Name(file_index);
       int failure = DeleteCacheFile(name) ? 0 : 1;
       UMA_HISTOGRAM_COUNTS("DiskCache.DeleteFailed2", failure);
       if (failure)
diff --git a/net/disk_cache/cache_util_win.cc b/net/disk_cache/cache_util_win.cc
index aefce30..3e1d673 100644
--- a/net/disk_cache/cache_util_win.cc
+++ b/net/disk_cache/cache_util_win.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
+// Copyright (c) 2006-2010 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.
 
@@ -58,7 +58,21 @@ void DeleteCache(const FilePath& path, bool remove_folder) {
 bool DeleteCacheFile(const FilePath& name) {
   // We do a simple delete, without ever falling back to SHFileOperation, as the
   // version from base does.
-  return DeleteFile(name.value().c_str()) ? true : false;
+  if (!DeleteFile(name.value().c_str())) {
+    // There is an error, but we share delete access so let's see if there is a
+    // file to open. Note that this code assumes that we have a handle to the
+    // file at all times (even now), so nobody can have a handle that prevents
+    // us from opening the file again (unless it was deleted).
+    DWORD sharing = FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE;
+    DWORD access = SYNCHRONIZE;
+    ScopedHandle file(CreateFile(name.value().c_str(), access, sharing, NULL,
+                                 OPEN_EXISTING, 0, NULL));
+    if (file.IsValid())
+      return false;
+
+    // Most likely there is no file to open... and that's what we wanted.
+  }
+  return true;
 }
 
 }  // namespace disk_cache
diff --git a/net/disk_cache/entry_impl.cc b/net/disk_cache/entry_impl.cc
index c8ee9b0..0655b28 100644
--- a/net/disk_cache/entry_impl.cc
+++ b/net/disk_cache/entry_impl.cc
@@ -945,14 +945,14 @@ void EntryImpl::DeleteData(Addr address, int index) {
   if (!address.is_initialized())
     return;
   if (address.is_separate_file()) {
-    if (files_[index])
-      files_[index] = NULL;  // Releases the object.
-
-    int failure = DeleteCacheFile(backend_->GetFileName(address)) ? 0 : 1;
+    int failure = !DeleteCacheFile(backend_->GetFileName(address));
     CACHE_UMA(COUNTS, "DeleteFailed", 0, failure);
-    if (failure)
+    if (failure) {
       LOG(ERROR) << "Failed to delete " <<
           backend_->GetFileName(address).value() << " from the cache.";
+    }
+    if (files_[index])
+      files_[index] = NULL;  // Releases the object.
   } else {
     backend_->DeleteBlock(address, true);
   }
diff --git a/net/disk_cache/file_win.cc b/net/disk_cache/file_win.cc
index 1ac6c24..5b01224 100644
--- a/net/disk_cache/file_win.cc
+++ b/net/disk_cache/file_win.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2006-2008 The Chromium Authors. All rights reserved.
+// Copyright (c) 2006-2010 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.
 
@@ -72,9 +72,9 @@ bool File::Init(const FilePath& name) {
   if (init_)
     return false;
 
-  platform_file_ = CreateFile(name.value().c_str(),
-                              GENERIC_READ | GENERIC_WRITE,
-                              FILE_SHARE_READ | FILE_SHARE_WRITE, NULL,
+  DWORD sharing = FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE;
+  DWORD access = GENERIC_READ | GENERIC_WRITE | DELETE;
+  platform_file_ = CreateFile(name.value().c_str(), access, sharing, NULL,
                               OPEN_EXISTING, FILE_FLAG_OVERLAPPED, NULL);
 
   if (INVALID_HANDLE_VALUE == platform_file_)
@@ -84,9 +84,7 @@ bool File::Init(const FilePath& name) {
       platform_file_, Singleton<CompletionHandler>::get());
 
   init_ = true;
-  sync_platform_file_  = CreateFile(name.value().c_str(),
-                                    GENERIC_READ | GENERIC_WRITE,
-                                    FILE_SHARE_READ | FILE_SHARE_WRITE, NULL,
+  sync_platform_file_  = CreateFile(name.value().c_str(), access, sharing, NULL,
                                     OPEN_EXISTING, 0, NULL);
 
   if (INVALID_HANDLE_VALUE == sync_platform_file_)