Doom and create new entry when validation is not a match

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 9bc83264feab97d61d1c8acdcd567b2442cc5ea9..e2c2f6067eddef67fa1bf152693d8ecd10d50142 100644
--- a/net/http/http_cache_transaction.cc
+++ b/net/http/http_cache_transaction.cc
@@ -164,6 +164,7 @@ HttpCache::Transaction::Transaction(RequestPriority priority, HttpCache* cache)
       handling_206_(false),
       cache_pending_(false),
       done_reading_(false),
+      done_headers_create_new_entry_(false),
       vary_mismatch_(false),
       couldnt_conditionalize_request_(false),
       bypass_lock_for_test_(false),
@@ -757,6 +758,9 @@ int HttpCache::Transaction::DoLoop(int result) {
       case STATE_ADD_TO_ENTRY_COMPLETE:
         rv = DoAddToEntryComplete(rv);
         break;
+      case STATE_DONE_HEADERS_ADD_TO_ENTRY_COMPLETE:
+        rv = DoDoneHeadersAddToEntryComplete(rv);
+        break;
       case STATE_CACHE_READ_RESPONSE:
         DCHECK_EQ(OK, rv);
         rv = DoCacheReadResponse();
@@ -1115,10 +1119,22 @@ int HttpCache::Transaction::DoCreateEntryComplete(int result) {
       // already created the entry. If we want to eliminate this issue, we
       // need an atomic OpenOrCreate() method exposed by the disk cache.
       DLOG(WARNING) << "Unable to create cache entry";
+      // Switching into passing through data directly from the network, avoiding

[jkarlin, 2017/06/16 18:28:01]

Suggest: Set the mode to NONE in order to bypass the cache entry and read from
the network directly.

[shivanisha, 2017/06/27 15:31:14]

done

+      // the cache entry.
       mode_ = NONE;
-      if (partial_)
-        partial_->RestoreHeaders(&custom_request_->extra_headers);
-      TransitionToState(STATE_SEND_REQUEST);
+      if (!done_headers_create_new_entry_) {
+        if (partial_)
+          partial_->RestoreHeaders(&custom_request_->extra_headers);
+        TransitionToState(STATE_SEND_REQUEST);
+        return OK;
+      }
+      // The headers have already been received as a result of validation,
+      // triggering the doom of the old entry.  So no network request needs to
+      // be sent. Note that since mode_ is set to pass-through, response will

[jkarlin, 2017/06/16 18:28:01]

Suggest: Note that since mode_ is NONE, the response won't be written to cache.
Transition to STATE_CACHE_WRITE_RESPONSE as that's the state the transaction
left off on when it tried to create the new entry.

[shivanisha, 2017/06/27 15:31:14]

done

+      // not be written to the cache but moving to state
+      // STATE_CACHE_WRITE_RESPONSE for consistency.
+      done_headers_create_new_entry_ = false;
+      TransitionToState(STATE_CACHE_WRITE_RESPONSE);
   }
   return OK;
 }
@@ -1127,44 +1143,56 @@ int HttpCache::Transaction::DoAddToEntry() {
   TRACE_EVENT0("io", "HttpCacheTransaction::DoAddToEntry");
   DCHECK(new_entry_);
   cache_pending_ = true;
-  TransitionToState(STATE_ADD_TO_ENTRY_COMPLETE);
   net_log_.BeginEvent(NetLogEventType::HTTP_CACHE_ADD_TO_ENTRY);
   DCHECK(entry_lock_waiting_since_.is_null());
-  entry_lock_waiting_since_ = TimeTicks::Now();
   int rv = cache_->AddTransactionToEntry(new_entry_, this);
-  if (rv == ERR_IO_PENDING) {
-    if (bypass_lock_for_test_) {
-      base::ThreadTaskRunnerHandle::Get()->PostTask(
-          FROM_HERE,
-          base::Bind(&HttpCache::Transaction::OnAddToEntryTimeout,
-                     weak_factory_.GetWeakPtr(), entry_lock_waiting_since_));
-    } else {
-      int timeout_milliseconds = 20 * 1000;
-      if (partial_ && new_entry_->writer &&
-          new_entry_->writer->range_requested_) {
-        // Quickly timeout and bypass the cache if we're a range request and
-        // we're blocked by the reader/writer lock. Doing so eliminates a long
-        // running issue, http://crbug.com/31014, where two of the same media
-        // resources could not be played back simultaneously due to one locking
-        // the cache entry until the entire video was downloaded.
-        //
-        // Bypassing the cache is not ideal, as we are now ignoring the cache
-        // entirely for all range requests to a resource beyond the first. This
-        // is however a much more succinct solution than the alternatives, which
-        // would require somewhat significant changes to the http caching logic.
-        //
-        // Allow some timeout slack for the entry addition to complete in case
-        // the writer lock is imminently released; we want to avoid skipping
-        // the cache if at all possible. See http://crbug.com/408765
-        timeout_milliseconds = 25;
-      }
-      base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
-          FROM_HERE,
-          base::Bind(&HttpCache::Transaction::OnAddToEntryTimeout,
-                     weak_factory_.GetWeakPtr(), entry_lock_waiting_since_),
-          TimeDelta::FromMilliseconds(timeout_milliseconds));
+  DCHECK_EQ(rv, ERR_IO_PENDING);
+
+  // If headers phase is already done and we are here because of validation not
+  // matching and creating a new entry, then this transaction should be the
+  // first transaction of that entry and thus it should not be subject
+  // to any cache lock delays, thus returning early from here.

[jkarlin, 2017/06/16 18:28:01]

This is a very long sentence, please break it up.

[shivanisha, 2017/06/27 15:31:14]

done

+  if (done_headers_create_new_entry_) {
+    DCHECK_EQ(mode_, WRITE);
+    TransitionToState(STATE_DONE_HEADERS_ADD_TO_ENTRY_COMPLETE);
+    return rv;
+  }
+
+  TransitionToState(STATE_ADD_TO_ENTRY_COMPLETE);
+  entry_lock_waiting_since_ = TimeTicks::Now();
+
+  if (bypass_lock_for_test_) {
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE,
+        base::Bind(&HttpCache::Transaction::OnAddToEntryTimeout,
+                   weak_factory_.GetWeakPtr(), entry_lock_waiting_since_));
+  } else {
+    int timeout_milliseconds = 20 * 1000;
+    if (partial_ && new_entry_->writer &&
+        new_entry_->writer->range_requested_) {
+      // Quickly timeout and bypass the cache if we're a range request and
+      // we're blocked by the reader/writer lock. Doing so eliminates a long
+      // running issue, http://crbug.com/31014, where two of the same media
+      // resources could not be played back simultaneously due to one locking
+      // the cache entry until the entire video was downloaded.
+      //
+      // Bypassing the cache is not ideal, as we are now ignoring the cache
+      // entirely for all range requests to a resource beyond the first. This
+      // is however a much more succinct solution than the alternatives, which
+      // would require somewhat significant changes to the http caching logic.
+      //
+      // Allow some timeout slack for the entry addition to complete in case
+      // the writer lock is imminently released; we want to avoid skipping
+      // the cache if at all possible. See http://crbug.com/408765
+      timeout_milliseconds = 25;
     }
+    base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
+        FROM_HERE,
+        base::Bind(&HttpCache::Transaction::OnAddToEntryTimeout,
+                   weak_factory_.GetWeakPtr(), entry_lock_waiting_since_),
+        TimeDelta::FromMilliseconds(timeout_milliseconds));
   }
+
   return rv;
 }
 
@@ -1232,6 +1260,28 @@ int HttpCache::Transaction::DoAddToEntryComplete(int result) {
   return OK;
 }
 
+int HttpCache::Transaction::DoDoneHeadersAddToEntryComplete(int result) {
+  // This state is reached when |this| has already completed validation leading
+  // to a no-match with original entry which was doomed and |new_entry_| was

[jkarlin, 2017/06/16 18:28:01]

Suggest: This transaction's response headers did not match its ActiveEntry so it
created a new ActiveEntry (new_entry_) to write to (and doomed the old one). Now
that the new entry has been created, start writing the response.

[shivanisha, 2017/06/27 15:31:14]

done

+  // created. A response of no-match from a validation request also includes the
+  // full contents of the URL, so go ahead and write the response to the newly
+  // created entry.
+
+  DCHECK_EQ(result, OK);
+  DCHECK_EQ(mode_, WRITE);
+  DCHECK(new_entry_);
+  DCHECK(response_.headers);
+
+  cache_pending_ = false;
+  entry_ = new_entry_;
+  done_headers_create_new_entry_ = false;
+  bool is_match = response_.headers->response_code() == 304;

[jkarlin, 2017/06/16 18:28:01]

Hmm, can it be a 304? If it were a 304 then the transaction wouldn't have
overwritten the old response right? Can we DCHECK_NE?

[shivanisha, 2017/06/27 15:31:14]

done

+  DCHECK(cache_->CanTransactionWriteResponseHeaders(
+      entry_, this, partial_ != nullptr, is_match));
+  TransitionToState(STATE_CACHE_WRITE_RESPONSE);
+  return OK;
+}
+
 int HttpCache::Transaction::DoCacheReadResponse() {
   TRACE_EVENT0("io", "HttpCacheTransaction::DoCacheReadResponse");
   DCHECK(entry_);
@@ -1717,7 +1767,6 @@ int HttpCache::Transaction::DoOverwriteCachedResponse() {
 
 int HttpCache::Transaction::DoCacheWriteResponse() {
   TRACE_EVENT0("io", "HttpCacheTransaction::DoCacheWriteResponse");
-  TransitionToState(STATE_CACHE_WRITE_RESPONSE_COMPLETE);
 
   // Invalidate any current entry with a successful response if this transaction
   // cannot write to this entry. This transaction then continues to read from
@@ -1726,12 +1775,19 @@ int HttpCache::Transaction::DoCacheWriteResponse() {
   if (entry_ && response_.headers &&
       !cache_->CanTransactionWriteResponseHeaders(
           entry_, this, partial_ != nullptr, is_match)) {
-    cache_->DoneWritingToEntry(entry_, false, this);
+    done_headers_create_new_entry_ = true;
+
+    // This transaction should not add itself to any other existing entry but
+    // create a new entry. Going to state STATE_INIT_ENTRY and setting mode_ to
+    // WRITE will take care of dooming if any other entry exists.

[jkarlin, 2017/06/16 18:28:01]

Suggest:

The transaction needs to overwrite this response. Doom the current entry, create
a new one (by going to STATE_INIT_ENTRY), and then jump straight to writing out
the response, bypassing the headers checks. The mode_ is set to WRITE in order
to doom any other existing entries that might exist so that this transaction can
go straight to writing a response.

[shivanisha, 2017/06/27 15:31:14]

done

+    mode_ = WRITE;
+    TransitionToState(STATE_INIT_ENTRY);
+    cache_->DoomEntryValidationNoMatch(entry_, this);
     entry_ = nullptr;
-    mode_ = NONE;
     return OK;
   }
 
+  TransitionToState(STATE_CACHE_WRITE_RESPONSE_COMPLETE);
   return WriteResponseInfoToEntry(truncated_);
 }