Revert 212336 "Reland r211790: "Make simple cache not use optimi..."

This appears to have broken the SimpleCacheAppCacheEnumerations2 test on linux

http://build.chromium.org/p/chromium.linux/builders/Linux%20Tests%20%28dbg%29%281%29%2832%29/builds/2062/steps/net_unittests/logs/SimpleCacheAppCacheEnumerations2

DiskCacheBackendTest.SimpleCacheAppCacheEnumerations2: 
[5611:5613:0718/071756:402235648:INFO:simple_index_file.cc(317)] Simple Cache Index is being restored from disk.
../../net/disk_cache/backend_unittest.cc:1120: Failure
Value of: second
Actual: "second"
Expected: entry2->GetKey()
Which is: "first"
../../net/disk_cache/backend_unittest.cc:1128: Failure
Value of: first
Actual: "first"
Expected: entry2->GetKey()
Which is: "second"
../../net/disk_cache/backend_unittest.cc:1138: Failure
Value of: second
Actual: "second"
Expected: entry2->GetKey()
Which is: "first"



> Reland r211790: "Make simple cache not use optimistic operations f..."
> 
> The initial attempt failed due to the SimpleAppCacheAppCacheLoad and
> SimpleCacheAppCacheEnumerations2 unit tests failing on Mac due to the file
> descriptor limit (set to 256).
> 
> 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.
> 
> BUG=257616
> 
> Review URL: https://chromiumcodereview.appspot.com/19369004

TBR=pliard@chromium.org

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

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

6 files changed, 56 insertions, 211 deletions

diff --git a/net/disk_cache/backend_unittest.cc b/net/disk_cache/backend_unittest.cc
index fe23805..31adbc4 100644
--- a/net/disk_cache/backend_unittest.cc
+++ b/net/disk_cache/backend_unittest.cc
@@ -2886,22 +2886,11 @@ 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();
@@ -2911,37 +2900,16 @@ 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 SIMPLE_MAYBE_MACOS(TestName) DISABLED_ ## TestName
+#define MAYBE_SimpleCacheLoad DISABLED_SimpleCacheLoad
 #else
-#define SIMPLE_MAYBE_MACOS(TestName) TestName
+#define MAYBE_SimpleCacheLoad SimpleCacheLoad
 #endif
-
-TEST_F(DiskCacheBackendTest, SIMPLE_MAYBE_MACOS(SimpleCacheLoad)) {
+TEST_F(DiskCacheBackendTest, MAYBE_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,
-       SIMPLE_MAYBE_MACOS(SimpleCacheAppCacheEnumerations2)) {
-  SetCacheType(net::APP_CACHE);
-  SetSimpleCacheMode();
-  BackendEnumerations2();
-}
-
 TEST_F(DiskCacheBackendTest, SimpleDoomRecent) {
   SetSimpleCacheMode();
   BackendDoomRecent();
@@ -2958,12 +2926,6 @@ 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 b49d308..9492a9e 100644
--- a/net/disk_cache/entry_unittest.cc
+++ b/net/disk_cache/entry_unittest.cc
@@ -2541,121 +2541,6 @@ 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 4037e8e..16e5ebe 100644
--- a/net/disk_cache/simple/simple_backend_impl.cc
+++ b/net/disk_cache/simple/simple_backend_impl.cc
@@ -177,10 +177,6 @@ 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) {
 }
 
@@ -396,8 +392,8 @@ scoped_refptr<SimpleEntryImpl> SimpleBackendImpl::CreateOrFindActiveEntry(
   if (insert_result.second)
     DCHECK(!it->second.get());
   if (!it->second.get()) {
-    SimpleEntryImpl* entry = new SimpleEntryImpl(
-        path_, entry_hash, entry_operations_mode_, this, net_log_);
+    SimpleEntryImpl* entry =
+        new SimpleEntryImpl(this, path_, entry_hash, net_log_);
     entry->set_key(key);
     it->second = entry->AsWeakPtr();
   }
@@ -412,6 +408,7 @@ scoped_refptr<SimpleEntryImpl> SimpleBackendImpl::CreateOrFindActiveEntry(
   return make_scoped_refptr(it->second.get());
 }
 
+
 int SimpleBackendImpl::OpenEntryFromHash(uint64 hash,
                                          Entry** entry,
                                          const CompletionCallback& callback) {
@@ -420,7 +417,7 @@ int SimpleBackendImpl::OpenEntryFromHash(uint64 hash,
     return OpenEntry(has_active->second->key(), entry, callback);
 
   scoped_refptr<SimpleEntryImpl> simple_entry =
-      new SimpleEntryImpl(path_, hash, entry_operations_mode_, this, net_log_);
+      new SimpleEntryImpl(this, path_, hash, 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 a21c3c7..e2fc8bc 100644
--- a/net/disk_cache/simple/simple_backend_impl.h
+++ b/net/disk_cache/simple/simple_backend_impl.h
@@ -18,7 +18,6 @@
 #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;
@@ -166,13 +165,12 @@ 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* const net_log_;
+  net::NetLog* 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 62f27f0..d2620d2 100644
--- a/net/disk_cache/simple/simple_entry_impl.cc
+++ b/net/disk_cache/simple/simple_entry_impl.cc
@@ -94,16 +94,14 @@ class SimpleEntryImpl::ScopedOperationRunner {
   SimpleEntryImpl* const entry_;
 };
 
-SimpleEntryImpl::SimpleEntryImpl(const FilePath& path,
+SimpleEntryImpl::SimpleEntryImpl(SimpleBackendImpl* backend,
+                                 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),
@@ -163,9 +161,10 @@ int SimpleEntryImpl::CreateEntry(Entry** out_entry,
   DCHECK(backend_.get());
   DCHECK_EQ(entry_hash_, simple_util::GetEntryHashKey(key_));
   int ret_value = net::ERR_FAILED;
-  if (use_optimistic_operations_ &&
-      state_ == STATE_UNINITIALIZED && pending_operations_.size() == 0) {
+  if (state_ == STATE_UNINITIALIZED &&
+      pending_operations_.size() == 0) {
     ReturnEntryToCaller(out_entry);
+    // We can do optimistic Create.
     EnqueueOperation(base::Bind(&SimpleEntryImpl::CreateEntryInternal,
                                 this,
                                 CompletionCallback(),
@@ -184,7 +183,8 @@ 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.
-  backend_->index()->Insert(key_);
+  if (backend_.get())
+    backend_->index()->Insert(key_);
 
   RunNextOperationIfNeeded();
   return ret_value;
@@ -288,36 +288,46 @@ int SimpleEntryImpl::WriteData(int stream_index,
     RecordWriteResult(WRITE_RESULT_OVER_MAX_SIZE);
     return net::ERR_FAILED;
   }
-  ScopedOperationRunner operation_runner(this);
 
-  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 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;
 }
 
 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 a6e69ca..f816f63 100644
--- a/net/disk_cache/simple/simple_entry_impl.h
+++ b/net/disk_cache/simple/simple_entry_impl.h
@@ -37,15 +37,9 @@ class SimpleEntryImpl : public Entry, public base::RefCounted<SimpleEntryImpl>,
     public base::SupportsWeakPtr<SimpleEntryImpl> {
   friend class base::RefCounted<SimpleEntryImpl>;
  public:
-  enum OperationsMode {
-    NON_OPTIMISTIC_OPERATIONS,
-    OPTIMISTIC_OPERATIONS,
-  };
-
-  SimpleEntryImpl(const base::FilePath& path,
+  SimpleEntryImpl(SimpleBackendImpl* backend,
+                  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
@@ -236,7 +230,6 @@ 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