Reland r211790: "Make simple cache not use optimistic operations f..."

This is the second reland. The previous attempt failed due to the
fact that the SimpleCacheAppCacheEnumerations2 test was failing on the Linux
bot (although the CL passed the CQ and the tests were run in a loop on a Linux
developer workstation).

This CL is needed as part of the default disk backend -> simple backend
migration for app cache (on Android).

This CL also enables a few unit tests in app cache mode with the simple
backend.

TBR=gavinp@chromium.org
BUG=257616

Review URL: https://chromiumcodereview.appspot.com/20033002

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

6 files changed, 204 insertions, 56 deletions

diff --git a/net/disk_cache/backend_unittest.cc b/net/disk_cache/backend_unittest.cc
index 509f59c..1458134 100644
--- a/net/disk_cache/backend_unittest.cc
+++ b/net/disk_cache/backend_unittest.cc
@@ -3046,11 +3046,22 @@ TEST_F(DiskCacheBackendTest, SimpleCacheBasics) {
   BackendBasics();
 }
 
+TEST_F(DiskCacheBackendTest, SimpleCacheAppCacheBasics) {
+  SetCacheType(net::APP_CACHE);
+  SetSimpleCacheMode();
+  BackendBasics();
+}
+
 TEST_F(DiskCacheBackendTest, SimpleCacheKeying) {
   SetSimpleCacheMode();
   BackendKeying();
 }
 
+TEST_F(DiskCacheBackendTest, SimpleCacheAppCacheKeying) {
+  SetSimpleCacheMode();
+  SetCacheType(net::APP_CACHE);
+  BackendKeying();
+}
 
 TEST_F(DiskCacheBackendTest, DISABLED_SimpleCacheSetSize) {
   SetSimpleCacheMode();
@@ -3060,16 +3071,30 @@ TEST_F(DiskCacheBackendTest, DISABLED_SimpleCacheSetSize) {
 // MacOS has a default open file limit of 256 files, which is incompatible with
 // this simple cache test.
 #if defined(OS_MACOSX)
-#define MAYBE_SimpleCacheLoad DISABLED_SimpleCacheLoad
+#define SIMPLE_MAYBE_MACOS(TestName) DISABLED_ ## TestName
 #else
-#define MAYBE_SimpleCacheLoad SimpleCacheLoad
+#define SIMPLE_MAYBE_MACOS(TestName) TestName
 #endif
-TEST_F(DiskCacheBackendTest, MAYBE_SimpleCacheLoad) {
+
+TEST_F(DiskCacheBackendTest, SIMPLE_MAYBE_MACOS(SimpleCacheLoad)) {
   SetMaxSize(0x100000);
   SetSimpleCacheMode();
   BackendLoad();
 }
 
+TEST_F(DiskCacheBackendTest, SIMPLE_MAYBE_MACOS(SimpleCacheAppCacheLoad)) {
+  SetCacheType(net::APP_CACHE);
+  SetSimpleCacheMode();
+  SetMaxSize(0x100000);
+  BackendLoad();
+}
+
+TEST_F(DiskCacheBackendTest, SimpleCacheAppCacheChain) {
+  SetCacheType(net::APP_CACHE);
+  SetSimpleCacheMode();
+  BackendChain();
+}
+
 TEST_F(DiskCacheBackendTest, SimpleDoomRecent) {
   SetSimpleCacheMode();
   BackendDoomRecent();
@@ -3086,6 +3111,12 @@ TEST_F(DiskCacheBackendTest, FLAKY_SimpleCacheDoomAll) {
   BackendDoomAll();
 }
 
+TEST_F(DiskCacheBackendTest, SimpleCacheAppCacheOnlyDoomAll) {
+  SetCacheType(net::APP_CACHE);
+  SetSimpleCacheMode();
+  BackendDoomAll();
+}
+
 TEST_F(DiskCacheBackendTest, SimpleCacheTracingBackendBasics) {
   SetSimpleCacheMode();
   TracingBackendBasics();
diff --git a/net/disk_cache/entry_unittest.cc b/net/disk_cache/entry_unittest.cc
index 9492a9e..b49d308 100644
--- a/net/disk_cache/entry_unittest.cc
+++ b/net/disk_cache/entry_unittest.cc
@@ -2541,6 +2541,121 @@ TEST_F(DiskCacheEntryTest, SimpleCacheNoEOF) {
   DisableIntegrityCheck();
 }
 
+TEST_F(DiskCacheEntryTest, SimpleCacheNonOptimisticOperationsBasic) {
+  // Test sequence:
+  // Create, Write, Read, Close.
+  SetCacheType(net::APP_CACHE);  // APP_CACHE doesn't use optimistic operations.
+  SetSimpleCacheMode();
+  InitCache();
+  disk_cache::Entry* const null_entry = NULL;
+
+  disk_cache::Entry* entry = NULL;
+  EXPECT_EQ(net::OK, CreateEntry("my key", &entry));
+  ASSERT_NE(null_entry, entry);
+
+  const int kBufferSize = 10;
+  scoped_refptr<net::IOBufferWithSize> write_buffer(
+      new net::IOBufferWithSize(kBufferSize));
+  CacheTestFillBuffer(write_buffer->data(), write_buffer->size(), false);
+  EXPECT_EQ(
+      write_buffer->size(),
+      WriteData(entry, 0, 0, write_buffer.get(), write_buffer->size(), false));
+
+  scoped_refptr<net::IOBufferWithSize> read_buffer(
+      new net::IOBufferWithSize(kBufferSize));
+  EXPECT_EQ(
+      read_buffer->size(),
+      ReadData(entry, 0, 0, read_buffer.get(), read_buffer->size()));
+  entry->Close();
+}
+
+TEST_F(DiskCacheEntryTest, SimpleCacheNonOptimisticOperationsDontBlock) {
+  // Test sequence:
+  // Create, Write, Close.
+  SetCacheType(net::APP_CACHE);  // APP_CACHE doesn't use optimistic operations.
+  SetSimpleCacheMode();
+  InitCache();
+  disk_cache::Entry* const null_entry = NULL;
+
+  MessageLoopHelper helper;
+  CallbackTest create_callback(&helper, false);
+
+  int expected_callback_runs = 0;
+  const int kBufferSize = 10;
+  scoped_refptr<net::IOBufferWithSize> write_buffer(
+      new net::IOBufferWithSize(kBufferSize));
+
+  disk_cache::Entry* entry = NULL;
+  EXPECT_EQ(net::OK, CreateEntry("my key", &entry));
+  ASSERT_NE(null_entry, entry);
+
+  CacheTestFillBuffer(write_buffer->data(), write_buffer->size(), false);
+  CallbackTest write_callback(&helper, false);
+  int ret = entry->WriteData(
+      0,
+      0,
+      write_buffer.get(),
+      write_buffer->size(),
+      base::Bind(&CallbackTest::Run, base::Unretained(&write_callback)),
+      false);
+  ASSERT_EQ(net::ERR_IO_PENDING, ret);
+  helper.WaitUntilCacheIoFinished(++expected_callback_runs);
+
+  entry->Close();
+}
+
+TEST_F(DiskCacheEntryTest,
+       SimpleCacheNonOptimisticOperationsBasicsWithoutWaiting) {
+  // Test sequence:
+  // Create, Write, Read, Close.
+  SetCacheType(net::APP_CACHE);  // APP_CACHE doesn't use optimistic operations.
+  SetSimpleCacheMode();
+  InitCache();
+  disk_cache::Entry* const null_entry = NULL;
+  MessageLoopHelper helper;
+
+  disk_cache::Entry* entry = NULL;
+  // Note that |entry| is only set once CreateEntry() completed which is why we
+  // have to wait (i.e. use the helper CreateEntry() function).
+  EXPECT_EQ(net::OK, CreateEntry("my key", &entry));
+  ASSERT_NE(null_entry, entry);
+
+  const int kBufferSize = 10;
+  scoped_refptr<net::IOBufferWithSize> write_buffer(
+      new net::IOBufferWithSize(kBufferSize));
+  CacheTestFillBuffer(write_buffer->data(), write_buffer->size(), false);
+  CallbackTest write_callback(&helper, false);
+  int ret = entry->WriteData(
+      0,
+      0,
+      write_buffer.get(),
+      write_buffer->size(),
+      base::Bind(&CallbackTest::Run, base::Unretained(&write_callback)),
+      false);
+  EXPECT_EQ(net::ERR_IO_PENDING, ret);
+  int expected_callback_runs = 1;
+
+  scoped_refptr<net::IOBufferWithSize> read_buffer(
+      new net::IOBufferWithSize(kBufferSize));
+  CallbackTest read_callback(&helper, false);
+  ret = entry->ReadData(
+      0,
+      0,
+      read_buffer.get(),
+      read_buffer->size(),
+      base::Bind(&CallbackTest::Run, base::Unretained(&read_callback)));
+  EXPECT_EQ(net::ERR_IO_PENDING, ret);
+  ++expected_callback_runs;
+
+  helper.WaitUntilCacheIoFinished(expected_callback_runs);
+  ASSERT_EQ(read_buffer->size(), write_buffer->size());
+  EXPECT_EQ(
+      0,
+      memcmp(read_buffer->data(), write_buffer->data(), read_buffer->size()));
+
+  entry->Close();
+}
+
 TEST_F(DiskCacheEntryTest, SimpleCacheOptimistic) {
   // Test sequence:
   // Create, Write, Read, Write, Read, Close.
diff --git a/net/disk_cache/simple/simple_backend_impl.cc b/net/disk_cache/simple/simple_backend_impl.cc
index 16e5ebe..4037e8e 100644
--- a/net/disk_cache/simple/simple_backend_impl.cc
+++ b/net/disk_cache/simple/simple_backend_impl.cc
@@ -177,6 +177,10 @@ SimpleBackendImpl::SimpleBackendImpl(const FilePath& path,
     : path_(path),
       cache_thread_(cache_thread),
       orig_max_size_(max_bytes),
+      entry_operations_mode_(
+          type == net::DISK_CACHE ?
+              SimpleEntryImpl::OPTIMISTIC_OPERATIONS :
+              SimpleEntryImpl::NON_OPTIMISTIC_OPERATIONS),
       net_log_(net_log) {
 }
 
@@ -392,8 +396,8 @@ scoped_refptr<SimpleEntryImpl> SimpleBackendImpl::CreateOrFindActiveEntry(
   if (insert_result.second)
     DCHECK(!it->second.get());
   if (!it->second.get()) {
-    SimpleEntryImpl* entry =
-        new SimpleEntryImpl(this, path_, entry_hash, net_log_);
+    SimpleEntryImpl* entry = new SimpleEntryImpl(
+        path_, entry_hash, entry_operations_mode_, this, net_log_);
     entry->set_key(key);
     it->second = entry->AsWeakPtr();
   }
@@ -408,7 +412,6 @@ scoped_refptr<SimpleEntryImpl> SimpleBackendImpl::CreateOrFindActiveEntry(
   return make_scoped_refptr(it->second.get());
 }
 
-
 int SimpleBackendImpl::OpenEntryFromHash(uint64 hash,
                                          Entry** entry,
                                          const CompletionCallback& callback) {
@@ -417,7 +420,7 @@ int SimpleBackendImpl::OpenEntryFromHash(uint64 hash,
     return OpenEntry(has_active->second->key(), entry, callback);
 
   scoped_refptr<SimpleEntryImpl> simple_entry =
-      new SimpleEntryImpl(this, path_, hash, net_log_);
+      new SimpleEntryImpl(path_, hash, entry_operations_mode_, this, net_log_);
   CompletionCallback backend_callback =
       base::Bind(&SimpleBackendImpl::OnEntryOpenedFromHash,
                  AsWeakPtr(),
diff --git a/net/disk_cache/simple/simple_backend_impl.h b/net/disk_cache/simple/simple_backend_impl.h
index e2fc8bc..a21c3c7 100644
--- a/net/disk_cache/simple/simple_backend_impl.h
+++ b/net/disk_cache/simple/simple_backend_impl.h
@@ -18,6 +18,7 @@
 #include "base/task_runner.h"
 #include "net/base/cache_type.h"
 #include "net/disk_cache/disk_cache.h"
+#include "net/disk_cache/simple/simple_entry_impl.h"
 
 namespace base {
 class SingleThreadTaskRunner;
@@ -165,12 +166,13 @@ class NET_EXPORT_PRIVATE SimpleBackendImpl : public Backend,
   scoped_refptr<base::TaskRunner> worker_pool_;
 
   int orig_max_size_;
+  const SimpleEntryImpl::OperationsMode entry_operations_mode_;
 
   // TODO(gavinp): Store the entry_hash in SimpleEntryImpl, and index this map
   // by hash. This will save memory, and make IndexReadyForDoom easier.
   EntryMap active_entries_;
 
-  net::NetLog* net_log_;
+  net::NetLog* const net_log_;
 };
 
 }  // namespace disk_cache
diff --git a/net/disk_cache/simple/simple_entry_impl.cc b/net/disk_cache/simple/simple_entry_impl.cc
index d2620d2..62f27f0 100644
--- a/net/disk_cache/simple/simple_entry_impl.cc
+++ b/net/disk_cache/simple/simple_entry_impl.cc
@@ -94,14 +94,16 @@ class SimpleEntryImpl::ScopedOperationRunner {
   SimpleEntryImpl* const entry_;
 };
 
-SimpleEntryImpl::SimpleEntryImpl(SimpleBackendImpl* backend,
-                                 const FilePath& path,
+SimpleEntryImpl::SimpleEntryImpl(const FilePath& path,
                                  const uint64 entry_hash,
+                                 OperationsMode operations_mode,
+                                 SimpleBackendImpl* backend,
                                  net::NetLog* net_log)
     : backend_(backend->AsWeakPtr()),
       worker_pool_(backend->worker_pool()),
       path_(path),
       entry_hash_(entry_hash),
+      use_optimistic_operations_(operations_mode == OPTIMISTIC_OPERATIONS),
       last_used_(Time::Now()),
       last_modified_(last_used_),
       open_count_(0),
@@ -161,10 +163,9 @@ int SimpleEntryImpl::CreateEntry(Entry** out_entry,
   DCHECK(backend_.get());
   DCHECK_EQ(entry_hash_, simple_util::GetEntryHashKey(key_));
   int ret_value = net::ERR_FAILED;
-  if (state_ == STATE_UNINITIALIZED &&
-      pending_operations_.size() == 0) {
+  if (use_optimistic_operations_ &&
+      state_ == STATE_UNINITIALIZED && pending_operations_.size() == 0) {
     ReturnEntryToCaller(out_entry);
-    // We can do optimistic Create.
     EnqueueOperation(base::Bind(&SimpleEntryImpl::CreateEntryInternal,
                                 this,
                                 CompletionCallback(),
@@ -183,8 +184,7 @@ int SimpleEntryImpl::CreateEntry(Entry** out_entry,
   // have the entry in the index but we don't have the created files yet, this
   // way we never leak files. CreationOperationComplete will remove the entry
   // from the index if the creation fails.
-  if (backend_.get())
-    backend_->index()->Insert(key_);
+  backend_->index()->Insert(key_);
 
   RunNextOperationIfNeeded();
   return ret_value;
@@ -288,46 +288,36 @@ int SimpleEntryImpl::WriteData(int stream_index,
     RecordWriteResult(WRITE_RESULT_OVER_MAX_SIZE);
     return net::ERR_FAILED;
   }
+  ScopedOperationRunner operation_runner(this);
 
-  int ret_value = net::ERR_FAILED;
-  if (state_ == STATE_READY && pending_operations_.size() == 0) {
-    // We can only do optimistic Write if there is no pending operations, so
-    // that we are sure that the next call to RunNextOperationIfNeeded will
-    // actually run the write operation that sets the stream size. It also
-    // prevents from previous possibly-conflicting writes that could be stacked
-    // in the |pending_operations_|. We could optimize this for when we have
-    // only read operations enqueued.
-    // TODO(gavinp,pasko): For performance, don't use a copy of an IOBuffer
-    // here to avoid paying the price of the RefCountedThreadSafe atomic
-    // operations.
-    IOBuffer* buf_copy = NULL;
-    if (buf) {
-      buf_copy = new IOBuffer(buf_len);
-      memcpy(buf_copy->data(), buf->data(), buf_len);
-    }
-    EnqueueOperation(base::Bind(&SimpleEntryImpl::WriteDataInternal,
-                                this,
-                                stream_index,
-                                offset,
-                                make_scoped_refptr(buf_copy),
-                                buf_len,
-                                CompletionCallback(),
-                                truncate));
-    ret_value = buf_len;
-  } else {
-    EnqueueOperation(base::Bind(&SimpleEntryImpl::WriteDataInternal,
-                                this,
-                                stream_index,
-                                offset,
-                                make_scoped_refptr(buf),
-                                buf_len,
-                                callback,
-                                truncate));
-    ret_value = net::ERR_IO_PENDING;
-  }
-
-  RunNextOperationIfNeeded();
-  return ret_value;
+  const bool do_optimistic_write = use_optimistic_operations_ &&
+      state_ == STATE_READY && pending_operations_.size() == 0;
+  if (!do_optimistic_write) {
+    pending_operations_.push(
+        base::Bind(&SimpleEntryImpl::WriteDataInternal, this, stream_index,
+                   offset, make_scoped_refptr(buf), buf_len, callback,
+                   truncate));
+    return net::ERR_IO_PENDING;
+  }
+
+  // We can only do optimistic Write if there is no pending operations, so that
+  // we are sure that the next call to RunNextOperationIfNeeded will actually
+  // run the write operation that sets the stream size. It also prevents from
+  // previous possibly-conflicting writes that could be stacked in the
+  // |pending_operations_|. We could optimize this for when we have only read
+  // operations enqueued.
+  // TODO(gavinp,pasko): For performance, don't use a copy of an IOBuffer here
+  // to avoid paying the price of the RefCountedThreadSafe atomic operations.
+  IOBuffer* buf_copy = NULL;
+  if (buf) {
+    buf_copy = new IOBuffer(buf_len);
+    memcpy(buf_copy->data(), buf->data(), buf_len);
+  }
+  EnqueueOperation(
+      base::Bind(&SimpleEntryImpl::WriteDataInternal, this, stream_index,
+                 offset, make_scoped_refptr(buf_copy), buf_len,
+                 CompletionCallback(), truncate));
+  return buf_len;
 }
 
 int SimpleEntryImpl::ReadSparseData(int64 offset,
diff --git a/net/disk_cache/simple/simple_entry_impl.h b/net/disk_cache/simple/simple_entry_impl.h
index f816f63..a6e69ca 100644
--- a/net/disk_cache/simple/simple_entry_impl.h
+++ b/net/disk_cache/simple/simple_entry_impl.h
@@ -37,9 +37,15 @@ class SimpleEntryImpl : public Entry, public base::RefCounted<SimpleEntryImpl>,
     public base::SupportsWeakPtr<SimpleEntryImpl> {
   friend class base::RefCounted<SimpleEntryImpl>;
  public:
-  SimpleEntryImpl(SimpleBackendImpl* backend,
-                  const base::FilePath& path,
+  enum OperationsMode {
+    NON_OPTIMISTIC_OPERATIONS,
+    OPTIMISTIC_OPERATIONS,
+  };
+
+  SimpleEntryImpl(const base::FilePath& path,
                   uint64 entry_hash,
+                  OperationsMode operations_mode,
+                  SimpleBackendImpl* backend,
                   net::NetLog* net_log);
 
   // Adds another reader/writer to this entry, if possible, returning |this| to
@@ -230,6 +236,7 @@ class SimpleEntryImpl : public Entry, public base::RefCounted<SimpleEntryImpl>,
   const scoped_refptr<base::TaskRunner> worker_pool_;
   const base::FilePath path_;
   const uint64 entry_hash_;
+  const bool use_optimistic_operations_;
   std::string key_;
 
   // |last_used_|, |last_modified_| and |data_size_| are copied from the