Adds cache lock timeout handling after finishing headers phase.

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 29d040c885adf1ad9706a4754694155bf9755246..00e79d16e394d8e9898b40556c33f721ed9c2488 100644
--- a/net/http/http_cache_transaction.cc
+++ b/net/http/http_cache_transaction.cc
@@ -168,6 +168,7 @@ HttpCache::Transaction::Transaction(RequestPriority priority, HttpCache* cache)
       vary_mismatch_(false),
       couldnt_conditionalize_request_(false),
       bypass_lock_for_test_(false),
+      bypass_lock_after_headers_for_test_(false),
       fail_conditionalization_for_test_(false),
       io_buf_len_(0),
       read_offset_(0),
@@ -853,7 +854,7 @@ int HttpCache::Transaction::DoLoop(int result) {
         rv = DoCacheReadMetadataComplete(rv);
         break;
       case STATE_HEADERS_PHASE_CANNOT_PROCEED:
-        rv = DoHeadersPhaseCannotProceed();
+        rv = DoHeadersPhaseCannotProceed(rv);
         break;
       case STATE_FINISH_HEADERS:
         rv = DoFinishHeaders(rv);
@@ -923,6 +924,10 @@ int HttpCache::Transaction::DoGetBackendComplete(int result) {
                                     result);
   cache_pending_ = false;
 
+  // Reset mode_ that might get set in this function. This is done because this
+  // function can be invoked multiple times for a transaction.
+  mode_ = NONE;
+
   if (!ShouldPassThrough()) {
     cache_key_ = cache_->GenerateCacheKey(request_);
 
@@ -1131,40 +1136,44 @@ int HttpCache::Transaction::DoAddToEntry() {
   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));
+  if (rv == ERR_IO_PENDING)
+    AddCacheLockTimeoutHandler(new_entry_);
+  return rv;
+}
+
+void HttpCache::Transaction::AddCacheLockTimeoutHandler(ActiveEntry* entry) {
+  if ((bypass_lock_for_test_ && next_state_ == STATE_ADD_TO_ENTRY_COMPLETE) ||

[Randy Smith (Not in Mondays), 2017/06/23 20:20:46]

suggestion, no need for it to be in this CL if you like it: Change the variables
to be a single "bypass_lock_for_state_" which would be set to a particular state
and then just checked against the current state here.  The positive side of this
is that it's less test code in the production binary, the negative is that it
requires any test that uses it to be a friend (so that it has visibility into
HC::T state names).  

[Randy Smith (Not in Mondays), 2017/06/23 20:20:47]

suggestion: Add DCHECK that we're in one of those two states.

+      (bypass_lock_after_headers_for_test_ &&
+       next_state_ == STATE_FINISH_HEADERS_COMPLETE)) {
+    base::ThreadTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE,
+        base::Bind(&HttpCache::Transaction::OnCacheLockTimeout,
+                   weak_factory_.GetWeakPtr(), entry_lock_waiting_since_));
+  } else {
+    int timeout_milliseconds = 20 * 1000;
+    if (partial_ && entry->writer && 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::OnCacheLockTimeout,
+                   weak_factory_.GetWeakPtr(), entry_lock_waiting_since_),
+        TimeDelta::FromMilliseconds(timeout_milliseconds));
   }
-  return rv;
 }
 
 int HttpCache::Transaction::DoAddToEntryComplete(int result) {
@@ -1817,7 +1826,7 @@ int HttpCache::Transaction::DoPartialHeadersReceived() {
   return OK;
 }
 
-int HttpCache::Transaction::DoHeadersPhaseCannotProceed() {
+int HttpCache::Transaction::DoHeadersPhaseCannotProceed(int result) {
   // If its the Start state machine and it cannot proceed due to a cache
   // failure, restart this transaction.
   DCHECK(!reading_);
@@ -1831,6 +1840,10 @@ int HttpCache::Transaction::DoHeadersPhaseCannotProceed() {
 
   entry_ = nullptr;
 
+  // Bypass the cache for timeout scenario.
+  if (result == ERR_CACHE_LOCK_TIMEOUT)
+    effective_load_flags_ |= LOAD_DISABLE_CACHE;
+
   TransitionToState(STATE_GET_BACKEND);
   return OK;
 }
@@ -1854,13 +1867,20 @@ int HttpCache::Transaction::DoFinishHeaders(int result) {
   // If the transaction needs to wait because another transaction is still
   // writing the response body, it will return ERR_IO_PENDING now and the
   // io_callback_ will be invoked when the wait is done.
-  return cache_->DoneWithResponseHeaders(entry_, this, partial_ != nullptr);
+  int rv = cache_->DoneWithResponseHeaders(entry_, this, partial_ != nullptr);
+  if (rv == ERR_IO_PENDING) {
+    DCHECK(entry_lock_waiting_since_.is_null());
+    entry_lock_waiting_since_ = TimeTicks::Now();
+    AddCacheLockTimeoutHandler(entry_);

[Randy Smith (Not in Mondays), 2017/06/23 20:20:47]

nit, suggestion, doesn't need to be in this CL if you like it: Put the
assignment to entry_lock_waiting_since_ inside of AddCacheLockTimeoutHandler()
and have a matching function (DisableCacheLockTimeoutHandler()) which resets it.
 

+  }
+  return rv;
 }
 
 int HttpCache::Transaction::DoFinishHeadersComplete(int rv) {
-  if (rv == ERR_CACHE_RACE) {
+  entry_lock_waiting_since_ = TimeTicks();
+  if (rv == ERR_CACHE_RACE || rv == ERR_CACHE_LOCK_TIMEOUT) {
     TransitionToState(STATE_HEADERS_PHASE_CANNOT_PROCEED);
-    return OK;
+    return rv;
   }
 
   TransitionToState(STATE_NONE);
@@ -2805,16 +2825,20 @@ int HttpCache::Transaction::OnCacheReadError(int result, bool restart) {
   return ERR_CACHE_READ_FAILURE;
 }
 
-void HttpCache::Transaction::OnAddToEntryTimeout(base::TimeTicks start_time) {
+void HttpCache::Transaction::OnCacheLockTimeout(base::TimeTicks start_time) {
   if (entry_lock_waiting_since_ != start_time)
     return;
 
-  DCHECK_EQ(next_state_, STATE_ADD_TO_ENTRY_COMPLETE);
+  DCHECK(next_state_ == STATE_ADD_TO_ENTRY_COMPLETE ||
+         next_state_ == STATE_FINISH_HEADERS_COMPLETE);
 
   if (!cache_)
     return;
 
-  cache_->RemovePendingTransaction(this);
+  if (next_state_ == STATE_ADD_TO_ENTRY_COMPLETE)
+    cache_->RemovePendingTransaction(this);
+  else
+    cache_->DoneWithEntry(entry_, this, false, partial_ != nullptr);
   OnIOComplete(ERR_CACHE_LOCK_TIMEOUT);
 }