[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 77ea96c12ec3f44cd5c71a34df112c7ecc0cb1d4..89fe2ddbfc8f8e204b7d26abdcd357c2e5d12f39 100644
--- a/net/http/http_cache_transaction.cc
+++ b/net/http/http_cache_transaction.cc
@@ -263,9 +263,10 @@ HttpCache::Transaction::Transaction(RequestPriority priority, HttpCache* cache)
       range_requested_(false),
       handling_206_(false),
       cache_pending_(false),
-      done_reading_(false),
+      done_writing_(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),
@@ -300,7 +301,7 @@ HttpCache::Transaction::~Transaction() {
         }
       }
 
-      cache_->DoneWithEntry(entry_, this, cancel_request);
+      DoneWithEntry(cancel_request);
     } else if (cache_pending_) {
       cache_->RemovePendingTransaction(this);
     }
@@ -334,7 +335,7 @@ bool HttpCache::Transaction::AddTruncatedFlag() {
     return false;
 
   // We may have received the whole resource already.
-  if (done_reading_)
+  if (done_writing_)
     return true;
 
   truncated_ = true;
@@ -366,7 +367,7 @@ int HttpCache::Transaction::Start(const HttpRequestInfo* request,
   DCHECK(!reading_);
   DCHECK(!network_trans_.get());
   DCHECK(!entry_);
-  DCHECK_EQ(next_state_, STATE_NONE);
+  DCHECK_EQ(STATE_NONE, next_state_);

[rvargas (doing something else), 2015/07/27 22:25:12]

Don't invert the arguments. DCHECKs should read like an if, not like an EXPECT

[asanka, 2015/08/18 22:46:59]

Done.

 
   if (!cache_.get())
     return ERR_UNEXPECTED;
@@ -391,6 +392,7 @@ int HttpCache::Transaction::RestartIgnoringLastError(
 
   // Ensure that we only have one asynchronous call at a time.
   DCHECK(callback_.is_null());
+  DCHECK_EQ(STATE_NONE, next_state_);
 
   if (!cache_.get())
     return ERR_UNEXPECTED;
@@ -410,6 +412,7 @@ int HttpCache::Transaction::RestartWithCertificate(
 
   // Ensure that we only have one asynchronous call at a time.
   DCHECK(callback_.is_null());
+  DCHECK_EQ(STATE_NONE, next_state_);
 
   if (!cache_.get())
     return ERR_UNEXPECTED;
@@ -430,6 +433,7 @@ int HttpCache::Transaction::RestartWithAuth(
 
   // Ensure that we only have one asynchronous call at a time.
   DCHECK(callback_.is_null());
+  DCHECK_EQ(STATE_NONE, next_state_);
 
   if (!cache_.get())
     return ERR_UNEXPECTED;
@@ -453,12 +457,13 @@ bool HttpCache::Transaction::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());
 
+  // Ensure that we only have one asynchronous call at a time.
   DCHECK(callback_.is_null());
+  DCHECK_EQ(next_state_, STATE_NONE);
 
   if (!cache_.get())
     return ERR_UNEXPECTED;
@@ -495,21 +500,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;
-  }
+  stopped_caching_ = true;
+  UpdateTransactionPattern(PATTERN_NOT_COVERED);
 }
 
 bool HttpCache::Transaction::GetFullRequestHeaders(
@@ -529,18 +521,8 @@ 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);
+  DoneWithRequest();
 }
 
 const HttpResponseInfo* HttpCache::Transaction::GetResponseInfo() const {
@@ -1353,16 +1335,9 @@ 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;
-  }
+  // This is the end of the request.
+  if (!result)
+    return DoneWithRequest();
 
   if (result < 0)
     return result;
@@ -1508,9 +1483,7 @@ int HttpCache::Transaction::DoSuccessfulSendRequest() {
       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.
@@ -1645,7 +1618,6 @@ int HttpCache::Transaction::DoOverwriteCachedResponse() {
   if (request_->method == "HEAD") {
     // This response is replacing the cached one.
     DoneWritingToEntry(false);
-    mode_ = NONE;
     new_response_ = NULL;
     return OK;
   }
@@ -1778,11 +1750,21 @@ int HttpCache::Transaction::DoNetworkReadComplete(int result) {
   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)
+  if (result < 0)
     return result;
 
+  // Skip writing if StopCaching() has been called.
+  //
+  // For partial requests the transaction still needs to update its state. If
+  // the network transaction being read was conditionalized, the cache
+  // transaction may need to issue additional network or cache reads to fulfil
+  // the original request.
+  if (stopped_caching_ && partial_)
+    return DoPartialNetworkReadCompleted(result);
+
+  if (mode_ == NONE || stopped_caching_)
+    return DoReadComplete(result);
+
   next_state_ = STATE_CACHE_WRITE_DATA;
   return result;
 }
@@ -1851,12 +1833,9 @@ 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())

[rvargas (doing something else), 2015/07/27 22:25:12]

needs {}

[asanka, 2015/08/18 22:46:59]

Done.

+    net_log_.EndEventWithNetErrorCode(NetLog::TYPE_HTTP_CACHE_WRITE_DATA,
+                                      result);
   if (!cache_.get())
     return ERR_UNEXPECTED;
 
@@ -1867,11 +1846,11 @@ int HttpCache::Transaction::DoCacheWriteDataComplete(int result) {
     // We want to ignore errors writing to disk and just keep reading from
     // the network.
     result = write_len_;
-  } else if (!done_reading_ && entry_) {
+  } else if (!done_writing_ && 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;
+      done_writing_ = true;

[rvargas (doing something else), 2015/07/27 22:25:12]

We should probably keep the old name. So far, it still means that the user is
still reading from the entry, not that we are not writing to the cache anymore.

[asanka, 2015/08/18 22:46:59]

Changed to the old name.

   }
 
   if (partial_) {
@@ -1885,10 +1864,9 @@ int HttpCache::Transaction::DoCacheWriteDataComplete(int 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) {
+    if (done_writing_ || !entry_ || partial_ ||
+        response_.headers->GetContentLength() <= 0)

[rvargas (doing something else), 2015/07/27 22:25:12]

needs {}

[asanka, 2015/08/18 22:46:59]

Done.

       DoneWritingToEntry(true);
-    }
   }
 
   return result;
@@ -2673,6 +2651,31 @@ int HttpCache::Transaction::OnWriteResponseInfoToEntryComplete(int result) {
   return OK;
 }
 
+int HttpCache::Transaction::DoneWithRequest() {
+  DCHECK_EQ(STATE_NONE, next_state_);
+
+  if (!cache_ || !entry_)
+    return OK;
+
+  if (!(mode_ & WRITE)) {
+    cache_->DoneReadingFromEntry(entry_, this);
+    entry_ = nullptr;
+    return OK;
+  }
+
+  if (stopped_caching_ && !is_sparse_ && !done_writing_ && !entry_->doomed) {
+    if (CanResume(true)) {
+      truncated_ = true;
+      return DoCacheWriteTruncatedResponse();

[rvargas (doing something else), 2015/07/27 22:25:12]

This may return pending and that is not expected by the callers. (aka, we should
not enter the sm here).

[asanka, 2015/08/18 22:46:59]

This code was removed in the latest patch set.

+    }
+    int ret = cache_->DoomEntry(cache_key_, nullptr);
+    DCHECK_EQ(OK, ret);
+  }
+
+  DoneWritingToEntry(true);
+  return OK;
+}
+
 void HttpCache::Transaction::DoneWritingToEntry(bool success) {
   if (!entry_)
     return;
@@ -2684,6 +2687,13 @@ void HttpCache::Transaction::DoneWritingToEntry(bool success) {
   mode_ = NONE;  // switch to 'pass through' mode
 }
 
+void HttpCache::Transaction::DoneWithEntry(bool cancel) {
+  cache_->DoneWithEntry(entry_, this, cancel);
+  entry_ = nullptr;
+  is_sparse_ = false;
+  truncated_ = false;
+}
+
 int HttpCache::Transaction::OnCacheReadError(int result, bool restart) {
   DLOG(ERROR) << "ReadData failed: " << result;
   const int result_for_histogram = std::max(0, -result);
@@ -2702,9 +2712,7 @@ int HttpCache::Transaction::OnCacheReadError(int result, bool restart) {
   if (restart) {
     DCHECK(!reading_);
     DCHECK(!network_trans_.get());
-    cache_->DoneWithEntry(entry_, this, false);
-    entry_ = NULL;
-    is_sparse_ = false;
+    DoneWithEntry(false);
     partial_.reset();
     next_state_ = STATE_GET_BACKEND;
     return OK;
@@ -2730,12 +2738,9 @@ 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;
-  is_sparse_ = false;
-  truncated_ = false;
+  DoneWithEntry(false);
   if (delete_object)
-    partial_.reset(NULL);
+    partial_.reset();
 }
 
 int HttpCache::Transaction::DoPartialNetworkReadCompleted(int result) {
@@ -2745,8 +2750,9 @@ int HttpCache::Transaction::DoPartialNetworkReadCompleted(int result) {
     // We need to move on to the next range.
     ResetNetworkTransaction();
     next_state_ = STATE_START_PARTIAL_CACHE_VALIDATION;
+    return OK;
   }
-  return result;
+  return DoReadComplete(result);
 }
 
 int HttpCache::Transaction::DoPartialCacheReadCompleted(int result) {
@@ -2773,6 +2779,47 @@ int HttpCache::Transaction::DoRestartPartialRequest() {
   return OK;
 }
 
+int HttpCache::Transaction::DoReadComplete(int result) {
+  DCHECK(mode_ & WRITE || mode_ == NONE);
+  DCHECK(cache_);
+  DCHECK_LE(0, result);
+  DCHECK(reading_);
+
+  // If there is no partial data, or if this is the last range of, then all the
+  // remaining bytes of the response are going to be coming from the network.
+  // Hence we can release the cache entry and switch to passthrough mode.
+  if ((mode_ & WRITE) && stopped_caching_ &&
+      (!partial_ || partial_->IsLastRange()))

[rvargas (doing something else), 2015/07/27 22:25:12]

needs {}

[asanka, 2015/08/18 22:46:59]

Done.

+    return BeginCacheEntryRelease(result);
+  return result;
+}
+
+int HttpCache::Transaction::BeginCacheEntryRelease(int result_to_forward) {
+  DCHECK_EQ(STATE_NONE, next_state_);
+  DCHECK(stopped_caching_);
+
+  // Any errors from the cache truncation operation is going to be ignored.
+  // There's not much the caller can do about it.
+  callback_ = base::Bind(&Transaction::OnCacheReleaseComplete,

[rvargas (doing something else), 2015/07/27 22:25:12]

This is surprising. So far callback_ is always a caller supplied callback and
adding this hear makes that another axis to think about.

[asanka, 2015/08/18 22:46:59]

Yeah. This was removed.

+                         weak_factory_.GetWeakPtr(),
+                         base::Bind(callback_, result_to_forward));
+  return DoneWithRequest();
+}
+
+void HttpCache::Transaction::OnCacheReleaseComplete(

[rvargas (doing something else), 2015/07/27 22:25:12]

Why is this not part of the state machine?

[asanka, 2015/08/18 22:46:59]

Point taken, but this code was removed.

+    const base::Closure& closure,
+    int result) {
+  // TODO(asanka): Clean this up.
+  if (entry_) {
+    DCHECK(mode_ & WRITE);
+    DoneWritingToEntry(true);
+  }
+  mode_ = NONE;
+  stopped_caching_ = false;
+  if (!closure.is_null())
+    closure.Run();
+}
+
 void HttpCache::Transaction::ResetPartialState(bool delete_object) {
   partial_->RestoreHeaders(&custom_request_->extra_headers);
   DoomPartialEntry(delete_object);