Http cache: Make sure that issuing a byte range request

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"
 
 #if defined(OS_POSIX)
 #include <unistd.h>
@@ skipping 1554 matching lines @@
 }
 
 int HttpCache::Transaction::BeginCacheValidation() {
   DCHECK(mode_ == READ_WRITE);
 
   bool skip_validation = effective_load_flags_ & LOAD_PREFERRING_CACHE ||
                          !RequiresValidation();
 
   if (truncated_)
     skip_validation = !partial_->initial_validation();
 

[gavinp, 2012/07/30 18:35:07]

Is it worthwhile adding a DCHECK here that captures having called
partial_->PrepareCacheValidation()... here? I guess not, since
...::BeginCacheValidation() is only ever called from
DoCompletePartialCacheValidation() or BeginPartialCacheValidation(). But I am a
big freaked out by that dependency, I wish we could DCHECK.

-  if ((partial_.get() && !partial_->IsCurrentRangeCached()) || invalid_range_)
+  if (partial_.get() && (is_sparse_ || truncated_) &&
+      (!partial_->IsCurrentRangeCached() || invalid_range_)) {
+    // Only force revalidation for sparse or truncated entries.

[gavinp, 2012/07/30 18:35:07]

I think the "Only" here is a bit confusing: skip_validation can be false for
other reasons, right, such as net::HttpCache::PLAYBACK?

Separately, I think we still skip validation if the request is for a range that
we have cached.. 

So I propose:

// Force revalidation for sparse or truncated entries if we aren't requesting
from the cached range.

[rvargas (doing something else), 2012/07/30 21:38:52]

but this is not about skip_validation being false, but about ignoring the
regular logic for that and deciding to check anyway.

Updated the comment.

     skip_validation = false;
+  }
 
   if (skip_validation) {
     if (partial_.get()) {
-      // We are going to return the saved response headers to the caller, so
-      // we may need to adjust them first.
-      next_state_ = STATE_PARTIAL_HEADERS_RECEIVED;
-      return OK;
+      if (truncated_ || is_sparse_ || !invalid_range_) {
+        // We are going to return the saved response headers to the caller, so
+        // we may need to adjust them first.
+        next_state_ = STATE_PARTIAL_HEADERS_RECEIVED;
+        return OK;
+      } else {
+        partial_.reset();
+      }
     }
     cache_->ConvertWriterToReader(entry_);
     mode_ = READ;
 
     if (entry_->disk_entry->GetDataSize(kMetadataIndex))
       next_state_ = STATE_CACHE_READ_METADATA;
   } else {
     // Make the network request conditional, to see if we may reuse our cached
     // response.  If we cannot do so, then we just resort to a normal fetch.
     // Our mode remains READ_WRITE for a conditional request.  We'll switch to
     // either READ or WRITE mode once we hear back from the server.
     if (!ConditionalizeRequest()) {
-      DCHECK(!partial_.get());
+      if (partial_.get())
+        return DoRestartPartialRequest();

[gavinp, 2012/07/30 18:35:07]

So this makes me sad because if we have a range cached, and issue a partial
request that includes that range, we just ignore the range we do have cached. Is
that right?

[rvargas (doing something else), 2012/07/30 21:38:52]

Not unless we are unable to conditionalize the request.

We used to discard the cached copy as soon as the strong validator check failed
(when asked for a range). Now we wait to see if we actually have to issue a
conditional request.

+
       DCHECK_NE(206, response_.headers->response_code());
       mode_ = WRITE;
     }
     next_state_ = STATE_SEND_REQUEST;
   }
   return OK;
 }
 
 int HttpCache::Transaction::BeginPartialCacheValidation() {
   DCHECK(mode_ == READ_WRITE);
@@ skipping 16 matching lines @@
 
   return ValidateEntryHeadersAndContinue();
 }
 
 // This should only be called once per request.
 int HttpCache::Transaction::ValidateEntryHeadersAndContinue() {
   DCHECK(mode_ == READ_WRITE);
 
   if (!partial_->UpdateFromStoredHeaders(response_.headers, entry_->disk_entry,
                                          truncated_)) {
-    // The stored data cannot be used. Get rid of it and restart this request.
-    // We need to also reset the |truncated_| flag as a new entry is created.
-    DoomPartialEntry(!range_requested_);
-    mode_ = WRITE;
-    truncated_ = false;
-    next_state_ = STATE_INIT_ENTRY;
-    return OK;
+    return DoRestartPartialRequest();
   }
 
   if (response_.headers->response_code() == 206)
     is_sparse_ = true;
 
   if (!partial_->IsRequestedRangeOK()) {
     // The stored data is fine, but the request may be invalid.
     invalid_range_ = true;
   }
 
@@ skipping 219 matching lines @@
 
     if (response_code == 304 && partial_->ResponseHeadersOK(headers))
       return true;
   } else {
     // We asked for "If-Range: " so a 206 means just another range.
     if (partial_response && partial_->ResponseHeadersOK(headers)) {
       handling_206_ = true;
       return true;
     }
 
-    if (response_code == 200 && !reading_ && !is_sparse_) {
-      // The server is sending the whole resource, and we can save it.
-      DCHECK((truncated_ && !partial_->IsLastRange()) || range_requested_);
-      partial_.reset();
-      truncated_ = false;
-      return true;
+    if (!reading_ && !is_sparse_ && !partial_response) {
+      // See if we can ignore the fact that we issued a byte range request.
+      // If the server sends 200, just store it. If it sends an error, redirect
+      // or something else, we may store the response as long as we didn't have
+      // anything already stored.
+      if (response_code == 200 ||
+          (!truncated_ && response_code != 304 && response_code != 416)) {
+        // The server is sending something else, and we can save it.
+        DCHECK((truncated_ && !partial_->IsLastRange()) || range_requested_);
+        partial_.reset();
+        truncated_ = false;
+        return true;
+      }
     }
 
     // 304 is not expected here, but we'll spare the entry (unless it was
     // truncated).
     if (truncated_)
       failure = true;
   }
 
   if (failure) {
     // We cannot truncate this entry, it has to be deleted.
@@ skipping 176 matching lines @@
 
   if (result == 0 && mode_ == READ_WRITE) {
     // We need to move on to the next range.
     next_state_ = STATE_START_PARTIAL_CACHE_VALIDATION;
   } else if (result < 0) {
     return OnCacheReadError(result, false);
   }
   return result;
 }
 
+int HttpCache::Transaction::DoRestartPartialRequest() {
+  // The stored data cannot be used. Get rid of it and restart this request.
+  // We need to also reset the |truncated_| flag as a new entry is created.
+  DoomPartialEntry(!range_requested_);
+  mode_ = WRITE;
+  truncated_ = false;
+  next_state_ = STATE_INIT_ENTRY;
+  return OK;
+}
+
 // Histogram data from the end of 2010 show the following distribution of
 // response headers:
 //
 //   Content-Length............... 87%
 //   Date......................... 98%
 //   Last-Modified................ 49%
 //   Etag......................... 19%
 //   Accept-Ranges: bytes......... 25%
 //   Accept-Ranges: none.......... 0.4%
 //   Strong Validator............. 50%
@@ skipping 14 matching lines @@
     return false;
 
   return true;
 }
 
 void HttpCache::Transaction::OnIOComplete(int result) {
   DoLoop(result);
 }
 
 }  // namespace net