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

net/http/http_cache_transaction.cc

 // Copyright (c) 2012 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/http/http_cache_transaction.h"
 
 #include "build/build_config.h"  // For OS_POSIX
 
 #if defined(OS_POSIX)
 #include <unistd.h>
@@ skipping 248 matching lines @@
       reading_(false),
       invalid_range_(false),
       truncated_(false),
       is_sparse_(false),
       range_requested_(false),
       handling_206_(false),
       cache_pending_(false),
       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),
       write_len_(0),
       transaction_pattern_(PATTERN_UNDEFINED),
       total_received_bytes_(0),
       websocket_handshake_stream_base_create_helper_(NULL),
       weak_factory_(this) {
   static_assert(HttpCache::Transaction::kNumValidationHeaders ==
                     arraysize(kValidationHeaders),
                 "invalid number of validation headers");
 
   io_callback_ = base::Bind(&Transaction::OnIOComplete,
                               weak_factory_.GetWeakPtr());
 }
 
 HttpCache::Transaction::~Transaction() {
   // We may have to issue another IO, but we should never invoke the callback_
   // 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();
 
-      cache_->DoneWithEntry(entry_, this, cancel_request);
-    } else if (cache_pending_) {
+  if (!cache_)
+    return;
+
+  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,
                                           const CompletionCallback& callback) {
   DCHECK(buf);
   DCHECK_GT(buf_len, 0);
   DCHECK(!callback.is_null());
   if (!cache_.get() || !entry_)
     return ERR_UNEXPECTED;
 
   // We don't need to track this operation for anything.
   // It could be possible to check if there is something already written and
   // avoid writing again (it should be the same, right?), but let's allow the
   // caller to "update" the contents with something new.
   return entry_->disk_entry->WriteData(kMetadataIndex, 0, buf, 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();
   if (entry_ || !request_)
     return LOAD_STATE_IDLE;
   return LOAD_STATE_WAITING_FOR_CACHE;
 }
 
 const BoundNetLog& HttpCache::Transaction::net_log() const {
   return net_log_;
@@ skipping 94 matching lines @@
     return false;
   return network_trans_->IsReadyToRestartForAuth();
 }
 
 int HttpCache::Transaction::Read(IOBuffer* buf, int buf_len,
                                  const CompletionCallback& callback) {
   DCHECK_EQ(next_state_, STATE_NONE);
   DCHECK(buf);
   DCHECK_GT(buf_len, 0);
   DCHECK(!callback.is_null());
-
   DCHECK(callback_.is_null());
 
   if (!cache_.get())
     return ERR_UNEXPECTED;
 
   // If we have an intermediate auth response at this point, then it means the
   // user wishes to read the network response (the error page).  If there is a
   // previous response in the cache then we should leave it intact.
   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;
   } else {
     DCHECK(mode_ == READ || (mode_ == READ_WRITE && partial_));
     next_state_ = STATE_CACHE_READ_DATA;
   }
 
   int rv = DoLoop(OK);
 
   if (rv == ERR_IO_PENDING) {
     DCHECK(callback_.is_null());
     callback_ = callback;
   }
   return rv;
 }
 
 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(
     HttpRequestHeaders* headers) const {
   if (network_trans_)
     return network_trans_->GetFullRequestHeaders(headers);
 
   // TODO(ttuttle): Read headers from cache.
   return false;
 }
 
 int64 HttpCache::Transaction::GetTotalReceivedBytes() const {
   int64 total_received_bytes = total_received_bytes_;
   if (network_trans_)
     total_received_bytes += network_trans_->GetTotalReceivedBytes();
   return total_received_bytes;
 }
 
 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 {
   // Null headers means we encountered an error or haven't a response yet
   if (auth_response_.headers.get())
     return &auth_response_;
   return &response_;
 }
 
 LoadState HttpCache::Transaction::GetLoadState() const {
@@ skipping 341 matching lines @@
         DCHECK_EQ(OK, rv);
         rv = DoPartialHeadersReceived();
         break;
       case STATE_CACHE_READ_METADATA:
         DCHECK_EQ(OK, rv);
         rv = DoCacheReadMetadata();
         break;
       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();
         break;
       case STATE_NETWORK_READ_COMPLETE:
         rv = DoNetworkReadComplete(rv);
         break;
       case STATE_CACHE_READ_DATA:
         DCHECK_EQ(OK, rv);
         rv = DoCacheReadData();
         break;
       case STATE_CACHE_READ_DATA_COMPLETE:
         rv = DoCacheReadDataComplete(rv);
         break;
       case STATE_CACHE_WRITE_DATA:
         rv = DoCacheWriteData(rv);
         break;
       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;
         break;
     }
   } 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);
   }
 
   return rv;
 }
 
 int HttpCache::Transaction::DoGetBackend() {
   cache_pending_ = true;
   next_state_ = STATE_GET_BACKEND_COMPLETE;
   net_log_.BeginEvent(NetLog::TYPE_HTTP_CACHE_GET_BACKEND);
@@ skipping 42 matching lines @@
   // the cache. If we receive 200, it doesn't matter if there was a validation
   // header or not.
   if (request_->method == "HEAD" && mode_ == WRITE)
     mode_ = NONE;
 
   // If must use cache, then we must fail.  This can happen for back/forward
   // navigations to a page generated via a form post.
   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);
       partial_.reset();
     }
     next_state_ = STATE_SEND_REQUEST;
   } else {
     next_state_ = STATE_INIT_ENTRY;
   }
 
@@ skipping 31 matching lines @@
   // It is important that we go to STATE_ADD_TO_ENTRY whenever the result is
   // OK, otherwise the cache will end up with an active entry without any
   // transaction attached.
   net_log_.EndEventWithNetErrorCode(NetLog::TYPE_HTTP_CACHE_OPEN_ENTRY, result);
   cache_pending_ = false;
   if (result == OK) {
     next_state_ = STATE_ADD_TO_ENTRY;
     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;
   }
 
   if (request_->method == "PUT" || request_->method == "DELETE" ||
       (request_->method == "HEAD" && mode_ == READ_WRITE)) {
     DCHECK(mode_ == READ_WRITE || mode_ == WRITE || request_->method == "HEAD");
     mode_ = NONE;
     next_state_ = STATE_SEND_REQUEST;
     return OK;
   }
 
   if (mode_ == READ_WRITE) {
     mode_ = WRITE;
     next_state_ = STATE_CREATE_ENTRY;
     return OK;
   }
+
   if (mode_ == UPDATE) {
     // There is no cache entry to update; proceed without caching.
     mode_ = NONE;
     next_state_ = STATE_SEND_REQUEST;
     return OK;
   }
 
   // The entry does not exist, and we are not permitted to create a new entry,
   // so we must fail.
   return ERR_CACHE_MISS;
@@ skipping 197 matching lines @@
   // TODO(jkarlin): If DoUpdateCachedResponse is also called for this
   // transaction then metadata will be written to cache twice. If prefetching
   // becomes more common, consider combining the writes.
 
   // TODO(rtenneti): Remove ScopedTracker below once crbug.com/422516 is fixed.
   tracked_objects::ScopedTracker tracking_profile(
       FROM_HERE_WITH_EXPLICIT_FUNCTION(
           "422516 HttpCache::Transaction::DoCacheToggleUnusedSincePrefetch"));
 
   next_state_ = STATE_TOGGLE_UNUSED_SINCE_PREFETCH_COMPLETE;
-  return WriteResponseInfoToEntry(false);
+  return WriteResponseInfoToEntry(false, io_callback_);
 }
 
 int HttpCache::Transaction::DoCacheToggleUnusedSincePrefetchComplete(
     int result) {
   // Restore the original value for this transaction.
   response_.unused_since_prefetch = !response_.unused_since_prefetch;
   next_state_ = STATE_CACHE_DISPATCH_VALIDATION;
   return OnWriteResponseInfoToEntryComplete(result);
 }
 
@@ skipping 47 matching lines @@
 int HttpCache::Transaction::DoStartPartialCacheValidation() {
   if (mode_ == NONE)
     return OK;
 
   next_state_ = STATE_COMPLETE_PARTIAL_CACHE_VALIDATION;
   return partial_->ShouldValidateCache(entry_->disk_entry, io_callback_);
 }
 
 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)
     return result;
 
   partial_->PrepareCacheValidation(entry_->disk_entry,
                                    &custom_request_->extra_headers);
 
   if (reading_ && partial_->IsCurrentRangeCached()) {
     next_state_ = STATE_CACHE_READ_DATA;
@@ skipping 51 matching lines @@
   UpdateTransactionPattern(PATTERN_NOT_COVERED);
   if (IsCertificateError(result)) {
     // If we get a certificate error, then there is a certificate in ssl_info,
     // so GetResponseInfo() should never return NULL here.
     DCHECK(response);
     response_.ssl_info = response->ssl_info;
   } else if (result == ERR_SSL_CLIENT_AUTH_CERT_NEEDED) {
     DCHECK(response);
     response_.cert_request_info = response->cert_request_info;
   } else if (response_.was_cached) {
-    DoneWritingToEntry(true);
+    ReleaseCacheEntry(EntryState::SUCCEEDED);
   }
 
   return result;
 }
 
 // We received the response headers and there is no error.
 int HttpCache::Transaction::DoSuccessfulSendRequest() {
   DCHECK(!new_response_);
   const HttpResponseInfo* new_response = network_trans_->GetResponseInfo();
 
@@ skipping 35 matching lines @@
     // the new response.
     net_log_.AddEvent(NetLog::TYPE_HTTP_CACHE_RE_SEND_PARTIAL_REQUEST);
     UpdateTransactionPattern(PATTERN_NOT_COVERED);
     response_ = HttpResponseInfo();
     ResetNetworkTransaction();
     new_response_ = NULL;
     next_state_ = STATE_SEND_REQUEST;
     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 &&
       transaction_pattern_ != PATTERN_ENTRY_CANT_CONDITIONALIZE) {
     UpdateTransactionPattern(PATTERN_ENTRY_NOT_CACHED);
   }
 
   // Invalidate any cached GET with a successful PUT or DELETE.
   if (mode_ == WRITE &&
       (request_->method == "PUT" || request_->method == "DELETE")) {
     if (NonErrorResponse(new_response->headers->response_code())) {
       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.
   if (!(effective_load_flags_ & LOAD_DISABLE_CACHE) &&
       request_->method == "POST" &&
       NonErrorResponse(new_response->headers->response_code())) {
     cache_->DoomMainEntryForUrl(request_->url);
   }
 
   RecordNoStoreHeaderHistogram(request_->load_flags, new_response);
@@ skipping 58 matching lines @@
   return rv;
 }
 
 int HttpCache::Transaction::DoCacheWriteUpdatedResponse() {
   // TODO(rtenneti): Remove ScopedTracker below once crbug.com/422516 is fixed.
   tracked_objects::ScopedTracker tracking_profile(
       FROM_HERE_WITH_EXPLICIT_FUNCTION(
           "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) {
   next_state_ = STATE_UPDATE_CACHED_RESPONSE_COMPLETE;
   return OnWriteResponseInfoToEntryComplete(result);
 }
 
 int HttpCache::Transaction::DoUpdateCachedResponseComplete(int result) {
   if (mode_ == UPDATE) {
     DCHECK(!handling_206_);
     // We got a "not modified" response and already updated the corresponding
     // cache entry above.
     //
     // 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()) {
       cache_->ConvertWriterToReader(entry_);
       mode_ = READ;
     }
     // We no longer need the network transaction, so destroy it.
     final_upload_progress_ = network_trans_->GetUploadProgress();
     ResetNetworkTransaction();
   } else if (entry_ && handling_206_ && truncated_ &&
@@ skipping 18 matching lines @@
   }
 
   // We change the value of Content-Length for partial content.
   if (handling_206_ && partial_)
     partial_->FixContentLength(new_response_->headers.get());
 
   response_ = *new_response_;
 
   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;
     return OK;
   }
 
   next_state_ = STATE_CACHE_WRITE_RESPONSE;
   return OK;
 }
 
 int HttpCache::Transaction::DoCacheWriteResponse() {
   // TODO(rtenneti): Remove ScopedTracker below once crbug.com/422516 is fixed.
   tracked_objects::ScopedTracker tracking_profile(
       FROM_HERE_WITH_EXPLICIT_FUNCTION(
           "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) {
   next_state_ = STATE_TRUNCATE_CACHED_DATA;
   return OnWriteResponseInfoToEntryComplete(result);
 }
 
 int HttpCache::Transaction::DoTruncateCachedData() {
   next_state_ = STATE_TRUNCATE_CACHED_DATA_COMPLETE;
   if (!entry_)
@@ skipping 75 matching lines @@
                                       io_callback_);
 }
 
 int HttpCache::Transaction::DoCacheReadMetadataComplete(int result) {
   net_log_.EndEventWithNetErrorCode(NetLog::TYPE_HTTP_CACHE_READ_INFO, result);
   if (result != response_.metadata->size())
     return OnCacheReadError(result, false);
   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_);
 }
 
 int HttpCache::Transaction::DoNetworkReadComplete(int result) {
   DCHECK(mode_ & WRITE || mode_ == NONE);
 
   if (!cache_.get())
     return ERR_UNEXPECTED;
 
   // If there is an error or we aren't saving the data, we are done; just wait
   // until the destructor runs to see if we can keep the data.
   if (mode_ == NONE || result < 0)
     return result;
 
   next_state_ = STATE_CACHE_WRITE_DATA;
   return 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);
   if (partial_) {
     return partial_->CacheRead(entry_->disk_entry, read_buf_.get(), io_buf_len_,
                                io_callback_);
   }
 
   return entry_->disk_entry->ReadData(kResponseContentIndex, read_offset_,
                                       read_buf_.get(), io_buf_len_,
                                       io_callback_);
 }
 
 int HttpCache::Transaction::DoCacheReadDataComplete(int result) {
   if (net_log_.IsCapturing()) {
     net_log_.EndEventWithNetErrorCode(NetLog::TYPE_HTTP_CACHE_READ_DATA,
                                       result);
   }
 
   if (!cache_.get())
     return ERR_UNEXPECTED;
 
   if (partial_) {
     // Partial requests are confusing to report in histograms because they may
     // have multiple underlying requests.
     UpdateTransactionPattern(PATTERN_NOT_COVERED);
     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;
 }
 
 int HttpCache::Transaction::DoCacheWriteData(int num_bytes) {
   next_state_ = STATE_CACHE_WRITE_DATA_COMPLETE;
   write_len_ = num_bytes;
   if (entry_) {
     if (net_log_.IsCapturing())
       net_log_.BeginEvent(NetLog::TYPE_HTTP_CACHE_WRITE_DATA);
   }
 
   if (!entry_ || !num_bytes)
     return num_bytes;
 
   int current_size = entry_->disk_entry->GetDataSize(kResponseContentIndex);
   return WriteToEntry(kResponseContentIndex, current_size, read_buf_.get(),
                       num_bytes, io_callback_);
 }
 
 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();
     if (body_size >= 0 && body_size <= current_size)
       done_reading_ = true;
   }
 
   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() {
   std::string key =
       GetCacheKeyForCert(response_.ssl_info.cert->os_cert_handle());
   const X509Certificate::OSCertHandles& intermediates =
       response_.ssl_info.cert->GetIntermediateCertificates();
   int dist_from_root = intermediates.size();
 
   scoped_refptr<SharedChainData> shared_chain_data(
@@ skipping 237 matching lines @@
     UpdateTransactionPattern(PATTERN_ENTRY_USED);
     return SetupEntryForRead();
   } else {
     // Make the network request conditional, to see if we may reuse our cached
     // response.  If we cannot do so, then we just resort to a normal fetch.
     // Our mode remains READ_WRITE for a conditional request.  Even if the
     // conditionalization fails, we don't switch to WRITE mode until we
     // 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_)
         return DoRestartPartialRequest();
 
       DCHECK_NE(206, response_.headers->response_code());
     }
     next_state_ = STATE_SEND_REQUEST;
   }
   return OK;
@@ skipping 58 matching lines @@
     response_.headers->EnumerateHeader(
         NULL,
         kValidationHeaders[i].related_response_header_name,
         &validator);
 
     if (response_.headers->response_code() != 200 || truncated_ ||
         validator.empty() || validator != external_validation_.values[i]) {
       // 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);
     }
   }
 
   // TODO(ricea): This calculation is expensive to perform just to collect
   // statistics. Either remove it or use the result, depending on the result of
   // the experiment.
   ExternallyConditionalizedType type =
       EXTERNALLY_CONDITIONALIZED_CACHE_USABLE;
   if (mode_ == NONE)
     type = EXTERNALLY_CONDITIONALIZED_MISMATCHED_VALIDATORS;
@@ skipping 307 matching lines @@
   IgnoreRangeRequest();
   return true;
 }
 
 void HttpCache::Transaction::IgnoreRangeRequest() {
   // We have a problem. We may or may not be reading already (in which case we
   // returned the headers), but we'll just pretend that this request is not
   // 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() {
   if (response_.headers->response_code() == 206) {
     response_.headers->RemoveHeader("Content-Range");
     response_.headers->ReplaceStatusLine("HTTP/1.1 200 OK");
   }
 }
 
 int HttpCache::Transaction::SetupEntryForRead() {
@@ skipping 29 matching lines @@
   int rv = 0;
   if (!partial_ || !data_len) {
     rv = entry_->disk_entry->WriteData(index, offset, data, data_len, callback,
                                        true);
   } else {
     rv = partial_->CacheWrite(entry_->disk_entry, data, data_len, callback);
   }
   return rv;
 }
 
-int HttpCache::Transaction::WriteResponseInfoToEntry(bool truncated) {
+int HttpCache::Transaction::WriteResponseInfoToEntry(
+    bool truncated,
+    const CompletionCallback& callback) {
   if (!entry_)
     return OK;
 
   if (net_log_.IsCapturing())
     net_log_.BeginEvent(NetLog::TYPE_HTTP_CACHE_WRITE_INFO);
 
   // Do not cache no-store content.  Do not cache content with cert errors
   // either.  This is to prevent not reporting net errors when loading a
   // resource from the cache.  When we load a page over HTTPS with a cert error
   // we show an SSL blocking page.  If the user clicks proceed we reload the
   // resource ignoring the errors.  The loaded resource is then cached.  If that
   // resource is subsequently loaded from the cache, no net error is reported
   // (even though the cert status contains the actual errors) and no SSL
   // blocking page is shown.  An alternative would be to reverse-map the cert
   // 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;
   }
 
   // cert_cache() will be null if the CertCacheTrial field trial is disabled.
   if (cache_->cert_cache() && response_.ssl_info.is_valid())
     WriteCertChain();
 
   if (truncated)
     DCHECK_EQ(200, response_.headers->response_code());
 
   // When writing headers, we normally only write the non-transient headers.
   bool skip_transient_headers = true;
   scoped_refptr<PickledIOBuffer> data(new PickledIOBuffer());
   response_.Persist(data->pickle(), skip_transient_headers, truncated);
   data->Done();
 
   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) {
   if (!entry_)
     return OK;
   if (net_log_.IsCapturing()) {
     net_log_.EndEventWithNetErrorCode(NetLog::TYPE_HTTP_CACHE_WRITE_INFO,
                                       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) {
   DLOG(ERROR) << "ReadData failed: " << result;
   const int result_for_histogram = std::max(0, -result);
   if (restart) {
     UMA_HISTOGRAM_SPARSE_SLOWLY("HttpCache.ReadErrorRestartable",
                                 result_for_histogram);
   } else {
     UMA_HISTOGRAM_SPARSE_SLOWLY("HttpCache.ReadErrorNonRestartable",
                                 result_for_histogram);
   }
 
   // Avoid using this entry in the future.
   if (cache_.get())
     cache_->DoomActiveEntry(cache_key_);
 
   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;
     return OK;
   }
 
   return ERR_CACHE_READ_FAILURE;
 }
 
 void HttpCache::Transaction::OnAddToEntryTimeout(base::TimeTicks start_time) {
   if (entry_lock_waiting_since_ != start_time)
     return;
 
   DCHECK_EQ(next_state_, STATE_ADD_TO_ENTRY_COMPLETE);
 
   if (!cache_)
     return;
 
   cache_->RemovePendingTransaction(this);
   OnIOComplete(ERR_CACHE_LOCK_TIMEOUT);
 }
 
 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) {
   partial_->OnNetworkReadCompleted(result);
 
   if (result == 0) {
     // We need to move on to the next range.
     ResetNetworkTransaction();
     next_state_ = STATE_START_PARTIAL_CACHE_VALIDATION;
   }
@@ skipping 11 matching lines @@
   }
   return result;
 }
 
 int HttpCache::Transaction::DoRestartPartialRequest() {
   // The stored data cannot be used. Get rid of it and restart this request.
   net_log_.AddEvent(NetLog::TYPE_HTTP_CACHE_RESTART_PARTIAL_REQUEST);
 
   // WRITE + Doom + STATE_INIT_ENTRY == STATE_CREATE_ENTRY (without an attempt
   // to Doom the entry again).
+  ResetPartialState(!range_requested_);
   mode_ = WRITE;
-  ResetPartialState(!range_requested_);
   next_state_ = STATE_CREATE_ENTRY;
   return OK;
 }
 
 void HttpCache::Transaction::ResetPartialState(bool delete_object) {
   partial_->RestoreHeaders(&custom_request_->extra_headers);
   DoomPartialEntry(delete_object);
 
   if (!delete_object) {
     // The simplest way to re-initialize partial_ is to create a new object.
@@ skipping 54 matching lines @@
 void HttpCache::Transaction::UpdateTransactionPattern(
     TransactionPattern new_transaction_pattern) {
   if (transaction_pattern_ == PATTERN_NOT_COVERED)
     return;
   DCHECK(transaction_pattern_ == PATTERN_UNDEFINED ||
          new_transaction_pattern == PATTERN_NOT_COVERED);
   transaction_pattern_ = new_transaction_pattern;
 }
 
 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_);
   DCHECK(!first_cache_access_since_.is_null());
 
   TimeDelta total_time = base::TimeTicks::Now() - first_cache_access_since_;
 
   UMA_HISTOGRAM_TIMES("HttpCache.AccessToDone", total_time);
 
   bool did_send_request = !send_request_since_.is_null();
@@ skipping 53 matching lines @@
     }
     default:
       NOTREACHED();
   }
 }
 
 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