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 94560d545d7a5cff6a7c986b0ee7c1c288c63270..e8b6d3d58f2585b478ed1ed213de61fbaebcc617 100644
--- a/net/http/http_cache_transaction.cc
+++ b/net/http/http_cache_transaction.cc
@@ -165,6 +165,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),
@@ -754,6 +755,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();
@@ -1029,7 +1033,7 @@ int HttpCache::Transaction::DoOpenEntryComplete(int result) {
   }
 
   if (result == ERR_CACHE_RACE) {
-    TransitionToState(STATE_HEADERS_PHASE_CANNOT_PROCEED);
+    TransitionToState(STATE_INIT_ENTRY);
     return OK;
   }
 
@@ -1074,9 +1078,9 @@ int HttpCache::Transaction::DoDoomEntryComplete(int result) {
   net_log_.EndEventWithNetErrorCode(NetLogEventType::HTTP_CACHE_DOOM_ENTRY,
                                     result);
   cache_pending_ = false;
-  TransitionToState(result == ERR_CACHE_RACE
-                        ? STATE_HEADERS_PHASE_CANNOT_PROCEED
-                        : STATE_CREATE_ENTRY);
+
+  TransitionToState(result == ERR_CACHE_RACE ? STATE_INIT_ENTRY
+                                             : STATE_CREATE_ENTRY);
   return OK;
 }
 
@@ -1103,7 +1107,7 @@ int HttpCache::Transaction::DoCreateEntryComplete(int result) {
       break;
 
     case ERR_CACHE_RACE:
-      TransitionToState(STATE_HEADERS_PHASE_CANNOT_PROCEED);
+      TransitionToState(STATE_INIT_ENTRY);
       break;
 
     default:
@@ -1112,10 +1116,24 @@ 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
+      // 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
+      // 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;
 }
@@ -1124,43 +1142,50 @@ 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 (done_headers_create_new_entry_) {
+    TransitionToState(STATE_DONE_HEADERS_ADD_TO_ENTRY_COMPLETE);

[Randy Smith (Not in Mondays), 2017/04/27 17:47:12]

It's my belief that short-circuiting all the logic below about the cache lock in
this case is ok because we know that there aren't any other transactions racing
on this entry, since we just created it.  If that's true, could you put in a
comment about it?  (I'd love to have a DCHECK as well, but that probably
requires reaching into the cache's active entry which might be a layering
violation, so I'll just mention it rather than request it.)

[shivanisha, 2017/05/31 15:51:47]

Added a comment.

+    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;
 }
@@ -1228,6 +1253,25 @@ 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
+  // created. |this| transaction should thus go ahead and write the response to
+  // the newly created entry.

[Randy Smith (Not in Mondays), 2017/04/27 17:47:12]

Preface second sentence with something like "A response of no-match from a
validation request also includes the full contents of the URL, so ..."? 
(Suggestion.)

Possibly (also suggestion) also add a note about how a no-match will always
return the full contents of the entry, so the logic around partial and truncated
responses in DoAddToEntryComplete isn't necessary in this version.

[shivanisha, 2017/05/31 15:51:47]

Done
The logic around partial is also to do with cache lock timeout and since this
flow should never be subjected to cache lock, none of that logic makes sense for
this flow. Added a comment for that.

+  DCHECK_EQ(result, OK);
+  DCHECK(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;
+  DCHECK(cache_->CanTransactionWriteResponseHeaders(entry_, this, is_match));
+  TransitionToState(STATE_CACHE_WRITE_RESPONSE);
+  return OK;
+}
+
 int HttpCache::Transaction::DoCacheReadResponse() {
   TRACE_EVENT0("io", "HttpCacheTransaction::DoCacheReadResponse");
   DCHECK(entry_);
@@ -1719,7 +1763,13 @@ int HttpCache::Transaction::DoCacheWriteResponse() {
       !cache_->CanTransactionWriteResponseHeaders(entry_, this, is_match)) {
     cache_->DoneWritingToEntry(entry_, false, this);
     entry_ = nullptr;
-    mode_ = NONE;
+    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 will take care of
+    // dooming if any other entry exists.
+    mode_ = WRITE;
+    next_state_ = STATE_INIT_ENTRY;

[Randy Smith (Not in Mondays), 2017/04/27 17:47:12]

nit: TransitionToState?  (& move the TransitionToState() above after the if
block).

[shivanisha, 2017/05/31 15:51:47]

done

     return OK;
   }