Make synchronous methods on disk_cache::SimpleEntryImpl() reentrant.

For passing browser tests and unit tests, it turns out that the synchronous operations on disk_cache::Entry are called quite a bit. As I was trying to get us to the point that this cache can pass browser tests, I found this was necessary.

Note: this CL is based on https://codereview.chromium.org/12192005/ (Add simple cache backend) , and must land after it.

This CL is the basis of https://codereview.chromium.org/12223075/ (Make Doom asynchronous), and so it must land before that issue.

R=rvargas@chromium.org, felipeg@chromium.org, pasko@chromium.org, willchan@chromium.org
BUG=173392

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

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

5 files changed, 51 insertions, 27 deletions

diff --git a/net/disk_cache/simple/simple_disk_format.h b/net/disk_cache/simple/simple_disk_format.h
index 052adf2..9299a42 100644
--- a/net/disk_cache/simple/simple_disk_format.h
+++ b/net/disk_cache/simple/simple_disk_format.h
@@ -16,6 +16,8 @@ const uint64 kSimpleInitialMagicNumber = GG_UINT64_C(0xfcfb6d1ba7725c30);
 
 const uint32 kSimpleVersion = 1;
 
+static const int kSimpleEntryFileCount = 3;
+
 struct SimpleFileHeader {
   uint64 initial_magic_number;
   uint32 version;
diff --git a/net/disk_cache/simple/simple_entry_impl.cc b/net/disk_cache/simple/simple_entry_impl.cc
index ba19844..6d1b7b1 100644
--- a/net/disk_cache/simple/simple_entry_impl.cc
+++ b/net/disk_cache/simple/simple_entry_impl.cc
@@ -110,27 +110,15 @@ std::string SimpleEntryImpl::GetKey() const {
 }
 
 Time SimpleEntryImpl::GetLastUsed() const {
-  if (synchronous_entry_in_use_by_worker_) {
-    NOTIMPLEMENTED();
-    CHECK(false);
-  }
-  return synchronous_entry_->last_used();
+  return last_used_;
 }
 
 Time SimpleEntryImpl::GetLastModified() const {
-  if (synchronous_entry_in_use_by_worker_) {
-    NOTIMPLEMENTED();
-    CHECK(false);
-  }
-  return synchronous_entry_->last_modified();
+  return last_modified_;
 }
 
 int32 SimpleEntryImpl::GetDataSize(int index) const {
-  if (synchronous_entry_in_use_by_worker_) {
-    NOTIMPLEMENTED();
-    CHECK(false);
-  }
-  return synchronous_entry_->data_size(index);
+  return data_size_[index];
 }
 
 int SimpleEntryImpl::ReadData(int index,
@@ -227,11 +215,13 @@ int SimpleEntryImpl::ReadyForSparseIO(const CompletionCallback& callback) {
 SimpleEntryImpl::SimpleEntryImpl(
     SimpleSynchronousEntry* synchronous_entry)
     : ALLOW_THIS_IN_INITIALIZER_LIST(weak_ptr_factory_(this)),
+      path_(synchronous_entry->path()),
       key_(synchronous_entry->key()),
       synchronous_entry_(synchronous_entry),
       synchronous_entry_in_use_by_worker_(false),
       has_been_doomed_(false) {
   DCHECK(synchronous_entry);
+  SetSynchronousData();
 }
 
 SimpleEntryImpl::~SimpleEntryImpl() {
@@ -259,8 +249,23 @@ void SimpleEntryImpl::EntryOperationComplete(
   if (entry) {
     DCHECK(entry->synchronous_entry_in_use_by_worker_);
     entry->synchronous_entry_in_use_by_worker_ = false;
+    entry->SetSynchronousData();
   }
   completion_callback.Run(result);
 }
 
+void SimpleEntryImpl::SetSynchronousData() {
+  DCHECK(!synchronous_entry_in_use_by_worker_);
+
+  // TODO(felipeg): These copies to avoid data races are not optimal. While
+  // adding an IO thread index (for fast misses etc...), we can store this data
+  // in that structure. This also solves problems with last_used() on ext4
+  // filesystems not being accurate.
+
+  last_used_ = synchronous_entry_->last_used();
+  last_modified_ = synchronous_entry_->last_modified();
+  for (int i = 0; i < kSimpleEntryFileCount; ++i)
+    data_size_[i] = synchronous_entry_->data_size(i);
+}
+
 }  // namespace disk_cache
diff --git a/net/disk_cache/simple/simple_entry_impl.h b/net/disk_cache/simple/simple_entry_impl.h
index 26f9d4d..ca3c14d 100644
--- a/net/disk_cache/simple/simple_entry_impl.h
+++ b/net/disk_cache/simple/simple_entry_impl.h
@@ -7,8 +7,10 @@
 
 #include <string>
 
+#include "base/files/file_path.h"
 #include "base/memory/weak_ptr.h"
 #include "net/disk_cache/disk_cache.h"
+#include "net/disk_cache/simple/simple_disk_format.h"
 
 namespace net {
 class IOBuffer;
@@ -92,8 +94,24 @@ class SimpleEntryImpl : public Entry {
       base::WeakPtr<SimpleEntryImpl> entry,
       int result);
 
+  // Called on construction and also after the completion of asynchronous IO to
+  // initialize the IO thread copies of data returned by synchronous accessor
+  // functions. Copies data from |synchronous_entry_| into |this|, so that
+  // values can be returned during our next IO operation.
+  void SetSynchronousData();
+
   base::WeakPtrFactory<SimpleEntryImpl> weak_ptr_factory_;
-  std::string key_;
+
+  // |path_| and |key_| are copied from the synchronous entry on construction,
+  // and never updated as they are const.
+  const base::FilePath path_;
+  const std::string key_;
+
+  // |last_used_|, |last_modified_| and |data_size_| are copied from the
+  // synchronous entry at the completion of each item of asynchronous IO.
+  base::Time last_used_;
+  base::Time last_modified_;
+  int32 data_size_[kSimpleEntryFileCount];
 
   // The |synchronous_entry_| is the worker thread object that performs IO on
   // entries.
diff --git a/net/disk_cache/simple/simple_synchronous_entry.cc b/net/disk_cache/simple/simple_synchronous_entry.cc
index 6a8b7f5..d8e5148 100644
--- a/net/disk_cache/simple/simple_synchronous_entry.cc
+++ b/net/disk_cache/simple/simple_synchronous_entry.cc
@@ -20,7 +20,6 @@
 #include "base/task_runner.h"
 #include "net/base/io_buffer.h"
 #include "net/base/net_errors.h"
-#include "net/disk_cache/simple/simple_disk_format.h"
 
 using base::ClosePlatformFile;
 using base::FilePath;
@@ -104,7 +103,7 @@ void SimpleSynchronousEntry::DoomEntry(
     const std::string& key,
     scoped_refptr<TaskRunner> callback_runner,
     const net::CompletionCallback& callback) {
-  for (int i = 0; i < kIndexCount; ++i) {
+  for (int i = 0; i < kSimpleEntryFileCount; ++i) {
     FilePath to_delete = path.AppendASCII(GetFilenameForKeyAndIndex(key, i));
     bool ALLOW_UNUSED result = file_util::Delete(to_delete, false);
     DLOG_IF(ERROR, !result) << "Could not delete " << to_delete.MaybeAsASCII();
@@ -125,7 +124,7 @@ void SimpleSynchronousEntry::DoomAndClose() {
 }
 
 void SimpleSynchronousEntry::Close() {
-  for (int i = 0; i < kIndexCount; ++i) {
+  for (int i = 0; i < kSimpleEntryFileCount; ++i) {
     bool ALLOW_UNUSED result = ClosePlatformFile(files_[i]);
     DLOG_IF(INFO, !result) << "Could not Close() file.";
   }
@@ -194,7 +193,7 @@ SimpleSynchronousEntry::~SimpleSynchronousEntry() {
 }
 
 bool SimpleSynchronousEntry::OpenOrCreateFiles(bool create) {
-  for (int i = 0; i < kIndexCount; ++i) {
+  for (int i = 0; i < kSimpleEntryFileCount; ++i) {
     FilePath filename = path_.AppendASCII(GetFilenameForKeyAndIndex(key_, i));
     int flags = PLATFORM_FILE_READ | PLATFORM_FILE_WRITE;
     if (create)
@@ -216,7 +215,7 @@ bool SimpleSynchronousEntry::OpenOrCreateFiles(bool create) {
     }
   }
 
-  for (int i = 0; i < kIndexCount; ++i) {
+  for (int i = 0; i < kSimpleEntryFileCount; ++i) {
     PlatformFileInfo file_info;
     bool success = GetPlatformFileInfo(files_[i], &file_info);
     if (!success) {
@@ -236,7 +235,7 @@ bool SimpleSynchronousEntry::InitializeForOpen() {
   if (!OpenOrCreateFiles(false))
     return false;
 
-  for (int i = 0; i < kIndexCount; ++i) {
+  for (int i = 0; i < kSimpleEntryFileCount; ++i) {
     SimpleFileHeader header;
     int header_read_result =
         ReadPlatformFile(files_[i], 0, reinterpret_cast<char*>(&header),
@@ -292,7 +291,7 @@ bool SimpleSynchronousEntry::InitializeForCreate() {
     return false;
   }
 
-  for (int i = 0; i < kIndexCount; ++i) {
+  for (int i = 0; i < kSimpleEntryFileCount; ++i) {
     SimpleFileHeader header;
     header.initial_magic_number = kSimpleInitialMagicNumber;
     header.version = kSimpleVersion;
diff --git a/net/disk_cache/simple/simple_synchronous_entry.h b/net/disk_cache/simple/simple_synchronous_entry.h
index 1bf6a54..0c3b0bd 100644
--- a/net/disk_cache/simple/simple_synchronous_entry.h
+++ b/net/disk_cache/simple/simple_synchronous_entry.h
@@ -14,6 +14,7 @@
 #include "base/task_runner.h"
 #include "base/time.h"
 #include "net/base/completion_callback.h"
+#include "net/disk_cache/simple/simple_disk_format.h"
 
 namespace base {
 class SingleThreadTaskRunner;
@@ -72,14 +73,13 @@ class SimpleSynchronousEntry {
                  const SynchronousOperationCallback& callback,
                  bool truncate);
 
+  const base::FilePath& path() const { return path_; }
   std::string key() const { return key_; }
   base::Time last_used() const { return last_used_; }
   base::Time last_modified() const { return last_modified_; }
   int32 data_size(int index) const { return data_size_[index]; }
 
  private:
-  static const int kIndexCount = 3;
-
   SimpleSynchronousEntry(
       const scoped_refptr<base::TaskRunner>& callback_runner,
       const base::FilePath& path,
@@ -101,9 +101,9 @@ class SimpleSynchronousEntry {
 
   base::Time last_used_;
   base::Time last_modified_;
-  int32 data_size_[kIndexCount];
+  int32 data_size_[kSimpleEntryFileCount];
 
-  base::PlatformFile files_[kIndexCount];
+  base::PlatformFile files_[kSimpleEntryFileCount];
 };
 
 }  // namespace disk_cache