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 2a82a58d9e3d481bd1ecfe49f9a24e94441a0fa9..62a306c9abce2d953f33ee928bc55ef4a4959ed5 100644
--- a/net/http/http_cache_transaction.cc
+++ b/net/http/http_cache_transaction.cc
@@ -165,6 +165,7 @@ HttpCache::Transaction::Transaction(RequestPriority priority, HttpCache* cache)
       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),
@@ -754,6 +755,9 @@ int HttpCache::Transaction::DoLoop(int result) {
       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();
@@ -1112,7 +1116,14 @@ int HttpCache::Transaction::DoCreateEntryComplete(int result) {
       // 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";
+      // Change the mode to not write anything to the cache.

[Randy Smith (Not in Mondays), 2017/04/11 19:41:07]

This comment doesn't really add anything to the code; that's what "mode_ =
NONE;" means.  "Switching into passing through data directly from the network,
avoiding the cache entry" was more what I had in mind.

[shivanisha, 2017/04/24 17:23:46]

Done

       mode_ = NONE;
+      if (done_headers_create_new_entry_) {

[Randy Smith (Not in Mondays), 2017/04/11 19:41:07]

Maybe a comment here something like "The headers to use to write the response
have already been received as a result of validation, triggering the doom of the
old entry.  So no network request needs to be sent."? 

That comment's not ideal because the setting of the STATE_SEND_REQUEST is a ways
away from the point of comment, so if you want to fix that (by changing the
comment or restructuring the code) I'm good with that.  But I'd like to indicate
in comment what (at a high level, not just repeating the information in the
code) is being done by the conditional.

[shivanisha, 2017/04/24 17:23:46]

Comment added and restructured the code.

+        done_headers_create_new_entry_ = false;
+        TransitionToState(STATE_CACHE_WRITE_RESPONSE);
+        return OK;
+      }
+
       if (partial_)
         partial_->RestoreHeaders(&custom_request_->extra_headers);
       TransitionToState(STATE_SEND_REQUEST);
@@ -1124,43 +1135,49 @@ 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());
-  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));
+  DCHECK_EQ(rv, ERR_IO_PENDING);
+
+  if (done_headers_create_new_entry_) {
+    TransitionToState(STATE_DONE_HEADERS_ADD_TO_ENTRY_COMPLETE);
+    return rv;
+  }
+
+  TransitionToState(STATE_ADD_TO_ENTRY_COMPLETE);

[Randy Smith (Not in Mondays), 2017/04/11 19:41:07]

The naming makes it clear that these are parallel states, which is going to
raise the question in a code reader why the code within this state isn't
parallel.   Could you either make the code paths parallel (i.e. I'm not sure
there's harm in using the timeout path for the doom-and-create state, though we
know that the cache lock won't be held if we're creating a new entry) or put in
a comment about why they aren't parallel?

[shivanisha, 2017/04/24 17:23:46]

Added a comment in the new Do* function.

+
+  entry_lock_waiting_since_ = TimeTicks::Now();
+  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));

[Randy Smith (Not in Mondays), 2017/04/11 19:41:07]

Why is this pulled out of the conditional?  It looks like there'll be a double
post in the test case.

[shivanisha, 2017/04/24 17:23:46]

It is not out of the else conditional. This post task is part of the else but
out of the if (partial ...) case as in original code, so no double post.

I think the confusion arises because the diff view and that's because the
indentation changed from original as there isn't an if (rv == ERR_IO_PENDING)
outside the whole post-task logic.

   }
   return rv;
 }
@@ -1225,6 +1242,21 @@ int HttpCache::Transaction::DoAddToEntryComplete(int result) {
   return OK;
 }
 
+int HttpCache::Transaction::DoDoneHeadersAddToEntryComplete(int result) {

[Randy Smith (Not in Mondays), 2017/04/11 19:41:07]

Comment somewhere as to the context/motivation for the differences between this
and DoAddToEntryComplete?

[shivanisha, 2017/04/24 17:23:46]

done.

+  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;
+  DCHECK(cache_->CanTransactionWriteResponseHeaders(
+      entry_, this, response_.headers->response_code()));
+  TransitionToState(STATE_CACHE_WRITE_RESPONSE);
+  return OK;
+}
+
 int HttpCache::Transaction::DoCacheReadResponse() {
   TRACE_EVENT0("io", "HttpCacheTransaction::DoCacheReadResponse");
   DCHECK(entry_);
@@ -1713,7 +1745,8 @@ int HttpCache::Transaction::DoCacheWriteResponse() {
           entry_, this, response_.headers->response_code())) {
     cache_->DoneWritingToEntry(entry_, false, this);
     entry_ = nullptr;
-    mode_ = NONE;
+    next_state_ = STATE_CREATE_ENTRY;
+    done_headers_create_new_entry_ = true;
     return OK;
   }