[net] Better StopCaching() handling for HttpCache::Transaction.

net/http/http_cache_transaction.cc

Index: net/http/http_cache_transaction.cc
diff --git a/net/http/http_cache_transaction.cc b/net/http/http_cache_transaction.cc
index 85ad67ede6f1d05be19d29ea077ceb3bb500b749..f8f974d09c51a166029ca3f681bbb1d916a0fc8d 100644
--- a/net/http/http_cache_transaction.cc
+++ b/net/http/http_cache_transaction.cc
@@ -266,8 +266,10 @@ HttpCache::Transaction::Transaction(RequestPriority priority, HttpCache* cache)
       done_reading_(false),
       vary_mismatch_(false),
       couldnt_conditionalize_request_(false),
+      stopped_caching_(false),
       bypass_lock_for_test_(false),
       fail_conditionalization_for_test_(false),
+      read_buf_len_(0),
       io_buf_len_(0),
       read_offset_(0),
       effective_load_flags_(0),
@@ -289,22 +291,28 @@ HttpCache::Transaction::~Transaction() {
   // after this point.
   callback_.Reset();
 
-  if (cache_) {
-    if (entry_) {
-      bool cancel_request = reading_ && response_.headers.get();
-      if (cancel_request) {
-        if (partial_) {
-          entry_->disk_entry->CancelSparseIO();
-        } else {
-          cancel_request &= (response_.headers->response_code() == 200);
-        }
-      }
+  RecordHistograms();
+
+  if (!cache_)
+    return;
 
-      cache_->DoneWithEntry(entry_, this, cancel_request);
-    } else if (cache_pending_) {
+  if (!entry_) {
+    if (cache_pending_)
       cache_->RemovePendingTransaction(this);
-    }
+    return;
   }
+
+  bool cancel_request = reading_ && response_.headers.get() &&
+                        (response_.headers->response_code() == 200 || partial_);
+  if (cancel_request) {
+    if (partial_)
+      entry_->disk_entry->CancelSparseIO();
+
+    AbandonCacheEntry(CompletionCallback());
+    return;
+  }
+
+  ReleaseCacheEntry(EntryState::DOOMED);
 }
 
 int HttpCache::Transaction::WriteMetadata(IOBuffer* buf, int buf_len,
@@ -323,26 +331,6 @@ int HttpCache::Transaction::WriteMetadata(IOBuffer* buf, int buf_len,
                                        callback, true);
 }
 
-bool HttpCache::Transaction::AddTruncatedFlag() {
-  DCHECK(mode_ & WRITE || mode_ == NONE);
-
-  // Don't set the flag for sparse entries.
-  if (partial_ && !truncated_)
-    return true;
-
-  if (!CanResume(true))
-    return false;
-
-  // We may have received the whole resource already.
-  if (done_reading_)
-    return true;
-
-  truncated_ = true;
-  next_state_ = STATE_CACHE_WRITE_TRUNCATED_RESPONSE;
-  DoLoop(OK);
-  return true;
-}
-
 LoadState HttpCache::Transaction::GetWriterLoadState() const {
   if (network_trans_.get())
     return network_trans_->GetLoadState();
@@ -457,7 +445,6 @@ int HttpCache::Transaction::Read(IOBuffer* buf, int buf_len,
   DCHECK(buf);
   DCHECK_GT(buf_len, 0);
   DCHECK(!callback.is_null());
-
   DCHECK(callback_.is_null());
 
   if (!cache_.get())
@@ -469,14 +456,16 @@ int HttpCache::Transaction::Read(IOBuffer* buf, int buf_len,
   if (auth_response_.headers.get() && mode_ != NONE) {
     UpdateTransactionPattern(PATTERN_NOT_COVERED);
     DCHECK(mode_ & WRITE);
-    DoneWritingToEntry(mode_ == READ_WRITE);
-    mode_ = NONE;
+    ReleaseCacheEntry(mode_ == READ_WRITE ? EntryState::SUCCEEDED
+                                          : EntryState::DOOMED);
   }
 
   reading_ = true;
   read_buf_ = buf;
-  io_buf_len_ = buf_len;
-  if (network_trans_) {
+  read_buf_len_ = buf_len;

[rvargas (doing something else), 2015/09/11 23:56:18]

Why do we need another member? (vs io_buf_len_)

[asanka, 2015/09/17 21:59:41]

When we are switching to a single network request, the transaction might need to
overwrite io_buf_len_ to write the truncated bit.

[rvargas (doing something else), 2015/09/21 22:05:59]

Acknowledged.

+  if (stopped_caching_ && (mode_ & WRITE) && entry_) {

[rvargas (doing something else), 2015/09/11 23:56:18]

Shouldn't this look closer to line 1483? (aka, what if this is a range request
etc... the logic should be in only one place, if possible).

+    next_state_ = STATE_SWITCH_TO_NETWORK;
+  } else if (network_trans_) {
     DCHECK(mode_ == WRITE || mode_ == NONE ||
            (mode_ == READ_WRITE && partial_));
     next_state_ = STATE_NETWORK_READ;
@@ -495,21 +484,8 @@ int HttpCache::Transaction::Read(IOBuffer* buf, int buf_len,
 }
 
 void HttpCache::Transaction::StopCaching() {
-  // We really don't know where we are now. Hopefully there is no operation in
-  // progress, but nothing really prevents this method to be called after we
-  // returned ERR_IO_PENDING. We cannot attempt to truncate the entry at this
-  // point because we need the state machine for that (and even if we are really
-  // free, that would be an asynchronous operation). In other words, keep the
-  // entry how it is (it will be marked as truncated at destruction), and let
-  // the next piece of code that executes know that we are now reading directly
-  // from the net.
-  // TODO(mmenke):  This doesn't release the lock on the cache entry, so a
-  //                future request for the resource will be blocked on this one.
-  //                Fix this.
-  if (cache_.get() && entry_ && (mode_ & WRITE) && network_trans_.get() &&
-      !is_sparse_ && !range_requested_) {
-    mode_ = NONE;
-  }
+  DCHECK(callback_.is_null());
+  stopped_caching_ = true;
 }
 
 bool HttpCache::Transaction::GetFullRequestHeaders(
@@ -529,18 +505,11 @@ int64 HttpCache::Transaction::GetTotalReceivedBytes() const {
 }
 
 void HttpCache::Transaction::DoneReading() {
-  if (cache_.get() && entry_) {
-    DCHECK_NE(mode_, UPDATE);
-    if (mode_ & WRITE) {
-      DoneWritingToEntry(true);
-    } else if (mode_ & READ) {
-      // It is necessary to check mode_ & READ because it is possible
-      // for mode_ to be NONE and entry_ non-NULL with a write entry
-      // if StopCaching was called.
-      cache_->DoneReadingFromEntry(entry_, this);
-      entry_ = NULL;
-    }
-  }
+  DCHECK_NE(mode_, UPDATE);
+  if (!cache_ || !entry_)
+    return;
+  done_reading_ = true;
+  ReleaseCacheEntry(EntryState::SUCCEEDED);
 }
 
 const HttpResponseInfo* HttpCache::Transaction::GetResponseInfo() const {
@@ -902,6 +871,9 @@ int HttpCache::Transaction::DoLoop(int result) {
       case STATE_CACHE_READ_METADATA_COMPLETE:
         rv = DoCacheReadMetadataComplete(rv);
         break;
+      case STATE_SWITCH_TO_NETWORK:
+        rv = DoSwitchToNetwork();
+        break;
       case STATE_NETWORK_READ:
         DCHECK_EQ(OK, rv);
         rv = DoNetworkRead();
@@ -922,13 +894,6 @@ int HttpCache::Transaction::DoLoop(int result) {
       case STATE_CACHE_WRITE_DATA_COMPLETE:
         rv = DoCacheWriteDataComplete(rv);
         break;
-      case STATE_CACHE_WRITE_TRUNCATED_RESPONSE:
-        DCHECK_EQ(OK, rv);
-        rv = DoCacheWriteTruncatedResponse();
-        break;
-      case STATE_CACHE_WRITE_TRUNCATED_RESPONSE_COMPLETE:
-        rv = DoCacheWriteTruncatedResponseComplete(rv);
-        break;
       default:
         NOTREACHED() << "bad state";
         rv = ERR_FAILED;
@@ -937,7 +902,8 @@ int HttpCache::Transaction::DoLoop(int result) {
   } while (rv != ERR_IO_PENDING && next_state_ != STATE_NONE);
 
   if (rv != ERR_IO_PENDING && !callback_.is_null()) {
-    read_buf_ = NULL;  // Release the buffer before invoking the callback.
+    read_buf_ = nullptr;  // Release the buffer before invoking the callback.
+    read_buf_len_ = 0;
     base::ResetAndReturn(&callback_).Run(rv);
   }
 
@@ -1000,6 +966,25 @@ int HttpCache::Transaction::DoGetBackendComplete(int result) {
   if (!(mode_ & READ) && effective_load_flags_ & LOAD_ONLY_FROM_CACHE)
     return ERR_CACHE_MISS;
 
+  // The client may have already decided not to write anything to the cache.
+  if (stopped_caching_ && mode_ != NONE) {

[rvargas (doing something else), 2015/09/11 23:56:17]

Is this for restart? We should not allow StopCaching to be called before Start.

Can we get here at all with the flag set?

+    // Avoid touching the cache at all in this case unless the requestor has
+    // also requested that we prefer the cache entry. The transaction interprets
+    // the latter case as "Don't write any more response data to the cache."
+    if (mode_ != READ && !(effective_load_flags_ & LOAD_PREFERRING_CACHE))
+      mode_ = NONE;
+
+    // If the WRITE bit is still set, then the transaction has to roll forward
+    // until the first Read() to decide what to do because it needs to examine
+    // the cache entry. We could conceivably deal with StopCaching() prior to
+    // the first Read(), but we are going to wait till the first Read() since
+    // that allows us to also deal with the case where the requestor calls
+    // StopCaching() after Start() or Read().
+    stopped_caching_ = !!(mode_ & WRITE);

[rvargas (doing something else), 2015/09/11 23:56:18]

nit: I hate !!x; How about x != 0 (if we need to keep this part).

+    DCHECK_IMPLIES(stopped_caching_,
+                   effective_load_flags_ & LOAD_PREFERRING_CACHE);
+  }
+
   if (mode_ == NONE) {
     if (partial_) {
       partial_->RestoreHeaders(&custom_request_->extra_headers);
@@ -1051,6 +1036,17 @@ int HttpCache::Transaction::DoOpenEntryComplete(int result) {
     return OK;
   }
 
+  // No need to create a cache entry if the transaction has already decided not
+  // to write to it.
+  if (stopped_caching_) {

[rvargas (doing something else), 2015/09/11 23:56:18]

same thing about start.

+    // stopped_caching_ should've been unset in DoGetBackendComplete() if mode_
+    // didn't have the WRITE bit set.
+    DCHECK(mode_ & WRITE);
+    mode_ = NONE;
+    next_state_ = STATE_SEND_REQUEST;
+    return OK;
+  }
+
   if (result == ERR_CACHE_RACE) {
     next_state_ = STATE_INIT_ENTRY;
     return OK;
@@ -1069,6 +1065,7 @@ int HttpCache::Transaction::DoOpenEntryComplete(int result) {
     next_state_ = STATE_CREATE_ENTRY;
     return OK;
   }
+
   if (mode_ == UPDATE) {
     // There is no cache entry to update; proceed without caching.
     mode_ = NONE;
@@ -1286,7 +1283,7 @@ int HttpCache::Transaction::DoCacheToggleUnusedSincePrefetch() {
           "422516 HttpCache::Transaction::DoCacheToggleUnusedSincePrefetch"));
 
   next_state_ = STATE_TOGGLE_UNUSED_SINCE_PREFETCH_COMPLETE;
-  return WriteResponseInfoToEntry(false);
+  return WriteResponseInfoToEntry(false, io_callback_);
 }
 
 int HttpCache::Transaction::DoCacheToggleUnusedSincePrefetchComplete(
@@ -1354,14 +1351,8 @@ int HttpCache::Transaction::DoStartPartialCacheValidation() {
 
 int HttpCache::Transaction::DoCompletePartialCacheValidation(int result) {
   if (!result) {
-    // This is the end of the request.
-    if (mode_ & WRITE) {
-      DoneWritingToEntry(true);
-    } else {
-      cache_->DoneReadingFromEntry(entry_, this);
-      entry_ = NULL;
-    }
-    return result;
+    ReleaseCacheEntry(EntryState::SUCCEEDED);
+    return 0;

[rvargas (doing something else), 2015/09/11 23:56:18]

nit: OK (or result).

[rvargas (doing something else), 2015/09/21 22:05:59]

Missed?

   }
 
   if (result < 0)
@@ -1433,7 +1424,7 @@ int HttpCache::Transaction::DoSendRequestComplete(int result) {
     DCHECK(response);
     response_.cert_request_info = response->cert_request_info;
   } else if (response_.was_cached) {
-    DoneWritingToEntry(true);
+    ReleaseCacheEntry(EntryState::SUCCEEDED);
   }
 
   return result;
@@ -1489,11 +1480,27 @@ int HttpCache::Transaction::DoSuccessfulSendRequest() {
     return OK;
   }
 
+  if (handling_206_ && stopped_caching_ && partial_ &&
+      partial_->IsLastRange() && !partial_->IsCurrentRangeCached() &&
+      (mode_ & WRITE) && entry_) {
+    // The current state of the transaction is:
+    // * The client has requested that no addtional data be cached.
+    // * The current network transaction spans the entire remainder of the
+    // requested range.
+    // * A cache entry exists and was being written to or updated.
+    // * The current range is not cached.
+    //
+    // At this point it is safe to abandon the cache entry and proceed with just
+    // the network transaction.
+    next_state_ = STATE_NETWORK_READ;

[rvargas (doing something else), 2015/09/11 23:56:18]

without returning data to the caller?

[asanka, 2015/09/17 21:59:40]

We do. Once AbadonCacheEntry() completes, the SM will progress through
DoNetworkRead(). Or am I misunderstanding the question?

[rvargas (doing something else), 2015/09/19 01:09:34]

The confusing thing is that we may be doing Start() and we should return to the
caller before jumping to Read. I guess stoped_caching_ guarantees that we are
not in Start anymore and instead we are in Read and I'm used to have reading_
expressing that. But if we are beyond Start, we are inside a Read, why are we
taking the decision after sending a new request? Shouldn't it be either at Read
start or after receiving bytes?

+    return AbandonCacheEntry(io_callback_);
+  }
+
   if (handling_206_ && mode_ == READ_WRITE && !truncated_ && !is_sparse_) {
     // We have stored the full entry, but it changed and the server is
     // sending a range. We have to delete the old entry.
     UpdateTransactionPattern(PATTERN_NOT_COVERED);
-    DoneWritingToEntry(false);
+    ReleaseCacheEntry(EntryState::DOOMED);
   }
 
   if (mode_ == WRITE &&
@@ -1508,9 +1515,7 @@ int HttpCache::Transaction::DoSuccessfulSendRequest() {
       int ret = cache_->DoomEntry(cache_key_, NULL);
       DCHECK_EQ(OK, ret);
     }
-    cache_->DoneWritingToEntry(entry_, true);
-    entry_ = NULL;
-    mode_ = NONE;
+    ReleaseCacheEntry(EntryState::SUCCEEDED);
   }
 
   // Invalidate any cached GET with a successful POST.
@@ -1589,7 +1594,7 @@ int HttpCache::Transaction::DoCacheWriteUpdatedResponse() {
           "422516 HttpCache::Transaction::DoCacheWriteUpdatedResponse"));
 
   next_state_ = STATE_CACHE_WRITE_UPDATED_RESPONSE_COMPLETE;
-  return WriteResponseInfoToEntry(false);
+  return WriteResponseInfoToEntry(false, io_callback_);
 }
 
 int HttpCache::Transaction::DoCacheWriteUpdatedResponseComplete(int result) {
@@ -1605,7 +1610,7 @@ int HttpCache::Transaction::DoUpdateCachedResponseComplete(int result) {
     //
     // By closing the cached entry now, we make sure that the 304 rather than
     // the cached 200 response, is what will be returned to the user.
-    DoneWritingToEntry(true);
+    ReleaseCacheEntry(EntryState::SUCCEEDED);
   } else if (entry_ && !handling_206_) {
     DCHECK_EQ(READ_WRITE, mode_);
     if (!partial_ || partial_->IsLastRange()) {
@@ -1644,16 +1649,15 @@ int HttpCache::Transaction::DoOverwriteCachedResponse() {
 
   if (request_->method == "HEAD") {
     // This response is replacing the cached one.
-    DoneWritingToEntry(false);
-    mode_ = NONE;
-    new_response_ = NULL;
+    ReleaseCacheEntry(EntryState::DOOMED);
+    new_response_ = nullptr;
     return OK;
   }
 
   if (handling_206_ && !CanResume(false)) {
     // There is no point in storing this resource because it will never be used.
     // This may change if we support LOAD_ONLY_FROM_CACHE with sparse entries.
-    DoneWritingToEntry(false);
+    ReleaseCacheEntry(EntryState::DOOMED);
     if (partial_)
       partial_->FixResponseHeaders(response_.headers.get(), true);
     next_state_ = STATE_PARTIAL_HEADERS_RECEIVED;
@@ -1671,7 +1675,7 @@ int HttpCache::Transaction::DoCacheWriteResponse() {
           "422516 HttpCache::Transaction::DoCacheWriteResponse"));
 
   next_state_ = STATE_CACHE_WRITE_RESPONSE_COMPLETE;
-  return WriteResponseInfoToEntry(truncated_);
+  return WriteResponseInfoToEntry(truncated_, io_callback_);
 }
 
 int HttpCache::Transaction::DoCacheWriteResponseComplete(int result) {
@@ -1767,7 +1771,82 @@ int HttpCache::Transaction::DoCacheReadMetadataComplete(int result) {
   return OK;
 }
 
+int HttpCache::Transaction::DoSwitchToNetwork() {
+  // Prerequisites of a switch to network:
+  // * A valid cache entry must exist. Otherwise we'd already be exclusively
+  //   dealing with the network.

[rvargas (doing something else), 2015/09/11 23:56:18]

Do we really need this comment (and the next one)?

[asanka, 2015/09/17 21:59:41]

Removed. I suppose those two should be obvious. Although the fact that we only
switch when writing to the cache and not when reading might not be too obvious
to a new reader.

+  DCHECK(entry_);
+  // * Need to be writing to the cache entry. If the transaction was exclusively
+  //   reading from the cache, then the transaction should continue doing so
+  //   without trying to switch to the network.
+  DCHECK(mode_ & WRITE);
+
+  // If the transaction is currently reading from the cache, then it should have
+  // a valid partial state. I.e. only part of the requested resource could be
+  // fulfiled via the cache. If the entire request was being fulfiled via the
+  // cache, the transaction shouldn't be trying to switch to the network at this
+  // point, and mode_ should be READ.
+  DCHECK_IMPLIES(!network_trans_, partial_);
+
+  // The network transaction can be trivially reused if it can promise to
+  // deliver all the bits necessary to fulfil the original request. If so,
+  // abandon the cache entry and continue with a network read.
+  if (network_trans_ && (!partial_ || partial_->IsLastRange())) {
+    next_state_ = STATE_NETWORK_READ;
+    return AbandonCacheEntry(io_callback_);
+  }
+
+  if (request_->method != "GET") {

[rvargas (doing something else), 2015/09/11 23:56:18]

Isn't it easier to check for this in StopCaching?

[asanka, 2015/09/17 21:59:41]

Check moved there. See earlier confusion about whether StopCaching() could be
called before Start().

[rvargas (doing something else), 2015/09/21 22:05:59]

but PS14 still has the if here...

+    // We have a non-GET request that the cache or network can partially fulfil.
+    // In other words, at some point the transaction is going to have to issue a
+    // partial network request to fulfil this non-GET request. Let's just ignore

[rvargas (doing something else), 2015/09/11 23:56:17]

I don't think this ever happens. (partial && !get)

[asanka, 2015/09/17 21:59:40]

Yeah. The condition we check now is:

  (!partial_ || request->method == GET) && (mode_ & WRITE)

.. which checks that:

  * we are writing to the cache.
  * no need to restart the network transaction (!partial_)
    OR
    method is GET and there's a good chance we'll be able to restart the
transaction.

+    // the StopCaching() directive for now.
+    stopped_caching_ = false;
+    next_state_ = network_trans_ ? STATE_NETWORK_READ : STATE_CACHE_READ_DATA;
+    return OK;
+  }
+
+  // The existing network transaction doesn't cover the entire range we wanted.
+  // Let's discard the network transaction and create a new one that will cover
+  // the entire range we need.
+  if (network_trans_)
+    ResetNetworkTransaction();
+
+  if (!partial_->SkipCacheForRemainder()) {
+    // PartialData only refuses to switch to network reads if the entire
+    // resource has been accounted for. There's nothing more to do.
+    // ** Caveat: This assumes that there's a strict agreement between the
+    // server indicated length of the resource and the actual length of the
+    // resource.

[rvargas (doing something else), 2015/09/11 23:56:18]

This is a pre-requisite for byte range requests

[asanka, 2015/09/17 21:59:41]

Caveat removed.

+    return OK;
+  }
+
+  // The new network request that's going to be issued has to be validated
+  // against the existing cache entry. By extension, this also validates the new
+  // request against the portion that has already been sent to our client.
+  partial_->PrepareCacheValidation(entry_->disk_entry,
+                                   &custom_request_->extra_headers);
+
+  // Since we called SkipCacheForRemainder(), the partial_ state should now
+  // assume that the remainder of the resource is not cached.
+  DCHECK(!partial_->IsCurrentRangeCached());
+  DCHECK(partial_->IsLastRange());
+
+  // READ shouldn't reach here at all. WRITE/UPDATE require a network_trans_
+  // that spans the entire request range and is handled at the top of this
+  // function. At this point we should be left with just READ_WRITE.
+  DCHECK_EQ(mode_, READ_WRITE);
+
+  // Start validating the cache. DoSuccessfulSendRequest() will now bear the
+  // responsibility of abandoning the cache entry if the new network transaction
+  // is successfully validated.
+  return BeginCacheValidation();
+}
+
 int HttpCache::Transaction::DoNetworkRead() {
+  DCHECK_GT(read_buf_len_, 0);
+
+  io_buf_len_ = read_buf_len_;
   next_state_ = STATE_NETWORK_READ_COMPLETE;
   return network_trans_->Read(read_buf_.get(), io_buf_len_, io_callback_);
 }
@@ -1788,11 +1867,14 @@ int HttpCache::Transaction::DoNetworkReadComplete(int result) {
 }
 
 int HttpCache::Transaction::DoCacheReadData() {
+  DCHECK_GT(read_buf_len_, 0);
+
   if (request_->method == "HEAD")
     return 0;
 
   DCHECK(entry_);
   next_state_ = STATE_CACHE_READ_DATA_COMPLETE;
+  io_buf_len_ = read_buf_len_;
 
   if (net_log_.IsCapturing())
     net_log_.BeginEvent(NetLog::TYPE_HTTP_CACHE_READ_DATA);
@@ -1822,15 +1904,13 @@ int HttpCache::Transaction::DoCacheReadDataComplete(int result) {
     return DoPartialCacheReadCompleted(result);
   }
 
-  if (result > 0) {
-    read_offset_ += result;
-  } else if (result == 0) {  // End of file.
-    RecordHistograms();
-    cache_->DoneReadingFromEntry(entry_, this);
-    entry_ = NULL;
-  } else {
+  if (result < 0)
     return OnCacheReadError(result, false);
-  }
+
+  if (result == 0)
+    ReleaseCacheEntry(EntryState::SUCCEEDED);
+
+  read_offset_ += result;
   return result;
 }
 
@@ -1851,22 +1931,27 @@ int HttpCache::Transaction::DoCacheWriteData(int num_bytes) {
 }
 
 int HttpCache::Transaction::DoCacheWriteDataComplete(int result) {
-  if (entry_) {
-    if (net_log_.IsCapturing()) {
-      net_log_.EndEventWithNetErrorCode(NetLog::TYPE_HTTP_CACHE_WRITE_DATA,
-                                        result);
-    }
+  if (entry_ && net_log_.IsCapturing()) {
+    net_log_.EndEventWithNetErrorCode(NetLog::TYPE_HTTP_CACHE_WRITE_DATA,
+                                      result);
   }
+
   if (!cache_.get())
     return ERR_UNEXPECTED;
 
   if (result != write_len_) {
     DLOG(ERROR) << "failed to write response data to cache";
-    DoneWritingToEntry(false);
+    ReleaseCacheEntry(EntryState::DOOMED);
 
     // We want to ignore errors writing to disk and just keep reading from
     // the network.
     result = write_len_;
+
+    // TODO(asanka): More needs to be done to ignore the error and keep reading
+    // from the network. E.g. if the cache entry was sparse, it is possible that
+    // the current network_trans_ can't fulfil the entire requested range.
+    // Consider setting up state to perform an immediate STATE_SWITCH_TO_NETWORK
+    // at the next Read() instead of releasing the entry now.
   } else if (!done_reading_ && entry_) {
     int current_size = entry_->disk_entry->GetDataSize(kResponseContentIndex);
     int64 body_size = response_.headers->GetContentLength();
@@ -1875,34 +1960,55 @@ int HttpCache::Transaction::DoCacheWriteDataComplete(int result) {
   }
 
   if (partial_) {
-    // This may be the last request.
-    if (result != 0 || truncated_ ||
-        !(partial_->IsLastRange() || mode_ == WRITE)) {
+    // The transaction needs to update partial state under the following
+    // circumstances:
+    const bool need_to_update_partial_state =
+        // Still reading. The partial state needs to be updated to account for
+        // the data read so far.
+        result != 0 ||
+
+        // Reached the end of one network request, but more transactions may be
+        // necessary to fulfil the request. DoPartialNetworkReadCompleted()
+        // initiates additional network transactions as needed.
+        truncated_ ||
+
+        // If this isn't the last range, then more network transactions may be
+        // necessary. But skip this step for new cache entries (mode_ == WRITE).
+        // Range requests with new cache entries will have
+        // partial_->IsLastRange() set to false. IsLastRange() is only set to
+        // true when validating the last range of an existing cache entry.
+        (!partial_->IsLastRange() && mode_ != WRITE);
+
+    if (need_to_update_partial_state)
       return DoPartialNetworkReadCompleted(result);
-    }
   }
 
   if (result == 0) {
-    // End of file. This may be the result of a connection problem so see if we
-    // have to keep the entry around to be flagged as truncated later on.
-    if (done_reading_ || !entry_ || partial_ ||
-        response_.headers->GetContentLength() <= 0) {
-      DoneWritingToEntry(true);
+    // If the entire response body was successfully read, then the transaction
+    // should release the cache entry.
+    //
+    // While response == 0 typically indicates EOF, it is possible for that to
+    // happen due to a connection problem. In the latter case the transaction
+    // should retain ownership of entry_ until the transaction is destroyed so

[rvargas (doing something else), 2015/09/11 23:56:17]

But now AbandonCacheEntry is called from here.

[asanka, 2015/09/17 21:59:40]

Comment updated.

+    // that the resulting AbandonCacheEntry() call would cause the entry to be
+    // marked as truncated.
+    //
+    // A succesful receipt of the response body is considered to be indicated by
+    // one of:
+    //  * done_reading_ : Received response body was of expected size.
+    //  * Content-Length <= 0 : Response body size was not known.
+    //  * partial_ :
+    if (entry_ && (done_reading_ || partial_ ||
+                   response_.headers->GetContentLength() <= 0)) {
+      ReleaseCacheEntry(EntryState::SUCCEEDED);
+    } else {
+      AbandonCacheEntry(CompletionCallback());

[rvargas (doing something else), 2015/09/11 23:56:18]

This is not doing the same checks that the dtor performs.

[asanka, 2015/09/17 21:59:40]

Different circumstances.
When we get here, the network transaction is done reading and all data from the
network was written to disk. Hence the condition is trying to determine how
confident we are that the end of the network stream legitimately signalled the
end of the resource.

In the destructor, the state of the network stream isn't known. So the
destructor tries to determine whether the transaction received a legitimate
response at all. If so it attempts to abandon the entry instead of marking it as
DOOMed.

[rvargas (doing something else), 2015/09/21 22:05:59]

Poor wording on my part. The destructor ends up looking at
  reading_ && response_.headers.get() &&
  (response_.headers->response_code() == 200 || partial_)

plus what AddTruncatedFlag looks at:
  partial_, CanResume() and done_reading_

The conditions for AddTruncatedFlag moved to AbandonCacheEntry, and from the
dtor itself reading_ && response_.headers.get() are implied because we are here.
That leaves 200 || partial_ (and partial_ is handled by AddTruncatedFlag).

So the only thing that seems missing is 200. Maybe it doesn't matter? (the 200
is a weak test to only do work for "normal" requests). It looks like we still
need it.

     }
   }
 
   return result;
 }
 
-int HttpCache::Transaction::DoCacheWriteTruncatedResponse() {
-  next_state_ = STATE_CACHE_WRITE_TRUNCATED_RESPONSE_COMPLETE;
-  return WriteResponseInfoToEntry(true);
-}
-
-int HttpCache::Transaction::DoCacheWriteTruncatedResponseComplete(int result) {
-  return OnWriteResponseInfoToEntryComplete(result);
-}
-
 //-----------------------------------------------------------------------------
 
 void HttpCache::Transaction::ReadCertChain() {
@@ -2160,6 +2266,11 @@ int HttpCache::Transaction::BeginCacheValidation() {
     // know we won't be falling back to using the cache entry in the
     // LOAD_FROM_CACHE_IF_OFFLINE case.
     if (!ConditionalizeRequest()) {
+      // Conditionalization should only depend on the state of the cache prior
+      // to starting the request and the request method. Failure is unexpected
+      // after the client has started reading the stream.
+      DCHECK(!reading_);
+
       couldnt_conditionalize_request_ = true;
       UpdateTransactionPattern(PATTERN_ENTRY_CANT_CONDITIONALIZE);
       if (partial_)
@@ -2238,7 +2349,7 @@ int HttpCache::Transaction::BeginExternallyConditionalizedRequest() {
       // The externally conditionalized request is not a validation request
       // for our existing cache entry. Proceed with caching disabled.
       UpdateTransactionPattern(PATTERN_NOT_COVERED);
-      DoneWritingToEntry(true);
+      ReleaseCacheEntry(EntryState::SUCCEEDED);
     }
   }
 
@@ -2566,14 +2677,9 @@ void HttpCache::Transaction::IgnoreRangeRequest() {
   // using the cache and see what happens. Most likely this is the first
   // response from the server (it's not changing its mind midway, right?).
   UpdateTransactionPattern(PATTERN_NOT_COVERED);
-  if (mode_ & WRITE)
-    DoneWritingToEntry(mode_ != WRITE);
-  else if (mode_ & READ && entry_)
-    cache_->DoneReadingFromEntry(entry_, this);
-
-  partial_.reset(NULL);
-  entry_ = NULL;
-  mode_ = NONE;
+  ReleaseCacheEntry(mode_ == WRITE ? EntryState::DOOMED

[rvargas (doing something else), 2015/09/11 23:56:18]

wasn't the argument irrelevant for readers? In which case this can pass doomed
all the time.

[asanka, 2015/09/17 21:59:41]

mode_ == READ_WRITE also keeps the entry.

[rvargas (doing something else), 2015/09/21 22:05:59]

Acknowledged.

+                                   : EntryState::SUCCEEDED);
+  partial_.reset();
 }
 
 void HttpCache::Transaction::FixHeadersForHead() {
@@ -2623,7 +2729,9 @@ int HttpCache::Transaction::WriteToEntry(int index, int offset,
   return rv;
 }
 
-int HttpCache::Transaction::WriteResponseInfoToEntry(bool truncated) {
+int HttpCache::Transaction::WriteResponseInfoToEntry(
+    bool truncated,
+    const CompletionCallback& callback) {
   if (!entry_)
     return OK;
 
@@ -2641,7 +2749,7 @@ int HttpCache::Transaction::WriteResponseInfoToEntry(bool truncated) {
   // status to a net error and replay the net error.
   if ((response_.headers->HasHeaderValue("cache-control", "no-store")) ||
       IsCertStatusError(response_.ssl_info.cert_status)) {
-    DoneWritingToEntry(false);
+    ReleaseCacheEntry(EntryState::DOOMED);
     if (net_log_.IsCapturing())
       net_log_.EndEvent(NetLog::TYPE_HTTP_CACHE_WRITE_INFO);
     return OK;
@@ -2662,7 +2770,7 @@ int HttpCache::Transaction::WriteResponseInfoToEntry(bool truncated) {
 
   io_buf_len_ = data->pickle()->size();
   return entry_->disk_entry->WriteData(kResponseInfoIndex, 0, data.get(),
-                                       io_buf_len_, io_callback_, true);
+                                       io_buf_len_, callback, true);
 }
 
 int HttpCache::Transaction::OnWriteResponseInfoToEntryComplete(int result) {
@@ -2675,20 +2783,20 @@ int HttpCache::Transaction::OnWriteResponseInfoToEntryComplete(int result) {
 
   if (result != io_buf_len_) {
     DLOG(ERROR) << "failed to write response info to cache";
-    DoneWritingToEntry(false);
+    ReleaseCacheEntry(EntryState::DOOMED);
   }
   return OK;
 }
 
-void HttpCache::Transaction::DoneWritingToEntry(bool success) {
-  if (!entry_)
+void HttpCache::Transaction::ReleaseCacheEntry(EntryState entry_state) {
+  if (!cache_ || !entry_)
     return;
 
-  RecordHistograms();
-
-  cache_->DoneWritingToEntry(entry_, success);
-  entry_ = NULL;
-  mode_ = NONE;  // switch to 'pass through' mode
+  cache_->DoneWithEntry(entry_, this, entry_state);
+  entry_ = nullptr;
+  stopped_caching_ = false;
+  mode_ = NONE;
+  return;

[rvargas (doing something else), 2015/09/11 23:56:18]

remove

[asanka, 2015/09/17 21:59:41]

Done.

 }
 
 int HttpCache::Transaction::OnCacheReadError(int result, bool restart) {
@@ -2709,8 +2817,7 @@ int HttpCache::Transaction::OnCacheReadError(int result, bool restart) {
   if (restart) {
     DCHECK(!reading_);
     DCHECK(!network_trans_.get());
-    cache_->DoneWithEntry(entry_, this, false);
-    entry_ = NULL;
+    ReleaseCacheEntry(EntryState::DOOMED);
     is_sparse_ = false;
     partial_.reset();
     next_state_ = STATE_GET_BACKEND;
@@ -2735,14 +2842,11 @@ void HttpCache::Transaction::OnAddToEntryTimeout(base::TimeTicks start_time) {
 
 void HttpCache::Transaction::DoomPartialEntry(bool delete_object) {
   DVLOG(2) << "DoomPartialEntry";
-  int rv = cache_->DoomEntry(cache_key_, NULL);
-  DCHECK_EQ(OK, rv);
-  cache_->DoneWithEntry(entry_, this, false);
-  entry_ = NULL;
+  ReleaseCacheEntry(EntryState::DOOMED);
   is_sparse_ = false;
   truncated_ = false;
   if (delete_object)
-    partial_.reset(NULL);
+    partial_.reset();
 }
 
 int HttpCache::Transaction::DoPartialNetworkReadCompleted(int result) {
@@ -2774,8 +2878,8 @@ int HttpCache::Transaction::DoRestartPartialRequest() {
 
   // WRITE + Doom + STATE_INIT_ENTRY == STATE_CREATE_ENTRY (without an attempt
   // to Doom the entry again).
-  mode_ = WRITE;
   ResetPartialState(!range_requested_);
+  mode_ = WRITE;
   next_state_ = STATE_CREATE_ENTRY;
   return OK;
 }
@@ -2850,14 +2954,14 @@ void HttpCache::Transaction::UpdateTransactionPattern(
 }
 
 void HttpCache::Transaction::RecordHistograms() {
-  DCHECK_NE(PATTERN_UNDEFINED, transaction_pattern_);
-  if (!cache_.get() || !cache_->GetCurrentBackend() ||
+  if (transaction_pattern_ == PATTERN_UNDEFINED || !cache_.get() ||
+      !cache_->GetCurrentBackend() ||
       cache_->GetCurrentBackend()->GetCacheType() != DISK_CACHE ||
       cache_->mode() != NORMAL || request_->method != "GET") {
     return;
   }
-  UMA_HISTOGRAM_ENUMERATION(
-      "HttpCache.Pattern", transaction_pattern_, PATTERN_MAX);
+  UMA_HISTOGRAM_ENUMERATION("HttpCache.Pattern", transaction_pattern_,
+                            PATTERN_MAX);
   if (transaction_pattern_ == PATTERN_NOT_COVERED)
     return;
   DCHECK(!range_requested_);
@@ -2931,4 +3035,41 @@ void HttpCache::Transaction::OnIOComplete(int result) {
   DoLoop(result);
 }
 
+int HttpCache::Transaction::AbandonCacheEntry(
+    const CompletionCallback& callback) {
+  if (is_sparse_ || done_reading_ || !(mode_ & WRITE)) {

[rvargas (doing something else), 2015/09/11 23:56:18]

is_sparse != partial_ && !truncated_ (which is what AddTruncatedFlag looks at).

[asanka, 2015/09/17 21:59:40]

Condition updated with explanation.

+    ReleaseCacheEntry(EntryState::SUCCEEDED);
+    return OK;
+  }
+  if (!reading_ || !CanResume(true)) {
+    ReleaseCacheEntry(EntryState::DOOMED);
+    return OK;
+  }
+  if (stopped_caching_ && ((mode_ == WRITE && !truncated_) ||

[rvargas (doing something else), 2015/09/11 23:56:18]

why write vs rw? why write && truncated_ is ok?

[asanka, 2015/09/17 21:59:41]

Removed this condition block. This "if" is trying to decide whether
StopCaching() was called on a truncated entry. Incidentally (WRITE && !trucated)
is taken to mean that this was a new cache entry which was stopped halfway. In
the absence of the condition we'd mark it as truncated, which is what we are
doing with the latest patchset.

+                           (mode_ == READ_WRITE && truncated_))) {
+    ReleaseCacheEntry(EntryState::DOOMED);

[rvargas (doing something else), 2015/09/11 23:56:18]

This deserves a comment (what we're after with the preceding condition).

[asanka, 2015/09/17 21:59:41]

(removed)

+    return OK;
+  }
+
+  truncated_ = true;
+  int result = WriteResponseInfoToEntry(
+      true, base::Bind(&HttpCache::Transaction::OnAbandonCacheEntryIOComplete,
+                       weak_factory_.GetWeakPtr()));
+  if (result == ERR_IO_PENDING) {
+    if (!callback.is_null()) {

[rvargas (doing something else), 2015/09/11 23:56:17]

nit: invert the logic

[asanka, 2015/09/17 21:59:41]

Done.

+      abandon_cache_entry_callback_ = callback;
+    } else {
+      ReleaseCacheEntry(EntryState::SUCCEEDED);
+    }
+  }
+  return result;
+}
+
+void HttpCache::Transaction::OnAbandonCacheEntryIOComplete(int result) {
+  OnWriteResponseInfoToEntryComplete(result);
+  ReleaseCacheEntry(EntryState::SUCCEEDED);
+  if (!abandon_cache_entry_callback_.is_null())
+    base::ResetAndReturn(&abandon_cache_entry_callback_).Run(result);
+}
+
 }  // namespace net