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 a02a4ea8d5ed3a4d846aab57f2343b7c65b55f56..9a3c8bef9243065a5cc770d634ced00c7c194f1d 100644
--- a/net/http/http_cache_transaction.cc
+++ b/net/http/http_cache_transaction.cc
@@ -180,6 +180,7 @@ HttpCache::Transaction::Transaction(RequestPriority priority, HttpCache* cache)
       handling_206_(false),
       cache_pending_(false),
       done_reading_(false),
+      done_headers_(false),
       vary_mismatch_(false),
       couldnt_conditionalize_request_(false),
       bypass_lock_for_test_(false),
@@ -1137,9 +1138,13 @@ int HttpCache::Transaction::DoCreateEntryComplete(int result) {
       // need an atomic OpenOrCreate() method exposed by the disk cache.
       DLOG(WARNING) << "Unable to create cache entry";
       mode_ = NONE;

[Randy Smith (Not in Mondays), 2017/04/04 21:54:03]

I presume this means we're transitioning into a mode where we aren't touching
the cache, just piping through the results of a network transaction?  Comment to
that effect?  (Not in code you're touching, but it's a simple cleanup.)

[shivanisha, 2017/04/07 23:24:13]

done

-      if (partial_)
-        partial_->RestoreHeaders(&custom_request_->extra_headers);
-      TransitionToState(STATE_SEND_REQUEST);
+      if (done_headers_) {
+        TransitionToState(STATE_OVERWRITE_CACHED_RESPONSE);
+      } else {
+        if (partial_)
+          partial_->RestoreHeaders(&custom_request_->extra_headers);
+        TransitionToState(STATE_SEND_REQUEST);
+      }
   }
   return OK;
 }
@@ -1207,6 +1212,15 @@ int HttpCache::Transaction::DoAddToEntryComplete(int result) {
   // If there is a failure, the cache should have taken care of new_entry_.
   new_entry_ = NULL;
 
+  // If this entry creation was a result of validation not matching, then this
+  // transaction should be the headers_transaction.
+  if (done_headers_) {
+    DCHECK_EQ(result, OK);
+    DCHECK_EQ(entry_->headers_transaction, this);
+    TransitionToState(STATE_OVERWRITE_CACHED_RESPONSE);
+    return OK;
+  }
+
   if (result == ERR_CACHE_RACE) {
     TransitionToState(STATE_INIT_ENTRY);
     return OK;
@@ -1603,10 +1617,10 @@ int HttpCache::Transaction::DoSuccessfulSendRequest() {
     mode_ = WRITE;
   }
 
-  TransitionToState(STATE_OVERWRITE_CACHED_RESPONSE);
-
-  if (!entry_)
+  if (!entry_) {

[Randy Smith (Not in Mondays), 2017/04/04 21:54:03]

Could this conditional be more succinctly implemented by eliminate this line and
the next and testing for entry in the conditional below? 

[shivanisha, 2017/04/07 23:24:13]

N/A now that this code is implemented in DoCacheWriteResponse as part of changes
in the parent branch.

+    TransitionToState(STATE_OVERWRITE_CACHED_RESPONSE);
     return OK;
+  }
 
   // Invalidate any current entry with a successful response if this transaction
   // cannot write to this entry.
@@ -1615,10 +1629,12 @@ int HttpCache::Transaction::DoSuccessfulSendRequest() {
     DCHECK_EQ(entry_->headers_transaction, this);
     cache_->DoneResponseHeaders(entry_, this, false);
     entry_ = nullptr;
-    mode_ = NONE;
+    TransitionToState(STATE_CREATE_ENTRY);

[Randy Smith (Not in Mondays), 2017/04/04 21:54:03]

As I understand the logic that follows on from this, it's dependent on the
response_code() being 200, not just not being 304?

[shivanisha, 2017/04/07 23:24:13]

Note that the state OVERWRITE_CACHED_RESPONSE being reached implies that the
headers should be written (either to this entry or to a new entry). If the
response is 304, its ok to write to this entry but in any other case it should
be checked if it is to be written to this entry or another entry.

+    done_headers_ = true;
     return OK;
   }
 
+  TransitionToState(STATE_OVERWRITE_CACHED_RESPONSE);
   return OK;
 }