Http cache: Make sure that issuing a byte range request

net/http/http_cache_transaction.cc

Index: net/http/http_cache_transaction.cc
===================================================================
--- net/http/http_cache_transaction.cc	(revision 148620)
+++ net/http/http_cache_transaction.cc	(working copy)
@@ -1573,15 +1573,22 @@
   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;
@@ -1594,7 +1601,9 @@
     // 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;
     }
@@ -1631,13 +1640,7 @@
 
   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)
@@ -1877,12 +1880,19 @@
       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
@@ -2079,6 +2089,16 @@
   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:
 //