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 147 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_create_new_entry_(false),
       vary_mismatch_(false),
       couldnt_conditionalize_request_(false),
       bypass_lock_for_test_(false),
       fail_conditionalization_for_test_(false),
       io_buf_len_(0),
       read_offset_(0),
       effective_load_flags_(0),
       write_len_(0),
       cache_entry_status_(CacheEntryStatus::ENTRY_UNDEFINED),
       validation_cause_(VALIDATION_CAUSE_UNDEFINED),
@@ skipping 569 matching lines @@
       case STATE_CREATE_ENTRY_COMPLETE:
         rv = DoCreateEntryComplete(rv);
         break;
       case STATE_ADD_TO_ENTRY:
         DCHECK_EQ(OK, rv);
         rv = DoAddToEntry();
         break;
       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();
         break;
       case STATE_CACHE_READ_RESPONSE_COMPLETE:
         rv = DoCacheReadResponseComplete(rv);
         break;
       case STATE_TOGGLE_UNUSED_SINCE_PREFETCH:
         DCHECK_EQ(OK, rv);
         rv = DoCacheToggleUnusedSincePrefetch();
@@ skipping 255 matching lines @@
   // transaction attached.
   net_log_.EndEventWithNetErrorCode(NetLogEventType::HTTP_CACHE_OPEN_ENTRY,
                                     result);
   cache_pending_ = false;
   if (result == OK) {
     TransitionToState(STATE_ADD_TO_ENTRY);
     return OK;
   }
 
   if (result == ERR_CACHE_RACE) {
-    TransitionToState(STATE_HEADERS_PHASE_CANNOT_PROCEED);
+    TransitionToState(STATE_INIT_ENTRY);
     return OK;
   }
 
   if (request_->method == "PUT" || request_->method == "DELETE" ||
       (request_->method == "HEAD" && mode_ == READ_WRITE)) {
     DCHECK(mode_ == READ_WRITE || mode_ == WRITE || request_->method == "HEAD");
     mode_ = NONE;
     TransitionToState(STATE_SEND_REQUEST);
     return OK;
   }
@@ skipping 24 matching lines @@
     first_cache_access_since_ = TimeTicks::Now();
   net_log_.BeginEvent(NetLogEventType::HTTP_CACHE_DOOM_ENTRY);
   return cache_->DoomEntry(cache_key_, this);
 }
 
 int HttpCache::Transaction::DoDoomEntryComplete(int result) {
   TRACE_EVENT0("io", "HttpCacheTransaction::DoDoomEntryComplete");
   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;
 }
 
 int HttpCache::Transaction::DoCreateEntry() {
   TRACE_EVENT0("io", "HttpCacheTransaction::DoCreateEntry");
   DCHECK(!new_entry_);
   TransitionToState(STATE_CREATE_ENTRY_COMPLETE);
   cache_pending_ = true;
   net_log_.BeginEvent(NetLogEventType::HTTP_CACHE_CREATE_ENTRY);
   return cache_->CreateEntry(cache_key_, &new_entry_, this);
 }
 
 int HttpCache::Transaction::DoCreateEntryComplete(int result) {
   TRACE_EVENT0("io", "HttpCacheTransaction::DoCreateEntryComplete");
   // It is important that we go to STATE_ADD_TO_ENTRY whenever the result is
   // OK, otherwise the cache will end up with an active entry without any
   // transaction attached.
   net_log_.EndEventWithNetErrorCode(NetLogEventType::HTTP_CACHE_CREATE_ENTRY,
                                     result);
   cache_pending_ = false;
   switch (result) {
     case OK:
       TransitionToState(STATE_ADD_TO_ENTRY);
       break;
 
     case ERR_CACHE_RACE:
-      TransitionToState(STATE_HEADERS_PHASE_CANNOT_PROCEED);
+      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";
+      // 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;
 }
 
 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());
+  int rv = cache_->AddTransactionToEntry(new_entry_, this);
+  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();
-  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));
+
+  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;
 }
 
 int HttpCache::Transaction::DoAddToEntryComplete(int result) {
   TRACE_EVENT0("io", "HttpCacheTransaction::DoAddToEntryComplete");
   net_log_.EndEventWithNetErrorCode(NetLogEventType::HTTP_CACHE_ADD_TO_ENTRY,
                                     result);
   const TimeDelta entry_lock_wait =
       TimeTicks::Now() - entry_lock_waiting_since_;
@@ skipping 47 matching lines @@
       partial_->RestoreHeaders(&custom_request_->extra_headers);
     TransitionToState(STATE_SEND_REQUEST);
   } else {
     // We have to read the headers from the cached entry.
     DCHECK(mode_ & READ_META);
     TransitionToState(STATE_CACHE_READ_RESPONSE);
   }
   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_);
   TransitionToState(STATE_CACHE_READ_RESPONSE_COMPLETE);
 
   io_buf_len_ = entry_->GetDataSize(kResponseInfoIndex, this);
   read_buf_ = new IOBuffer(io_buf_len_);
 
   net_log_.BeginEvent(NetLogEventType::HTTP_CACHE_READ_INFO);
   return entry_->disk_entry->ReadData(kResponseInfoIndex, 0, read_buf_.get(),
@@ skipping 471 matching lines @@
   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
   // the network without writing to the backend.
   bool is_match = response_.headers->response_code() == 304;
   if (entry_ && response_.headers &&
       !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;
   }
 
   return WriteResponseInfoToEntry(truncated_);
 }
 
 int HttpCache::Transaction::DoCacheWriteResponseComplete(int result) {
   TRACE_EVENT0("io", "HttpCacheTransaction::DoCacheWriteResponseComplete");
   TransitionToState(STATE_TRUNCATE_CACHED_DATA);
   return OnWriteResponseInfoToEntryComplete(result);
@@ skipping 1381 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