Support optimistic Create and Write operations on the SimpleCache.

net/disk_cache/simple/simple_entry_impl.cc

 // Copyright (c) 2013 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_entry_impl.h"
 
 #include <algorithm>
 #include <cstring>
 #include <vector>
 
@@ skipping 53 matching lines @@
 SimpleEntryImpl::SimpleEntryImpl(SimpleBackendImpl* backend,
                                  const FilePath& path,
                                  const std::string& key,
                                  const uint64 entry_hash)
     : backend_(backend->AsWeakPtr()),
       path_(path),
       key_(key),
       entry_hash_(entry_hash),
       open_count_(0),
       state_(STATE_UNINITIALIZED),
-      synchronous_entry_(NULL) {
+      synchronous_entry_(NULL),
+      optimistic_(true) {
   DCHECK_EQ(entry_hash, simple_util::GetEntryHashKey(key));
   COMPILE_ASSERT(arraysize(data_size_) == arraysize(crc32s_end_offset_),
                  arrays_should_be_same_size);
   COMPILE_ASSERT(arraysize(data_size_) == arraysize(crc32s_),
                  arrays_should_be_same_size2);
   COMPILE_ASSERT(arraysize(data_size_) == arraysize(have_written_),
                  arrays_should_be_same_size3);
-
   MakeUninitialized();
 }
 
 int SimpleEntryImpl::OpenEntry(Entry** out_entry,
                                const CompletionCallback& callback) {
   DCHECK(backend_);
+  // This enumeration is used in histograms, add entries only at end.
+  enum OpenEntryIndexEnum {
+    INDEX_NOEXIST = 0,
+    INDEX_MISS = 1,
+    INDEX_HIT = 2,
+    INDEX_MAX = 3,
+  };
+  OpenEntryIndexEnum open_entry_index_enum = INDEX_NOEXIST;
+  if (backend_) {
+    if (backend_->index()->Has(key_))
+      open_entry_index_enum = INDEX_HIT;
+    else
+      open_entry_index_enum = INDEX_MISS;
+  }
+  UMA_HISTOGRAM_ENUMERATION("SimpleCache.OpenEntryIndexState",
+                            open_entry_index_enum, INDEX_MAX);
+
+  // If entry is not known to the index, initiate fast failover to the network.
+  if (open_entry_index_enum == INDEX_MISS)
+    return net::ERR_FAILED;
 
   pending_operations_.push(base::Bind(&SimpleEntryImpl::OpenEntryInternal,
-                                      this, out_entry, callback));
+                                      this, callback, out_entry));
   RunNextOperationIfNeeded();
   return net::ERR_IO_PENDING;
 }
 
 int SimpleEntryImpl::CreateEntry(Entry** out_entry,
                                  const CompletionCallback& callback) {
   DCHECK(backend_);
-  pending_operations_.push(base::Bind(&SimpleEntryImpl::CreateEntryInternal,
-                                      this, out_entry, callback));
+  int ret_value = net::ERR_FAILED;
+  if (state_ == STATE_UNINITIALIZED &&
+      pending_operations_.size() == 0 &&
+      optimistic_) {
+    ReturnEntryToCaller(out_entry);
+    // We can do optimistic Create.
+    pending_operations_.push(base::Bind(&SimpleEntryImpl::CreateEntryInternal,
+                                        this,
+                                        CompletionCallback(),
+                                        static_cast<Entry**>(NULL)));
+    ret_value = net::OK;
+  } else {
+    pending_operations_.push(base::Bind(&SimpleEntryImpl::CreateEntryInternal,
+                                        this,
+                                        callback,
+                                        out_entry));
+    ret_value = net::ERR_IO_PENDING;
+  }
+
+  // We insert the entry in the index before creating the entry files in the
+  // SimpleSynchronousEntry, because this way the worst scenario is when we
+  // 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_)
+    backend_->index()->Insert(key_);
+
+  // Since we don't know the correct values for |last_used_| and
+  // |last_modified_| yet, we make this approximation.
+  last_used_ = last_modified_ = base::Time::Now();
+
   RunNextOperationIfNeeded();
-  return net::ERR_IO_PENDING;
+  return ret_value;
 }
 
 int SimpleEntryImpl::DoomEntry(const CompletionCallback& callback) {
   MarkAsDoomed();
-
   scoped_ptr<int> result(new int());
   Closure task = base::Bind(&SimpleSynchronousEntry::DoomEntry, path_, key_,
                             entry_hash_, result.get());
   Closure reply = base::Bind(&CallCompletionCallback,
                              callback, base::Passed(&result));
   WorkerPool::PostTaskAndReply(FROM_HERE, task, reply, true);
   return net::ERR_IO_PENDING;
 }
 
 
@@ skipping 27 matching lines @@
   return last_used_;
 }
 
 Time SimpleEntryImpl::GetLastModified() const {
   DCHECK(io_thread_checker_.CalledOnValidThread());
   return last_modified_;
 }
 
 int32 SimpleEntryImpl::GetDataSize(int stream_index) const {
   DCHECK(io_thread_checker_.CalledOnValidThread());
+  DCHECK(data_size_[stream_index] >= 0);

[gavinp, 2013/05/01 13:11:22]

DCHECK_LE please.

[felipeg, 2013/05/02 09:49:27]

Done.

   return data_size_[stream_index];
 }
 
 int SimpleEntryImpl::ReadData(int stream_index,
                               int offset,
                               net::IOBuffer* buf,
                               int buf_len,
                               const CompletionCallback& callback) {
   DCHECK(io_thread_checker_.CalledOnValidThread());
   if (stream_index < 0 || stream_index >= kSimpleEntryFileCount || buf_len < 0)
     return net::ERR_INVALID_ARGUMENT;
   if (offset >= data_size_[stream_index] || offset < 0 || !buf_len)
     return 0;
-  buf_len = std::min(buf_len, data_size_[stream_index] - offset);
+
   // TODO(felipeg): Optimization: Add support for truly parallel read
   // operations.
   pending_operations_.push(
       base::Bind(&SimpleEntryImpl::ReadDataInternal,
                  this,
                  stream_index,
                  offset,
                  make_scoped_refptr(buf),
                  buf_len,
                  callback));
   RunNextOperationIfNeeded();
   return net::ERR_IO_PENDING;
 }
 
 int SimpleEntryImpl::WriteData(int stream_index,
                                int offset,
                                net::IOBuffer* buf,
                                int buf_len,
                                const CompletionCallback& callback,
                                bool truncate) {
   DCHECK(io_thread_checker_.CalledOnValidThread());
   if (stream_index < 0 || stream_index >= kSimpleEntryFileCount || offset < 0 ||
       buf_len < 0) {
     return net::ERR_INVALID_ARGUMENT;
   }
-  pending_operations_.push(
-      base::Bind(&SimpleEntryImpl::WriteDataInternal,
-                 this,
-                 stream_index,
-                 offset,
-                 make_scoped_refptr(buf),
-                 buf_len,
-                 callback,
-                 truncate));
+
+  int ret_value = net::ERR_FAILED;
+  if (state_ == STATE_READY && pending_operations_.size() == 0 && optimistic_) {
+    // 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.
+    pending_operations_.push(
+        base::Bind(&SimpleEntryImpl::WriteDataInternal,
+                   this,
+                   stream_index,
+                   offset,
+                   make_scoped_refptr(buf),
+                   buf_len,
+                   CompletionCallback(),
+                   truncate));
+    ret_value = buf_len;
+  } else {
+    pending_operations_.push(
+        base::Bind(&SimpleEntryImpl::WriteDataInternal,
+                   this,
+                   stream_index,
+                   offset,
+                   make_scoped_refptr(buf),
+                   buf_len,
+                   callback,
+                   truncate));
+    ret_value = net::ERR_IO_PENDING;
+  }
+
+  if (truncate)
+    data_size_[stream_index] = offset + buf_len;
+  else
+    data_size_[stream_index] = std::max(offset + buf_len,
+                                        data_size_[stream_index]);
+
+  // Since we don't know the correct values for |last_used_| and
+  // |last_modified_| yet, we make this approximation.
+  last_used_ = last_modified_ = base::Time::Now();
+
   RunNextOperationIfNeeded();
-  // TODO(felipeg): Optimization: Add support for optimistic writes, quickly
-  // returning net::OK here.
-  return net::ERR_IO_PENDING;
+  return ret_value;
 }
 
 int SimpleEntryImpl::ReadSparseData(int64 offset,
                                     net::IOBuffer* buf,
                                     int buf_len,
                                     const CompletionCallback& callback) {
   DCHECK(io_thread_checker_.CalledOnValidThread());
   // TODO(gavinp): Determine if the simple backend should support sparse data.
   NOTIMPLEMENTED();
   return net::ERR_FAILED;
@@ skipping 34 matching lines @@
 int SimpleEntryImpl::ReadyForSparseIO(const CompletionCallback& callback) {
   DCHECK(io_thread_checker_.CalledOnValidThread());
   // TODO(gavinp): Determine if the simple backend should support sparse data.
   NOTIMPLEMENTED();
   return net::ERR_FAILED;
 }
 
 SimpleEntryImpl::~SimpleEntryImpl() {
   DCHECK(io_thread_checker_.CalledOnValidThread());
   DCHECK_EQ(0U, pending_operations_.size());
-  DCHECK_EQ(STATE_UNINITIALIZED, state_);
+  DCHECK(STATE_UNINITIALIZED == state_ || STATE_FAILURE == state_);
   DCHECK(!synchronous_entry_);
   RemoveSelfFromBackend();
 }
 
 void SimpleEntryImpl::MakeUninitialized() {
+
   state_ = STATE_UNINITIALIZED;
   std::memset(crc32s_end_offset_, 0, sizeof(crc32s_end_offset_));
   std::memset(crc32s_, 0, sizeof(crc32s_));
   std::memset(have_written_, 0, sizeof(have_written_));
+  std::memset(data_size_, 0, sizeof(data_size_));
 }
 
 void SimpleEntryImpl::ReturnEntryToCaller(Entry** out_entry) {
+  if (!out_entry)

[gavinp, 2013/05/01 13:11:22]

This seems like a belt and suspenders; why not make this a DCHECK() and trust
callers to get this right?

Alternatively, rename it, but I think I prefer the DCHECK with this clear name.

[felipeg, 2013/05/02 09:49:27]

Done.

+    return;
+
   ++open_count_;
   AddRef();  // Balanced in Close()
   *out_entry = this;
 }
 
 void SimpleEntryImpl::RemoveSelfFromBackend() {
   if (!backend_)
     return;
   backend_->OnDeactivated(this);
   backend_.reset();
 }
 
 void SimpleEntryImpl::MarkAsDoomed() {
   if (!backend_)
     return;
   backend_->index()->Remove(key_);
   RemoveSelfFromBackend();
 }
 
 void SimpleEntryImpl::RunNextOperationIfNeeded() {
   DCHECK(io_thread_checker_.CalledOnValidThread());
-
   UMA_HISTOGRAM_CUSTOM_COUNTS("SimpleCache.EntryOperationsPending",
                               pending_operations_.size(), 0, 100, 20);
-
   if (!pending_operations_.empty() && state_ != STATE_IO_PENDING) {
     base::Closure operation = pending_operations_.front();
     pending_operations_.pop();
     operation.Run();
     // |this| may have been deleted.
   }
 }
 
-void SimpleEntryImpl::OpenEntryInternal(Entry** out_entry,
-                                        const CompletionCallback& callback) {
+void SimpleEntryImpl::OpenEntryInternal(const CompletionCallback& callback,
+                                        Entry** out_entry) {

[gavinp, 2013/05/01 13:11:22]

I'm confused by the value of optimistic open. Can you explain what the benefit
of it is?

[felipeg, 2013/05/02 09:49:27]

This CL doesnt have optimistic open.
Only Create and Write.

   ScopedOperationRunner operation_runner(this);
-
   if (state_ == STATE_READY) {
     ReturnEntryToCaller(out_entry);
-    MessageLoopProxy::current()->PostTask(FROM_HERE, base::Bind(callback,
-                                                                net::OK));
+    if (!callback.is_null())
+      MessageLoopProxy::current()->PostTask(FROM_HERE, base::Bind(callback,
+                                                                  net::OK));
+    return;
+  } else if (state_ == STATE_FAILURE) {
+    if (!callback.is_null())
+      MessageLoopProxy::current()->PostTask(FROM_HERE, base::Bind(
+          callback, net::ERR_FAILED));
     return;
   }
   DCHECK_EQ(STATE_UNINITIALIZED, state_);
-
-  // This enumeration is used in histograms, add entries only at end.
-  enum OpenEntryIndexEnum {
-    INDEX_NOEXIST = 0,
-    INDEX_MISS = 1,
-    INDEX_HIT = 2,
-    INDEX_MAX = 3,
-  };
-  OpenEntryIndexEnum open_entry_index_enum = INDEX_NOEXIST;
-  if (backend_) {
-    if (backend_->index()->Has(key_))
-      open_entry_index_enum = INDEX_HIT;
-    else
-      open_entry_index_enum = INDEX_MISS;
-  }
-  UMA_HISTOGRAM_ENUMERATION("SimpleCache.OpenEntryIndexState",
-                            open_entry_index_enum, INDEX_MAX);
-  // If entry is not known to the index, initiate fast failover to the network.
-  if (open_entry_index_enum == INDEX_MISS) {
-    MessageLoopProxy::current()->PostTask(FROM_HERE,
-                                          base::Bind(callback,
-                                                     net::ERR_FAILED));
-    return;
-  }
   state_ = STATE_IO_PENDING;
-
   const base::TimeTicks start_time = base::TimeTicks::Now();
   typedef SimpleSynchronousEntry* PointerToSimpleSynchronousEntry;
   scoped_ptr<PointerToSimpleSynchronousEntry> sync_entry(
       new PointerToSimpleSynchronousEntry());
   Closure task = base::Bind(&SimpleSynchronousEntry::OpenEntry, path_, key_,
                             entry_hash_, sync_entry.get());
   Closure reply = base::Bind(&SimpleEntryImpl::CreationOperationComplete, this,
                              callback, start_time, base::Passed(&sync_entry),
                              out_entry);
   WorkerPool::PostTaskAndReply(FROM_HERE, task, reply, true);
 }
 
-void SimpleEntryImpl::CreateEntryInternal(Entry** out_entry,
-                                          const CompletionCallback& callback) {
+void SimpleEntryImpl::CreateEntryInternal(const CompletionCallback& callback,
+                                          Entry** out_entry) {
   ScopedOperationRunner operation_runner(this);
-
-  if (state_ == STATE_READY) {
+  if (state_ != STATE_UNINITIALIZED) {

[gavinp, 2013/05/01 13:11:22]

This seems wrong. What if we're in STATE_IO_PENDING, and the pending IO
operation is a Close? Why doesn't this make a test fail?

[felipeg, 2013/05/02 09:49:27]

Because CreateInternal is pushed in the operations queue and is only called
after whatever operation (such as Close) has finished.

That is true to all XXInternal calls.
We could put a DCHECK( state_ == READY || == FAILURE) in every XXInternal call
...
but I don't think it is necessary.

     // There is already an active normal entry.
-    MessageLoopProxy::current()->PostTask(FROM_HERE,
-                                          base::Bind(callback,
-                                                     net::ERR_FAILED));
+    if (!callback.is_null())
+      MessageLoopProxy::current()->PostTask(FROM_HERE, base::Bind(
+          callback, net::ERR_FAILED));
     return;
   }
   DCHECK_EQ(STATE_UNINITIALIZED, state_);
 
   state_ = STATE_IO_PENDING;
 
   // If creation succeeds, we should mark all streams to be saved on close.
   for (int i = 0; i < kSimpleEntryFileCount; ++i)
     have_written_[i] = true;
 
-  // We insert the entry in the index before creating the entry files in the
-  // SimpleSynchronousEntry, because this way the worst scenario is when we
-  // 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_)
-    backend_->index()->Insert(key_);
   const base::TimeTicks start_time = base::TimeTicks::Now();
   typedef SimpleSynchronousEntry* PointerToSimpleSynchronousEntry;
   scoped_ptr<PointerToSimpleSynchronousEntry> sync_entry(
       new PointerToSimpleSynchronousEntry());
   Closure task = base::Bind(&SimpleSynchronousEntry::CreateEntry, path_, key_,
                             entry_hash_, sync_entry.get());
   Closure reply = base::Bind(&SimpleEntryImpl::CreationOperationComplete, this,
                              callback, start_time, base::Passed(&sync_entry),
                              out_entry);
   WorkerPool::PostTaskAndReply(FROM_HERE, task, reply, true);
 }
 
 void SimpleEntryImpl::CloseInternal() {
   DCHECK(io_thread_checker_.CalledOnValidThread());
-  DCHECK_EQ(0U, pending_operations_.size());
-  DCHECK_EQ(STATE_READY, state_);
-  DCHECK(synchronous_entry_);
-
-  state_ = STATE_IO_PENDING;
-
   typedef SimpleSynchronousEntry::CRCRecord CRCRecord;
-
   scoped_ptr<std::vector<CRCRecord> >
       crc32s_to_write(new std::vector<CRCRecord>());
-  for (int i = 0; i < kSimpleEntryFileCount; ++i) {
-    if (have_written_[i]) {
-      if (data_size_[i] == crc32s_end_offset_[i]) {
-        int32 crc = data_size_[i] == 0 ? crc32(0, Z_NULL, 0) : crc32s_[i];
-        crc32s_to_write->push_back(CRCRecord(i, true, crc));
-      } else {
-        crc32s_to_write->push_back(CRCRecord(i, false, 0));
+
+  if (state_ == STATE_READY) {
+    DCHECK(synchronous_entry_);
+    state_ = STATE_IO_PENDING;
+    for (int i = 0; i < kSimpleEntryFileCount; ++i) {
+      if (have_written_[i]) {
+        if (data_size_[i] == crc32s_end_offset_[i]) {
+          int32 crc = data_size_[i] == 0 ? crc32(0, Z_NULL, 0) : crc32s_[i];
+          crc32s_to_write->push_back(CRCRecord(i, true, crc));
+        } else {
+          crc32s_to_write->push_back(CRCRecord(i, false, 0));
+        }
       }
     }
+  } else {
+    DCHECK_EQ(STATE_FAILURE, state_);
   }
-  Closure task = base::Bind(&SimpleSynchronousEntry::Close,
-                            base::Unretained(synchronous_entry_),
-                            base::Passed(&crc32s_to_write));
-  Closure reply = base::Bind(&SimpleEntryImpl::CloseOperationComplete, this);
-  WorkerPool::PostTaskAndReply(FROM_HERE, task, reply, true);
-  synchronous_entry_ = NULL;
+
+  if (synchronous_entry_) {
+    Closure task = base::Bind(&SimpleSynchronousEntry::Close,
+                              base::Unretained(synchronous_entry_),
+                              base::Passed(&crc32s_to_write));
+    Closure reply = base::Bind(&SimpleEntryImpl::CloseOperationComplete, this);
+    synchronous_entry_ = NULL;
+    WorkerPool::PostTaskAndReply(FROM_HERE, task, reply, true);
+  } else {
+    synchronous_entry_ = NULL;
+    CloseOperationComplete();
+  }
 }
 
 void SimpleEntryImpl::ReadDataInternal(int stream_index,
                                        int offset,
                                        net::IOBuffer* buf,
                                        int buf_len,
                                        const CompletionCallback& callback) {
   DCHECK(io_thread_checker_.CalledOnValidThread());
+  ScopedOperationRunner operation_runner(this);
+
+  if (state_ == STATE_FAILURE || state_ == STATE_UNINITIALIZED) {
+    if (!callback.is_null())
+      MessageLoopProxy::current()->PostTask(FROM_HERE, base::Bind(
+          callback, net::ERR_FAILED));
+    return;
+  }
   DCHECK_EQ(STATE_READY, state_);
+  buf_len = std::min(buf_len, GetDataSize(stream_index) - offset);
+  if (offset < 0 || buf_len <= 0) {
+    // If there is nothing to read, we bail out before setting state_ to
+    // STATE_IO_PENDING.
+    if (!callback.is_null())
+      MessageLoopProxy::current()->PostTask(FROM_HERE, base::Bind(
+          callback, 0));
+    return;
+  }
+
   state_ = STATE_IO_PENDING;
   if (backend_)
     backend_->index()->UseIfExists(key_);
 
   scoped_ptr<uint32> read_crc32(new uint32());
   scoped_ptr<int> result(new int());
   Closure task = base::Bind(&SimpleSynchronousEntry::ReadData,
                             base::Unretained(synchronous_entry_),
                             stream_index, offset, make_scoped_refptr(buf),
                             buf_len, read_crc32.get(), result.get());
   Closure reply = base::Bind(&SimpleEntryImpl::ReadOperationComplete, this,
                              stream_index, offset, callback,
                              base::Passed(&read_crc32), base::Passed(&result));
   WorkerPool::PostTaskAndReply(FROM_HERE, task, reply, true);
 }
 
 void SimpleEntryImpl::WriteDataInternal(int stream_index,
                                        int offset,
                                        net::IOBuffer* buf,
                                        int buf_len,
                                        const CompletionCallback& callback,
                                        bool truncate) {
   DCHECK(io_thread_checker_.CalledOnValidThread());
+  ScopedOperationRunner operation_runner(this);
+  if (state_ == STATE_FAILURE || state_ == STATE_UNINITIALIZED) {
+    if (!callback.is_null()) {
+      // We need to posttask so that we don't go in a loop when we call the
+      // callback directly.
+      MessageLoopProxy::current()->PostTask(FROM_HERE, base::Bind(
+          callback, net::ERR_FAILED));
+    }
+    // |this| may be destroyed after return here.
+    return;
+  }
   DCHECK_EQ(STATE_READY, state_);
   state_ = STATE_IO_PENDING;
   if (backend_)
     backend_->index()->UseIfExists(key_);
   // It is easy to incrementally compute the CRC from [0 .. |offset + buf_len|)
   // if |offset == 0| or we have already computed the CRC for [0 .. offset).
   // We rely on most write operations being sequential, start to end to compute
   // the crc of the data. When we write to an entry and close without having
   // done a sequential write, we don't check the CRC on read.
   if (offset == 0 || crc32s_end_offset_[stream_index] == offset) {
@@ skipping 20 matching lines @@
 }
 
 void SimpleEntryImpl::CreationOperationComplete(
     const CompletionCallback& completion_callback,
     const base::TimeTicks& start_time,
     scoped_ptr<SimpleSynchronousEntry*> in_sync_entry,
     Entry** out_entry) {
   DCHECK(io_thread_checker_.CalledOnValidThread());
   DCHECK_EQ(state_, STATE_IO_PENDING);
   DCHECK(in_sync_entry);
-
   ScopedOperationRunner operation_runner(this);
 
   bool creation_failed = !*in_sync_entry;
   UMA_HISTOGRAM_BOOLEAN("SimpleCache.EntryCreationResult", creation_failed);
   if (creation_failed) {
-    completion_callback.Run(net::ERR_FAILED);
-    MarkAsDoomed();
+    if (!completion_callback.is_null())

[gavinp, 2013/05/01 13:11:22]

completion_callback.is_null() is another, redundant, signal of an optimistic
create. That's a bit confusing.

[felipeg, 2013/05/02 09:49:27]

It is not, it is just a side-effect, we should not rely in this "signal".

In fact the API requires us to check that the callback is not null before
calling. It is allowed to call operations with a null callback.
And Actually if the callback is null we should be synchronous (which we don't
support yet).

[gavinp, 2013/05/02 12:47:39]

Hrm, confusing. 
That second point, the synchronous interface, has to do with how sparse entries
are implemented and for testing, it's not vital for us unless we want to reuse
the sparse entry support, but maybe we want the test coverage? I'm not sure.

[felipeg, 2013/05/02 13:55:58]

I see.
But I still don't see a problem with checking is_null()
But I can change that if you want. What is your suggestion to avoid that ?

[gavinp, 2013/05/02 14:05:45]

Ugh, I was unclear again. I meant to say: OK, yeah, we do have to check it, I
agree now. I was just expanding on why sync mode might or might not make sense
in the future.

+      MessageLoopProxy::current()->PostTask(FROM_HERE, base::Bind(
+          completion_callback, net::ERR_FAILED));
+
+    MarkAsDoomed();  // Removes entry from the index.
     MakeUninitialized();
+    state_ = STATE_FAILURE;
+    // |this| may now be deleted.

[gavinp, 2013/05/01 13:11:22]

!!! if this comment is correct, then we are in grave danger, right?

[felipeg, 2013/05/02 09:49:27]

Done.

     return;
   }
+  // If out_entry is NULL, it means we already called ReturnEntryToCaller from
+  // the optimistic Create case.
+  if (out_entry)
+    ReturnEntryToCaller(out_entry);
+
   state_ = STATE_READY;
   synchronous_entry_ = *in_sync_entry;
   SetSynchronousData();
-  ReturnEntryToCaller(out_entry);
   UMA_HISTOGRAM_TIMES("SimpleCache.EntryCreationTime",
                       (base::TimeTicks::Now() - start_time));
-  completion_callback.Run(net::OK);
+
+  if (!completion_callback.is_null())
+    MessageLoopProxy::current()->PostTask(FROM_HERE, base::Bind(
+        completion_callback, net::OK));
 }
 
 void SimpleEntryImpl::EntryOperationComplete(
     int stream_index,
     const CompletionCallback& completion_callback,
     scoped_ptr<int> result) {
   DCHECK(io_thread_checker_.CalledOnValidThread());
   DCHECK(synchronous_entry_);
   DCHECK_EQ(STATE_IO_PENDING, state_);
   DCHECK(result);
-
   state_ = STATE_READY;
-
   if (*result < 0) {
     MarkAsDoomed();
+    state_ = STATE_FAILURE;
     crc32s_end_offset_[stream_index] = 0;
+  } else {
+    SetSynchronousData();
   }
-  SetSynchronousData();
-  completion_callback.Run(*result);
+
+  if (!completion_callback.is_null())
+    MessageLoopProxy::current()->PostTask(FROM_HERE, base::Bind(
+        completion_callback, *result));
   RunNextOperationIfNeeded();
 }
 
 void SimpleEntryImpl::ReadOperationComplete(
     int stream_index,
     int offset,
     const CompletionCallback& completion_callback,
     scoped_ptr<uint32> read_crc32,
     scoped_ptr<int> result) {
   DCHECK(io_thread_checker_.CalledOnValidThread());
@@ skipping 45 matching lines @@
   DCHECK_EQ(STATE_IO_PENDING, state_);
   DCHECK(result);
   if (*result == net::OK)
     *result = orig_result;
   EntryOperationComplete(stream_index, completion_callback, result.Pass());
 }
 
 void SimpleEntryImpl::CloseOperationComplete() {
   DCHECK(!synchronous_entry_);
   DCHECK_EQ(0, open_count_);
-  DCHECK_EQ(STATE_IO_PENDING, state_);
-
+  DCHECK(STATE_IO_PENDING == state_ || STATE_FAILURE == state_);
   MakeUninitialized();
   RunNextOperationIfNeeded();
 }
 
 void SimpleEntryImpl::SetSynchronousData() {
   DCHECK(io_thread_checker_.CalledOnValidThread());
+  DCHECK(synchronous_entry_);
   DCHECK_EQ(STATE_READY, state_);
   // 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);
   if (backend_)
     backend_->index()->UpdateEntrySize(key_, synchronous_entry_->GetFileSize());
 }
 
 }  // namespace disk_cache