[HttpCache::Transaction] Force states to set the next state

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 ede3541aa4010750256d9acb89f3140bf6938def..73ee69f0f3983c33a749be7a74805dffd35624ea 100644
--- a/net/http/http_cache_transaction.cc
+++ b/net/http/http_cache_transaction.cc
@@ -712,12 +712,13 @@ size_t HttpCache::Transaction::EstimateMemoryUsage() const {
 //   Like examples 2-4, only CacheToggleUnusedSincePrefetch* is inserted between
 //   CacheReadResponse* and CacheDispatchValidation.
 int HttpCache::Transaction::DoLoop(int result) {
+  DCHECK(next_state_ != STATE_UNSET);
   DCHECK(next_state_ != STATE_NONE);

[Randy Smith (Not in Mondays), 2017/03/22 14:49:44]

Suggestion: DCHECK_NE for both?

[jkarlin, 2017/03/22 15:04:26]

Done.

 
   int rv = result;
   do {
     State state = next_state_;
-    next_state_ = STATE_NONE;
+    next_state_ = STATE_UNSET;
     switch (state) {
       case STATE_GET_BACKEND:
         DCHECK_EQ(OK, rv);
@@ -879,10 +880,12 @@ int HttpCache::Transaction::DoLoop(int result) {
         rv = DoCacheWriteTruncatedResponseComplete(rv);
         break;
       default:
-        NOTREACHED() << "bad state";
+        NOTREACHED() << "bad state " << state;
         rv = ERR_FAILED;
         break;
     }
+    DCHECK(next_state_ != STATE_UNSET) << "Previous state was " << state;
+
   } while (rv != ERR_IO_PENDING && next_state_ != STATE_NONE);
 
   if (rv != ERR_IO_PENDING && !callback_.is_null()) {
@@ -913,6 +916,7 @@ int HttpCache::Transaction::DoGetBackendComplete(int result) {
     if (effective_load_flags_ & LOAD_ONLY_FROM_CACHE) {
       if (effective_load_flags_ & LOAD_BYPASS_CACHE) {
         // The client has asked for nonsense.
+        next_state_ = STATE_NONE;
         return ERR_CACHE_MISS;
       }
       mode_ = READ;
@@ -950,8 +954,10 @@ int HttpCache::Transaction::DoGetBackendComplete(int result) {
 
   // If must use cache, then we must fail.  This can happen for back/forward
   // navigations to a page generated via a form post.
-  if (!(mode_ & READ) && effective_load_flags_ & LOAD_ONLY_FROM_CACHE)
+  if (!(mode_ & READ) && effective_load_flags_ & LOAD_ONLY_FROM_CACHE) {
+    next_state_ = STATE_NONE;
     return ERR_CACHE_MISS;
+  }
 
   if (mode_ == NONE) {
     if (partial_) {
@@ -973,8 +979,10 @@ int HttpCache::Transaction::DoInitEntry() {
   TRACE_EVENT0("io", "HttpCacheTransaction::DoInitEntry");
   DCHECK(!new_entry_);
 
-  if (!cache_.get())
+  if (!cache_.get()) {
+    next_state_ = STATE_NONE;
     return ERR_UNEXPECTED;
+  }
 
   if (mode_ == WRITE) {
     next_state_ = STATE_DOOM_ENTRY;
@@ -1035,6 +1043,7 @@ int HttpCache::Transaction::DoOpenEntryComplete(int result) {
 
   // The entry does not exist, and we are not permitted to create a new entry,
   // so we must fail.
+  next_state_ = STATE_NONE;
   return ERR_CACHE_MISS;
 }
 
@@ -1165,8 +1174,10 @@ int HttpCache::Transaction::DoAddToEntryComplete(int result) {
   }
 
   if (result == ERR_CACHE_LOCK_TIMEOUT) {
-    if (mode_ == READ)
+    if (mode_ == READ) {
+      next_state_ = STATE_NONE;
       return ERR_CACHE_MISS;
+    }
 
     // The cache is busy, bypass it for this transaction.
     mode_ = NONE;
@@ -1180,8 +1191,10 @@ int HttpCache::Transaction::DoAddToEntryComplete(int result) {
 
   open_entry_last_used_ = entry_->disk_entry->GetLastUsed();
 
+  // TODO(jkarlin): We should either handle the case or DCHECK.

[shivanisha, 2017/03/22 14:48:29]

Not sure what the "todo" suggests.

[jkarlin, 2017/03/22 15:04:26]

It's against Chromium style to NOTREACHED/DCHECK if you also handle the case
(e.g., return something) (see the condition below). So the TODO is to either
DCHECK or just handle the condition.

   if (result != OK) {
     NOTREACHED();
+    next_state_ = STATE_NONE;
     return result;
   }
 
@@ -1323,16 +1336,20 @@ int HttpCache::Transaction::DoCacheQueryData() {
 
 int HttpCache::Transaction::DoCacheQueryDataComplete(int result) {
   DCHECK_EQ(OK, result);
-  if (!cache_.get())
+  if (!cache_.get()) {
+    next_state_ = STATE_NONE;
     return ERR_UNEXPECTED;
+  }
 
   return ValidateEntryHeadersAndContinue();

[shivanisha, 2017/03/22 15:19:48]

Since this Do function is setting the state in the helper functions, a dcheck
before return would be good.
rv = ValidateEntryHeadersAndContinue();
DCHECK_NE(next_state_, STATE_UNSET)
return rv;

[jkarlin, 2017/03/22 15:25:02]

Acknowledged.

 }
 
 // We may end up here multiple times for a given request.
 int HttpCache::Transaction::DoStartPartialCacheValidation() {
-  if (mode_ == NONE)
+  if (mode_ == NONE) {
+    next_state_ = STATE_NONE;
     return OK;
+  }
 
   next_state_ = STATE_COMPLETE_PARTIAL_CACHE_VALIDATION;
   return partial_->ShouldValidateCache(entry_->disk_entry, io_callback_);
@@ -1347,11 +1364,14 @@ int HttpCache::Transaction::DoCompletePartialCacheValidation(int result) {
       cache_->DoneReadingFromEntry(entry_, this);
       entry_ = NULL;
     }
+    next_state_ = STATE_NONE;
     return result;
   }
 
-  if (result < 0)
+  if (result < 0) {
+    next_state_ = STATE_NONE;
     return result;
+  }
 
   partial_->PrepareCacheValidation(entry_->disk_entry,
                                    &custom_request_->extra_headers);
@@ -1374,8 +1394,10 @@ int HttpCache::Transaction::DoSendRequest() {
   // Create a network transaction.
   int rv =
       cache_->network_layer_->CreateTransaction(priority_, &network_trans_);
-  if (rv != OK)
+  if (rv != OK) {
+    next_state_ = STATE_NONE;
     return rv;
+  }
   network_trans_->SetBeforeNetworkStartCallback(before_network_start_callback_);
   network_trans_->SetBeforeHeadersSentCallback(before_headers_sent_callback_);
 
@@ -1394,8 +1416,10 @@ int HttpCache::Transaction::DoSendRequest() {
 
 int HttpCache::Transaction::DoSendRequestComplete(int result) {
   TRACE_EVENT0("io", "HttpCacheTransaction::DoSendRequestComplete");
-  if (!cache_.get())
+  if (!cache_.get()) {
+    next_state_ = STATE_NONE;
     return ERR_UNEXPECTED;
+  }
 
   // If we tried to conditionalize the request and failed, we know
   // we won't be reading from the cache after this point.
@@ -1424,6 +1448,7 @@ int HttpCache::Transaction::DoSendRequestComplete(int result) {
     DoneWritingToEntry(true);
   }
 
+  next_state_ = STATE_NONE;
   return result;
 }
 
@@ -1436,8 +1461,10 @@ int HttpCache::Transaction::DoSuccessfulSendRequest() {
   if (new_response->headers->response_code() == 401 ||
       new_response->headers->response_code() == 407) {
     SetAuthResponse(*new_response);
-    if (!reading_)
+    if (!reading_) {
+      next_state_ = STATE_NONE;
       return OK;
+    }
 
     // We initiated a second request the caller doesn't know about. We should be
     // able to authenticate this request because we should have authenticated
@@ -1460,6 +1487,7 @@ int HttpCache::Transaction::DoSuccessfulSendRequest() {
     mode_ = NONE;
     partial_.reset();
     ResetNetworkTransaction();
+    next_state_ = STATE_NONE;
     return ERR_CACHE_AUTH_FAILURE_AFTER_READ;
   }
 
@@ -1515,6 +1543,7 @@ int HttpCache::Transaction::DoSuccessfulSendRequest() {
       (request_->method == "GET" || request_->method == "POST")) {
     // If there is an active entry it may be destroyed with this transaction.
     SetResponse(*new_response_);
+    next_state_ = STATE_NONE;
     return OK;
   }
 
@@ -1637,6 +1666,7 @@ int HttpCache::Transaction::DoOverwriteCachedResponse() {
     DoneWritingToEntry(false);
     mode_ = NONE;
     new_response_ = NULL;
+    next_state_ = STATE_NONE;
     return OK;
   }
 
@@ -1716,11 +1746,14 @@ int HttpCache::Transaction::DoTruncateCachedMetadataComplete(int result) {
 
 int HttpCache::Transaction::DoPartialHeadersReceived() {
   new_response_ = NULL;
-  if (entry_ && !partial_ && entry_->disk_entry->GetDataSize(kMetadataIndex))
-    next_state_ = STATE_CACHE_READ_METADATA;
 
-  if (!partial_)
+  if (!partial_) {
+    if (entry_ && entry_->disk_entry->GetDataSize(kMetadataIndex))
+      next_state_ = STATE_CACHE_READ_METADATA;
+    else
+      next_state_ = STATE_NONE;
     return OK;
+  }
 
   if (reading_) {
     if (network_trans_.get()) {
@@ -1732,6 +1765,9 @@ int HttpCache::Transaction::DoPartialHeadersReceived() {
     // We are about to return the headers for a byte-range request to the user,
     // so let's fix them.
     partial_->FixResponseHeaders(response_.headers.get(), true);
+    next_state_ = STATE_NONE;
+  } else {
+    next_state_ = STATE_NONE;
   }
   return OK;
 }
@@ -1758,6 +1794,7 @@ int HttpCache::Transaction::DoCacheReadMetadataComplete(int result) {
                                     result);
   if (result != response_.metadata->size())
     return OnCacheReadError(result, false);
+  next_state_ = STATE_NONE;
   return OK;
 }
 
@@ -1771,13 +1808,17 @@ int HttpCache::Transaction::DoNetworkReadComplete(int result) {
   TRACE_EVENT0("io", "HttpCacheTransaction::DoNetworkReadComplete");
   DCHECK(mode_ & WRITE || mode_ == NONE);
 
-  if (!cache_.get())
+  if (!cache_.get()) {
+    next_state_ = STATE_NONE;
     return ERR_UNEXPECTED;
+  }
 
   // If there is an error or we aren't saving the data, we are done; just wait
   // until the destructor runs to see if we can keep the data.
-  if (mode_ == NONE || result < 0)
+  if (mode_ == NONE || result < 0) {
+    next_state_ = STATE_NONE;
     return result;
+  }
 
   next_state_ = STATE_CACHE_WRITE_DATA;
   return result;
@@ -1785,8 +1826,11 @@ int HttpCache::Transaction::DoNetworkReadComplete(int result) {
 
 int HttpCache::Transaction::DoCacheReadData() {
   TRACE_EVENT0("io", "HttpCacheTransaction::DoCacheReadData");
-  if (request_->method == "HEAD")
+
+  if (request_->method == "HEAD") {
+    next_state_ = STATE_NONE;
     return 0;
+  }
 
   DCHECK(entry_);
   next_state_ = STATE_CACHE_READ_DATA_COMPLETE;
@@ -1810,8 +1854,10 @@ int HttpCache::Transaction::DoCacheReadDataComplete(int result) {
                                       result);
   }
 
-  if (!cache_.get())
+  if (!cache_.get()) {
+    next_state_ = STATE_NONE;
     return ERR_UNEXPECTED;
+  }
 
   if (partial_) {
     // Partial requests are confusing to report in histograms because they may
@@ -1829,6 +1875,8 @@ int HttpCache::Transaction::DoCacheReadDataComplete(int result) {
   } else {
     return OnCacheReadError(result, false);
   }
+
+  next_state_ = STATE_NONE;
   return result;
 }
 
@@ -1857,8 +1905,10 @@ int HttpCache::Transaction::DoCacheWriteDataComplete(int result) {
                                         result);
     }
   }
-  if (!cache_.get())
+  if (!cache_.get()) {
+    next_state_ = STATE_NONE;
     return ERR_UNEXPECTED;
+  }
 
   if (result != write_len_) {
     DLOG(ERROR) << "failed to write response data to cache";
@@ -1891,6 +1941,7 @@ int HttpCache::Transaction::DoCacheWriteDataComplete(int result) {
     }
   }
 
+  next_state_ = STATE_NONE;
   return result;
 }
 
@@ -1903,6 +1954,7 @@ int HttpCache::Transaction::DoCacheWriteTruncatedResponse() {
 int HttpCache::Transaction::DoCacheWriteTruncatedResponseComplete(int result) {
   TRACE_EVENT0("io", "HttpCacheTransaction::DoCacheWriteTruncatedResponse");
 
+  next_state_ = STATE_NONE;
   return OnWriteResponseInfoToEntryComplete(result);
 }
 
@@ -2036,23 +2088,31 @@ bool HttpCache::Transaction::ShouldPassThrough() {
 
 int HttpCache::Transaction::BeginCacheRead() {
   // We don't support any combination of LOAD_ONLY_FROM_CACHE and byte ranges.
+  // TODO(jkarlin): Either handle this case or DCHECK.
   if (response_.headers->response_code() == 206 || partial_) {
     NOTREACHED();
+    next_state_ = STATE_NONE;
     return ERR_CACHE_MISS;
   }
 
   // We don't have the whole resource.
-  if (truncated_)
+  if (truncated_) {
+    next_state_ = STATE_NONE;
     return ERR_CACHE_MISS;
+  }
 
-  if (RequiresValidation() != VALIDATION_NONE)
+  if (RequiresValidation() != VALIDATION_NONE) {
+    next_state_ = STATE_NONE;
     return ERR_CACHE_MISS;
+  }
 
   if (request_->method == "HEAD")
     FixHeadersForHead();
 
   if (entry_->disk_entry->GetDataSize(kMetadataIndex))
     next_state_ = STATE_CACHE_READ_METADATA;
+  else
+    next_state_ = STATE_NONE;
 
   return OK;
 }
@@ -2573,6 +2633,8 @@ int HttpCache::Transaction::SetupEntryForRead() {
 
   if (entry_->disk_entry->GetDataSize(kMetadataIndex))
     next_state_ = STATE_CACHE_READ_METADATA;
+  else
+    next_state_ = STATE_NONE;
   return OK;
 }
 
@@ -2682,6 +2744,7 @@ int HttpCache::Transaction::OnCacheReadError(int result, bool restart) {
     return OK;
   }
 
+  next_state_ = STATE_NONE;
   return ERR_CACHE_READ_FAILURE;
 }
 
@@ -2717,6 +2780,8 @@ int HttpCache::Transaction::DoPartialNetworkReadCompleted(int result) {
     // We need to move on to the next range.
     ResetNetworkTransaction();
     next_state_ = STATE_START_PARTIAL_CACHE_VALIDATION;
+  } else {
+    next_state_ = STATE_NONE;
   }
   return result;
 }
@@ -2729,6 +2794,8 @@ int HttpCache::Transaction::DoPartialCacheReadCompleted(int result) {
     next_state_ = STATE_START_PARTIAL_CACHE_VALIDATION;
   } else if (result < 0) {
     return OnCacheReadError(result, false);
+  } else {
+    next_state_ = STATE_NONE;
   }
   return result;
 }