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 146 matching lines @@
       new_response_(NULL),
       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 573 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 338 matching lines @@
     case ERR_CACHE_RACE:
       TransitionToState(STATE_HEADERS_PHASE_CANNOT_PROCEED);
       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

[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;
 }
 
 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 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();
-  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_;
   UMA_HISTOGRAM_TIMES("HttpCache.EntryLockWait", entry_lock_wait);
@@ 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

[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_);
   TransitionToState(STATE_CACHE_READ_RESPONSE_COMPLETE);
 
   io_buf_len_ = entry_->disk_entry->GetDataSize(kResponseInfoIndex);
   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 465 matching lines @@
     TransitionToState(STATE_PARTIAL_HEADERS_RECEIVED);
     return OK;
   }
 
   TransitionToState(STATE_CACHE_WRITE_RESPONSE);
   return OK;
 }
 
 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
   // the network without writing to the backend.
   bool is_match = response_.headers->response_code() == 304;
   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_);
 }
 
 int HttpCache::Transaction::DoCacheWriteResponseComplete(int result) {
   TRACE_EVENT0("io", "HttpCacheTransaction::DoCacheWriteResponseComplete");
   TransitionToState(STATE_TRUNCATE_CACHED_DATA);
   return OnWriteResponseInfoToEntryComplete(result);
 }
 
 int HttpCache::Transaction::DoTruncateCachedData() {
@@ skipping 1379 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