[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),
       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();

[rvargas (doing something else), 2015/08/19 23:46:38]

nit: rename to preserve_entry ?

       if (cancel_request) {
         if (partial_) {
           entry_->disk_entry->CancelSparseIO();
         } else {
           cancel_request &= (response_.headers->response_code() == 200);
         }
       }
 
-      cache_->DoneWithEntry(entry_, this, cancel_request);
+      ReleaseCacheEntry(cancel_request ? CACHE_ENTRY_PRESERVE
+                                       : CACHE_ENTRY_DOOM);
     } else if (cache_pending_) {
       cache_->RemovePendingTransaction(this);
     }
   }
 }
 
 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;
   }
 
   reading_ = true;
   read_buf_ = buf;
   io_buf_len_ = buf_len;
-  if (network_trans_) {
+  if (stopped_caching_ && (mode_ & WRITE) &&

[rvargas (doing something else), 2015/08/19 23:46:38]

Why do we need to check the mode here? (how can we drop that bit from 
StopCaching without dropping entry_?)

[asanka, 2015/09/04 19:09:04]

I removed the mode_ check from StopCaching() and we are checking it here
instead. StopCaching() could be called before Start() is called which could
result in mode_ not being set by the time StopCaching() is called.

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

hmmm. Related to comments elsewhere.

+      !(effective_load_flags_ & LOAD_PREFERRING_CACHE) && entry_) {

[rvargas (doing something else), 2015/08/19 23:46:38]

why do we look at load_flags_?

[asanka, 2015/09/04 19:09:03]

Originally did so because LOAD_PREFERRING_CACHE may have caused us to skip
validation. But it's also the case that once we've decided to use the cached
entry we also drop the WRITE bit. Removed the check.

+    next_state_ = STATE_RELEASE_CACHE_ENTRY_AND_CONTINUE_USING_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(!(effective_load_flags_ & LOAD_PREFETCH));

[rvargas (doing something else), 2015/08/19 23:46:38]

isn't this outside of the control of this code? What would break in the cache if
this is violated by callers?

[asanka, 2015/09/04 19:09:04]

Not sure how this crept in. Removed.

+  DCHECK(callback_.is_null());
+
+  // StopCaching() only affects cache writes and is meant to prevent excessively
+  // large resources from clogging up the cache. If this entry is being read
+  // exclusively from the cache already, then we are going to ignore the

[rvargas (doing something else), 2015/08/19 23:46:38]

I'd probably remove the second sentence, as it is a little confusing because it
talks about the opposite logic of what is tested by the code. The first sentence
already says why we only care for writes.

[asanka, 2015/09/04 19:09:03]

Removed the check and comment since we just set the flag here.

+  // StopCaching() request.
+  if (mode_ & WRITE) {
+    stopped_caching_ = true;
+    UpdateTransactionPattern(PATTERN_NOT_COVERED);
   }
 }
 
 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_) {

[rvargas (doing something else), 2015/08/19 23:46:38]

We still need this code, otherwise it will crash at cache_->

[asanka, 2015/09/04 19:09:03]

Done.

-    DCHECK_NE(mode_, UPDATE);
-    if (mode_ & WRITE) {
-      DoneWritingToEntry(true);
-    } else if (mode_ & READ) {
-      // It is necessary to check mode_ & READ because it is possible

[rvargas (doing something else), 2015/08/19 23:46:38]

We should be able to fix this code now and make this a plain else. (or even
better do a single call to the cache)

[asanka, 2015/09/04 19:09:03]

Done.

-      // 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 (mode_ & WRITE) {
+    DoneWritingToEntry(true);
+  } else if (mode_ & READ) {
+    cache_->DoneReadingFromEntry(entry_, this);
+    entry_ = nullptr;
   }
 }
 
 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_;
 }
 
@@ skipping 342 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_RELEASE_CACHE_ENTRY_AND_CONTINUE_USING_NETWORK:
+        rv = DoReleaseCacheEntryAndContinueUsingNetwork();
+        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();
@@ skipping 489 matching lines @@
   rv = network_trans_->Start(request_, io_callback_, net_log_);
   return rv;
 }
 
 int HttpCache::Transaction::DoSendRequestComplete(int result) {
   if (!cache_.get())
     return ERR_UNEXPECTED;
 
   // If we tried to conditionalize the request and failed, we know
   // we won't be reading from the cache after this point.
-  if (couldnt_conditionalize_request_)
-    mode_ = WRITE;
+  if (couldnt_conditionalize_request_ && mode_ == READ_WRITE)
+    mode_ = stopped_caching_ ? NONE : WRITE;

[rvargas (doing something else), 2015/08/19 23:46:38]

If we switch to NONE, shouldn't we release the cache entry?

[asanka, 2015/09/04 19:09:03]

Removed this logic.

 
   if (result == OK) {
     next_state_ = STATE_SUCCESSFUL_SEND_REQUEST;
     return OK;
   }
 
   const HttpResponseInfo* response = network_trans_->GetResponseInfo();
   response_.network_accessed = response->network_accessed;
 
   // Do not record requests that have network errors or restarts.
@@ skipping 43 matching lines @@
     if (entry_)
       DoomPartialEntry(false);
     mode_ = NONE;
     partial_.reset();
     ResetNetworkTransaction();
     return ERR_CACHE_AUTH_FAILURE_AFTER_READ;
   }
 
   new_response_ = new_response;
   if (!ValidatePartialResponse() && !auth_response_.headers.get()) {
+    if (reading_) {

[rvargas (doing something else), 2015/08/19 23:46:38]

Is this block related to this change? can we make it into a dedicated cl?

[asanka, 2015/09/04 19:09:03]

Removed.

+      // This transaction has already returned headers and possibly response
+      // data to the client on the assumption that future network requests will
+      // be validated. If this assumption is false, the transaction can't
+      // proceed any further.
+      int rv = ReleaseCacheEntry(CACHE_ENTRY_DOOM);
+      DCHECK_EQ(rv, OK);
+      return ERR_UNEXPECTED;
+    }
+
     // Something went wrong with this request and we have to restart it.
     // If we have an authentication response, we are exposed to weird things
     // hapenning if the user cancels the authentication before we receive
     // 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_) {
+    // The transaction successfully switched over to streaming exclusively from
+    // the network.

[rvargas (doing something else), 2015/08/19 23:46:38]

why?. Or what do you mean by "exclusively". This may be just another range in
the middle.

Why don't we let this go until next Read() ?

[asanka, 2015/09/04 19:09:03]

I expanded the condition here to only include the case where it makes sense to
abandon the cache entry and continue using the network. Look again?

+    DCHECK(partial_ && partial_->IsLastRange() &&
+           !partial_->IsCurrentRangeCached());
+    DCHECK(mode_ & WRITE);
+    int rv = ReleaseCacheEntry(CACHE_ENTRY_PRESERVE_PREEXISTING);
+    DCHECK_EQ(rv, OK);
+    mode_ = NONE;
+    stopped_caching_ = false;
+    next_state_ = STATE_NETWORK_READ;
+    return OK;
+  }
+
   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);
   }
 
   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;
+    DoneWritingToEntry(true);
   }
 
   // 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);
 
   if (new_response_->headers->response_code() == 416 &&
       (request_->method == "GET" || request_->method == "POST")) {
     // If there is an active entry it may be destroyed with this transaction.
     response_ = *new_response_;
     return OK;
   }
 
+  if (stopped_caching_) {
+    next_state_ = STATE_PARTIAL_HEADERS_RECEIVED;
+    return OK;
+  }
+
   // Are we expecting a response to a conditional query?
   if (mode_ == READ_WRITE || mode_ == UPDATE) {
     if (new_response->headers->response_code() == 304 || handling_206_) {
       UpdateTransactionPattern(PATTERN_ENTRY_VALIDATED);
       next_state_ = STATE_UPDATE_CACHED_RESPONSE;
       return OK;
     }
     UpdateTransactionPattern(PATTERN_ENTRY_UPDATED);
     mode_ = WRITE;
   }
@@ skipping 229 matching lines @@
   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

[rvargas (doing something else), 2015/08/19 23:46:38]

Why remove this?

[asanka, 2015/09/04 19:09:04]

Collateral. In an intermediate stage I experimented with explicitly removing the
cache lock at this point.

Comment restored.

-  // 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() {
   if (request_->method == "HEAD")
     return 0;
@@ skipping 51 matching lines @@
 
   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);
 
     // We want to ignore errors writing to disk and just keep reading from
     // the network.
     result = write_len_;
   } 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)) {
       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);
-    }
+  // 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.

[rvargas (doing something else), 2015/08/19 23:46:39]

nit: "End of file" is not a given anymore (as it is in the next if); the same
goes for "this may be the result...". Maybe Remove all that and say "see if we
have...", or rewrite to make clear that of or error is part of whet we have to
consider. (yes, this comment/if condition is far from clear)

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

Comment expanded.

+  if (result == 0 && (done_reading_ || !entry_ || partial_ ||
+                      response_.headers->GetContentLength() <= 0)) {
+    DoneWritingToEntry(true);
   }
 
   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);
+  OnWriteResponseInfoToEntryComplete(result);
+  if (entry_) {

[rvargas (doing something else), 2015/08/19 23:46:38]

Why can we be here without entry_?

[asanka, 2015/09/04 19:09:04]

OnWriteResponseInfoToEntry() will doom and release the entry if there's an error
writing the response info.

(I moved this logic to AbandonCacheEntry() and friends.)

+    cache_->DoneWritingToEntry(entry_, true);
+    entry_ = nullptr;
+  }
+  return OK;
+}
+
+int HttpCache::Transaction::DoReleaseCacheEntryAndContinueUsingNetwork() {
+  DCHECK(entry_);
+  DCHECK(mode_ & WRITE);
+
+  if (request_->method != "GET") {

[rvargas (doing something else), 2015/08/19 23:46:38]

look at this on StopCaching?

[asanka, 2015/09/04 19:09:03]

StopCaching() can be called prior to Start() being called.

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

Wait, why do we want to support that? That use case would be equivalent to a
load flag combination.

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

It was done initially on the assumption that callers of StopCaching() may not
necessarily be in a position to set load flags. But in retrospect that was
unnecessary, and certainly not something the network stack should be concerned
with.

I reworked the logic to not support calling StopCaching() before Start(). Added
a DCHECK() in StopCaching() and also moved the verb and WRITE bit checks there.

+    // Skipping between network and cache reads are only expected to work with
+    // GET requests. Ignore stopped_caching_ for everything else.
+    stopped_caching_ = false;
+    next_state_ = network_trans_ ? STATE_NETWORK_READ : STATE_CACHE_READ_DATA;
+    return OK;
+  }
+
+  // The network transaction can only be trivially reused if it can promise to
+  // deliver all the bits necessary to fulfil the original request.
+  if (network_trans_ && (!partial_ || partial_->IsLastRange())) {
+    // Unless this was a sparse entry or a complete preexiting entry, just get

[rvargas (doing something else), 2015/08/19 23:46:39]

typo: preexisting

[asanka, 2015/09/04 19:09:03]

comment changed.

+    // rid of the cache entry entirely.  If the requestor isn't going to use the
+    // entry, we are going to assume that it's unlikely that anyone will.
+    int rv = ReleaseCacheEntry(CACHE_ENTRY_PRESERVE_PREEXISTING);
+    DCHECK_EQ(rv, OK);
+
+    mode_ = NONE;

[rvargas (doing something else), 2015/08/19 23:46:38]

Do this inside Realease?

[asanka, 2015/09/04 19:09:03]

Done.

+    stopped_caching_ = false;

[rvargas (doing something else), 2015/08/19 23:46:38]

Do we have to set this to false (and elsewhere)? If it doesn't matter (or it's
easy to accommodate), I think it is better to leave it on as it will show up in
the occasional crash report and it's good to know what we did to this entry.

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

I think you missed this :)

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

Indeed! Done.

+    next_state_ = STATE_NETWORK_READ;
+    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();
+
+  // partial_ == nullptr case was handled above. From here we can assume that
+  // this transaction has a valid partial state.
+  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.
+    stopped_caching_ = false;
+    return OK;
+  }
+
+  partial_->PrepareCacheValidation(entry_->disk_entry,
+                                   &custom_request_->extra_headers);
+  DCHECK(!partial_->IsCurrentRangeCached());
+
+  // Since we are supposedly switching to network reads from cache reads, we
+  // still need to validate the cache entry. Since partial_ is now switched to
+  // network reads, it will continue with network reads from here on.
+  return BeginCacheValidation();
 }
 
 //-----------------------------------------------------------------------------
 
 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();
@@ skipping 232 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()) {
+      DCHECK(!reading_);  // Conditionalization should only depend on the state

[rvargas (doing something else), 2015/08/19 23:46:38]

nit:

// comment
// blah
DCHECK();

[asanka, 2015/09/04 19:09:03]

Done.

+                          // of the cache prior to starting the request and the
+                          // request method. Failure is unexpected after the
+                          // client has started reading the stream.
+
       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 500 matching lines @@
                                       result);
   }
 
   if (result != io_buf_len_) {
     DLOG(ERROR) << "failed to write response info to cache";
     DoneWritingToEntry(false);
   }
   return OK;
 }
 
+int HttpCache::Transaction::ReleaseCacheEntry(
+    CacheEntryDisposition entry_disposition) {
+  if (!cache_ || !entry_)
+    return OK;
+
+  if (entry_disposition == CACHE_ENTRY_PRESERVE_PREEXISTING) {
+    // The CACHE_ENTRY_PRESERVE_PREEXISTING option is used to indicate that the
+    // current client is not interested in keeping the cache entry around. Hence
+    // get rid of the cache entry unless it's sparse or it existed in its
+    // entirety prior to this transaction.

[rvargas (doing something else), 2015/08/19 23:46:38]

Why in its entirety?. If there's a truncated entry and we have a request for it,
and the caller decided that as far as it is concerned the entry can be gone,
doesn't the fact that the entry was already there isn't enough to not delete it?

[asanka, 2015/09/04 19:09:03]

The deletion is based on the assumption that most of the truncated cache entries
whom StopCaching() is called on are stubs left over from previous download
requests. I.e. the stubs weren't helping anyone by existing.

That said, I don't have any numbers to back up this claim, so I'm willing to
leave preexisting stubs be if you think we should leave them.

(The code here has changed and doesn't exist anymore. But the logic still
survives in AbandonCacheEntry() and friends).

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

I suspect the assumption is correct, but I also would not be surprised if some
truncated entries are not the result of a failed download and then there's a
download request[*].

On the other hand, I think deleting existing entries when we reach code with
disposition of _preserve_existing is surprising, so I'm mostly concerned about
removing surprising behavior, even if we end up doing a little bit more work
because there's a truncated entry.

[*] Weird script/server code, or even a large request where the user change its
mind and cancels it and then downloads it (yes, that would not result in
StopCaching, but that implies more knowledge about what the downloads code would
or would not do).

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

True. It's also not a necessary behavior change for this CL. Given everything, I
removed the logic for removing truncated cache entries after StopCaching() is
called.

+    entry_disposition = is_sparse_ || (mode_ == READ_WRITE && !truncated_)

[rvargas (doing something else), 2015/08/19 23:46:38]

is_sparse || mode & read || truncated_ ?.

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

Code removed.

+                            ? CACHE_ENTRY_PRESERVE
+                            : CACHE_ENTRY_DOOM;
+  }
+
+  // Nothing special needs to happen if this transaction didn't write to the
+  // entry. Just leave it as is and ignore |entry_disposition|.
+  if (!entry_->writer) {

[rvargas (doing something else), 2015/08/19 23:46:38]

We should let the cache decide this part.

[asanka, 2015/09/04 19:09:03]

Done.

+    RecordHistograms();
+    cache_->DoneReadingFromEntry(entry_, this);
+    entry_ = nullptr;
+    return OK;
+  }
+
+  // TODO(asanka): This shouldn't be necessary. HttpCache::DoneWritingToEntry
+  // should destroy the entry if it's already doomed instead of relying on the
+  // |success| parameter to decide to do so.
+  if (entry_->doomed)
+    entry_disposition = CACHE_ENTRY_DOOM;

[rvargas (doing something else), 2015/08/19 23:46:38]

Does this do anything?

[asanka, 2015/09/04 19:09:04]

DoneWithEntry would only destroy the entry and fire off ERR_CACHE_RACE errors to
pending transactions if this was set. Moved to HttpCache::DoneWithEntry()
instead.

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

if entry_->doomed is on, the entry will be discarded automatically by the cache
when the last reference is gone.

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

Yes. But if there are pending transactions, then HttpCache adds the next pending
transaction and proceeds. My sense is that we should invoke the pending
callbacks with ERR_CACHE_RACE and have the next writer re-establish the cache
entry. Is that not the case?

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

I see what you mean. I agree that the cache should figure out what to do... the
transaction should not be looking at ->doomed.

I think the issue is that doomed is not exactly the same as failure. Today,
there can be an ActiveEntry with a writer and 3 pending tx and then a new
request comes in with mode WRITE. That will doom the active entry and create a
new one for the new tx. When the first request completes, if it succeeds, the
first queued request will hopefully read from it, regardless of being another tx
in progress that is downloading a new version of the resource. I mean, no
failures here mean no transactions going from one ActiveEntry to another. The
logic, btw, is that the 5th request started after the first four, so it should
not affect if requests 2-4 read after request 1 or not, even if the data will be
discarded when request 4 completes.

In other words, if this code wants to do something because there was a previous
failure then there should be a flag on the tx, or something like that.

+
+  // The entry should be kept around only if we can use it later. However, the
+  // entry should be marked as truncated if the transaction stopped writing to
+  // it prematurely.
+  if (entry_disposition == CACHE_ENTRY_PRESERVE && !is_sparse_ && reading_ &&

[rvargas (doing something else), 2015/08/19 23:46:38]

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

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

I moved this condition to AbandonCacheEntry().

+      !done_reading_ && (mode_ & WRITE)) {
+    if (CanResume(true)) {
+      truncated_ = true;
+      return DoCacheWriteTruncatedResponse();

[rvargas (doing something else), 2015/08/19 23:46:38]

I don't believe we call a SM method directly anywhere... they are only called
from within the SM switch as part of a formal state transition.

Can we separate what is called from within DoLoop and what is not? The fact that
this method is called both from within and from outside the SM makes this
attempt to re-enter the SM suspicious.

[asanka, 2015/09/04 19:09:04]

Yeah. Fixed. I moved the logic for abandoning a cache entry out of the main SM
since it needs to be invoked outside of the main SM flow.

+    }
+    // The entry is incomplete and cannot be resumed. The data we have cannot be
+    // used.
+    entry_disposition = CACHE_ENTRY_DOOM;
+  }
+
+  DCHECK_EQ(this, entry_->writer);
+  cache_->DoneWritingToEntry(entry_, entry_disposition == CACHE_ENTRY_PRESERVE);
+  entry_ = nullptr;
+  return OK;
+}
+
 void HttpCache::Transaction::DoneWritingToEntry(bool success) {
   if (!entry_)
     return;
 
   RecordHistograms();
 
   cache_->DoneWritingToEntry(entry_, success);
   entry_ = NULL;
+  stopped_caching_ = false;
   mode_ = NONE;  // switch to 'pass through' mode
 }
 
 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;
-    is_sparse_ = false;

[rvargas (doing something else), 2015/08/19 23:46:38]

We're losing this part (and the transaction will be basically restarted).

[asanka, 2015/09/04 19:09:04]

Reinstated.

+    int rv = ReleaseCacheEntry(CACHE_ENTRY_DOOM);
+    DCHECK_EQ(rv, OK);
     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;
+  DCHECK_EQ(rv, OK);
+  rv = ReleaseCacheEntry(CACHE_ENTRY_DOOM);
+  DCHECK_EQ(rv, OK);
   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 87 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 (transaction_pattern_ == PATTERN_UNDEFINED)

[rvargas (doing something else), 2015/08/19 23:46:38]

nit: consolidate with the next line.

[rvargas (doing something else), 2015/08/19 23:46:39]

Now that we can reach this code without having inspected the operation enough to
know the pattern, we should call this method exclusively from the destructor.

[asanka, 2015/09/04 19:09:04]

Done. Also consolidated with next line.

+    return;
   if (!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);
   if (transaction_pattern_ == PATTERN_NOT_COVERED)
     return;
   DCHECK(!range_requested_);
@@ skipping 61 matching lines @@
     default:
       NOTREACHED();
   }
 }
 
 void HttpCache::Transaction::OnIOComplete(int result) {
   DoLoop(result);
 }
 
 }  // namespace net