Http cache: Fix the code that handles 206s when revalidating

a range from the cache.

BUG=12258
TEST=unittests

Review URL: http://codereview.chromium.org/174039

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@23881 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 91 insertions, 22 deletions

diff --git a/net/http/http_cache.cc b/net/http/http_cache.cc
index 23c462e..4de8080 100644
--- a/net/http/http_cache.cc
+++ b/net/http/http_cache.cc
@@ -1128,6 +1128,23 @@ bool HttpCache::Transaction::ConditionalizeRequest() {
   return true;
 }
 
+// We just received some headers from the server. We may have asked for a range,
+// in which case partial_ has an object. This could be the first network request
+// we make to fulfill the original request, or we may be already reading (from
+// the net and / or the cache). If we are not expecting a certain response, we
+// just bypass the cache for this request (but again, maybe we are reading), and
+// delete partial_ (so we are not able to "fix" the headers that we return to
+// the user). This results in either a weird response for the caller (we don't
+// expect it after all), or maybe a range that was not exactly what it was asked
+// for.
+//
+// For example, if the original request is for 30KB and we have the last 20KB,
+// we ask the server for the first 10KB. If the resourse has changed, we'll
+// end up forwarding the 200 back to the user (so far so good). However, if
+// we have instead the first 10KB, we end up sending back a byte range response
+// for the first 10KB, because we never asked the server for the last part. It's
+// just too complicated to restart the whole request from this point; and of
+// course, maybe we already returned the headers.
 bool HttpCache::Transaction::ValidatePartialResponse(
     const HttpResponseHeaders* headers) {
     int response_code = headers->response_code();
@@ -1150,30 +1167,38 @@ bool HttpCache::Transaction::ValidatePartialResponse(
     return false;
   }
 
-  // TODO(rvargas): Handle 206 when the current range is already cached.
-  bool failure = false;
-  if (!partial_content) {
-    if (!partial_.get())
-      return false;
+  if (!partial_.get()) {
+    // We are not expecting 206 but we may have one.
+    if (partial_content)
+      IgnoreRangeRequest();
 
-    // TODO(rvargas): Do we need to consider other results here?.
-    if (response_code == 200 || response_code == 416)
-      failure = true;
+    return false;
+  }
 
-    if (!reading_ && failure) {
-      // We are expecting 206 or 304 because we asked for a range. Given that
-      // the server is refusing the request we'll remove the sparse entry.
-      DoomPartialEntry(true);
-      mode_ = NONE;
-      return false;
-    }
+  // TODO(rvargas): Do we need to consider other results here?.
+  bool failure = response_code == 200 || response_code == 416;
+
+  if (partial_->IsCurrentRangeCached()) {
+    // We asked for "If-None-Match: " so a 206 means a new object.
+    if (partial_content)
+      failure = true;
 
     if (response_code == 304 && partial_->ResponseHeadersOK(headers))
       return false;
+  } else {
+    // We asked for "If-Range: " so a 206 means just another range.
+    if (partial_content && partial_->ResponseHeadersOK(headers))
+      return true;
+
+    // 304 is not expected here, but we'll spare the entry.
   }
 
-  if (!failure && partial_.get() && partial_->ResponseHeadersOK(headers))
-    return true;
+  if (failure) {
+    // We cannot truncate this entry, it has to be deleted.
+    DoomPartialEntry(true);
+    mode_ = NONE;
+    return false;
+  }
 
   IgnoreRangeRequest();
   return false;
@@ -1352,7 +1377,10 @@ void HttpCache::Transaction::OnNetworkInfoAvailable(int result) {
           if (response_.headers->HasHeaderValue("cache-control", "no-store")) {
             cache_->DoomEntry(cache_key_);
           } else {
-            WriteResponseInfoToEntry();
+            // If we are already reading, we already updated the headers for
+            // this request; doing it again will change Content-Length.
+            if (!reading_)
+              WriteResponseInfoToEntry();
           }
 
           if (mode_ == UPDATE) {
diff --git a/net/http/http_cache_unittest.cc b/net/http/http_cache_unittest.cc
index 96118d8..14299b0 100644
--- a/net/http/http_cache_unittest.cc
+++ b/net/http/http_cache_unittest.cc
@@ -465,13 +465,19 @@ class RangeTransactionServer {
  public:
   RangeTransactionServer() {
     not_modified_ = false;
+    modified_ = false;
   }
   ~RangeTransactionServer() {
     not_modified_ = false;
+    modified_ = false;
   }
 
+  // Returns only 416 or 304 when set.
   void set_not_modified(bool value) { not_modified_ = value; }
 
+  // Returns 206 when revalidating a range (instead of 304).
+  void set_modified(bool value) { modified_ = value; }
+
   static void RangeHandler(const net::HttpRequestInfo* request,
                            std::string* response_status,
                            std::string* response_headers,
@@ -479,9 +485,11 @@ class RangeTransactionServer {
 
  private:
   static bool not_modified_;
+  static bool modified_;
   DISALLOW_COPY_AND_ASSIGN(RangeTransactionServer);
 };
 bool RangeTransactionServer::not_modified_ = false;
+bool RangeTransactionServer::modified_ = false;
 
 // Static.
 void RangeTransactionServer::RangeHandler(const net::HttpRequestInfo* request,
@@ -519,7 +527,8 @@ void RangeTransactionServer::RangeHandler(const net::HttpRequestInfo* request,
                                            start, end);
   response_headers->append(content_range);
 
-  if (request->extra_headers.find("If-None-Match") == std::string::npos) {
+  if (request->extra_headers.find("If-None-Match") == std::string::npos ||
+      modified_) {
     EXPECT_EQ(9, (end - start) % 10);
     std::string data;
     for (int block_start = start; block_start < end; block_start += 10)
@@ -1766,6 +1775,41 @@ TEST(HttpCache, DISABLED_RangeGET_304) {
   RemoveMockTransaction(&kRangeGET_TransactionOK);
 }
 
+// Tests that we deal with 206s when revalidating range requests.
+TEST(HttpCache, DISABLED_RangeGET_ModifiedResult) {
+  MockHttpCache cache;
+  AddMockTransaction(&kRangeGET_TransactionOK);
+  std::string headers;
+
+  // Write to the cache (40-49).
+  RunTransactionTestWithResponse(cache.http_cache(), kRangeGET_TransactionOK,
+                                 &headers);
+
+  EXPECT_TRUE(Verify206Response(headers, 40, 49));
+  EXPECT_EQ(1, cache.network_layer()->transaction_count());
+  EXPECT_EQ(0, cache.disk_cache()->open_count());
+  EXPECT_EQ(1, cache.disk_cache()->create_count());
+
+  // Attempt to read from the cache (40-49).
+  RangeTransactionServer handler;
+  handler.set_modified(true);
+  RunTransactionTestWithResponse(cache.http_cache(), kRangeGET_TransactionOK,
+                                 &headers);
+
+  EXPECT_TRUE(Verify206Response(headers, 40, 49));
+  EXPECT_EQ(2, cache.network_layer()->transaction_count());
+  EXPECT_EQ(1, cache.disk_cache()->open_count());
+  EXPECT_EQ(1, cache.disk_cache()->create_count());
+
+  // And the entry should be gone.
+  RunTransactionTest(cache.http_cache(), kRangeGET_TransactionOK);
+  EXPECT_EQ(3, cache.network_layer()->transaction_count());
+  EXPECT_EQ(1, cache.disk_cache()->open_count());
+  EXPECT_EQ(2, cache.disk_cache()->create_count());
+
+  RemoveMockTransaction(&kRangeGET_TransactionOK);
+}
+
 // Tests that we can cache range requests when the start or end is unknown.
 // We start with one suffix request, followed by a request from a given point.
 TEST(HttpCache, DISABLED_UnknownRangeGET_1) {
diff --git a/net/http/partial_data.cc b/net/http/partial_data.cc
index d24d6fa..70d7d18 100644
--- a/net/http/partial_data.cc
+++ b/net/http/partial_data.cc
@@ -258,9 +258,6 @@ void PartialData::OnCacheReadCompleted(int result) {
     current_range_start_ += result;
     cached_min_len_ -= result;
     DCHECK(cached_min_len_ >= 0);
-  } else if (!result) {
-    // TODO(rvargas): we can detect this error and make sure that we are not
-    // in a loop of failure/retry.
   }
 }