[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 44acc4bcc27ae21a1ad7facc46700e0787c78e66..e7ae6a8e3823795c0e10beb6bcb44296c5daec76 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;
@@ -495,21 +496,12 @@ 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,7 @@ 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;
-    }
-  }
+  DoneWithRequest();
 }
 
 const HttpResponseInfo* HttpCache::Transaction::GetResponseInfo() const {
@@ -1351,16 +1332,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;
@@ -1506,9 +1480,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.
@@ -1643,7 +1615,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;
   }
@@ -1776,9 +1747,19 @@ 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 result;
 
   next_state_ = STATE_CACHE_WRITE_DATA;
@@ -1849,12 +1830,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())
+    net_log_.EndEventWithNetErrorCode(NetLog::TYPE_HTTP_CACHE_WRITE_DATA,
+                                      result);
   if (!cache_.get())
     return ERR_UNEXPECTED;
 
@@ -1865,11 +1843,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;
   }
 
   if (partial_) {
@@ -1883,10 +1861,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)
       DoneWritingToEntry(true);
-    }
   }
 
   return result;
@@ -2673,6 +2650,32 @@ int HttpCache::Transaction::OnWriteResponseInfoToEntryComplete(int result) {
   return OK;
 }
 
+int HttpCache::Transaction::DoneWithRequest() {
+  if (!cache_ || !entry_)
+    return OK;
+
+  DCHECK_EQ(STATE_NONE, next_state_);

[hubbe, 2015/07/16 15:56:12]

Shouldn't these DCHECKs go first?

[asanka, 2015/07/16 17:57:00]

Moved up.

+  DCHECK_NE(mode_, UPDATE);
+
+  if (!(mode_ & WRITE)) {
+    cache_->DoneReadingFromEntry(entry_, this);
+    entry_ = NULL;
+    return OK;
+  }
+
+  if (stopped_caching_ && !is_sparse_ && !done_writing_ && !entry_->doomed) {
+    if (CanResume(true)) {
+      truncated_ = true;
+      return DoCacheWriteTruncatedResponse();
+    }
+    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) {