Doom and create new entry when validation is not a match

net/http/http_cache_transaction.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/http/http_cache_transaction.h"
 
 #include "build/build_config.h"  // For OS_POSIX
 
 #if defined(OS_POSIX)
 #include <unistd.h>
@@ skipping 162 matching lines @@
       mode_(NONE),
       original_mode_(NONE),
       reading_(false),
       invalid_range_(false),
       truncated_(false),
       is_sparse_(false),
       range_requested_(false),
       handling_206_(false),
       cache_pending_(false),
       done_reading_(false),
+      done_headers_(false),
       vary_mismatch_(false),
       couldnt_conditionalize_request_(false),
       bypass_lock_for_test_(false),
       fail_conditionalization_for_test_(false),
       validating_cannot_proceed_(false),
       io_buf_len_(0),
       read_offset_(0),
       effective_load_flags_(0),
       write_len_(0),
       cache_entry_status_(CacheEntryStatus::ENTRY_UNDEFINED),
@@ skipping 936 matching lines @@
     case ERR_CACHE_RACE:
       TransitionToState(STATE_INIT_ENTRY);
       break;
 
     default:
       // We have a race here: Maybe we failed to open the entry and decided to
       // create one, but by the time we called create, another transaction
       // 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";
       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;
 }
 
 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);
@@ skipping 47 matching lines @@
   entry_lock_waiting_since_ = TimeTicks();
   DCHECK(new_entry_);
   cache_pending_ = false;
 
   if (result == OK)
     entry_ = new_entry_;
 
   // 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;
   }
 
   if (result == ERR_CACHE_LOCK_TIMEOUT) {
     if (mode_ == READ) {
       TransitionToState(STATE_NONE);
       return ERR_CACHE_MISS;
     }
@@ skipping 376 matching lines @@
   if (mode_ == READ_WRITE || mode_ == UPDATE) {
     if (new_response->headers->response_code() == 304 || handling_206_) {
       UpdateCacheEntryStatus(CacheEntryStatus::ENTRY_VALIDATED);
       TransitionToState(STATE_UPDATE_CACHED_RESPONSE);
       return OK;
     }
     UpdateCacheEntryStatus(CacheEntryStatus::ENTRY_UPDATED);
     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.
   if (new_response->headers->response_code() != 304 &&
       (entry_->writer || !entry_->readers.empty())) {
     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;
 }
 
 // We received 304 or 206 and we want to update the cached response headers.
 int HttpCache::Transaction::DoUpdateCachedResponse() {
   TRACE_EVENT0("io", "HttpCacheTransaction::DoUpdateCachedResponse");
   int rv = OK;
   // Update the cached response based on the headers and properties of
   // new_response_.
   response_.headers->Update(*new_response_->headers.get());
@@ skipping 1599 matching lines @@
 }
 
 void HttpCache::Transaction::TransitionToState(State state) {
   // Ensure that the state is only set once per Do* state.
   DCHECK(in_do_loop_);
   DCHECK_EQ(STATE_UNSET, next_state_) << "Next state is " << state;
   next_state_ = state;
 }
 
 }  // namespace net