Http cache: Avoid resuming (and keeping) truncated entries

if the server doesn't provide a strong validator.

We require:
- A strong etag or a strong last modified date.
- The total Content length.
- Lack of an explicit rejection of ranges (Accept-ranges: none) 

This aligns better with the conditions used by Firefox.

BUG=30220, b/2329250
TEST=unittests

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

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

6 files changed, 264 insertions, 17 deletions

diff --git a/net/http/http_cache_transaction.cc b/net/http/http_cache_transaction.cc
index b829623..32808e8 100644
--- a/net/http/http_cache_transaction.cc
+++ b/net/http/http_cache_transaction.cc
@@ -548,6 +548,11 @@ bool HttpCache::Transaction::AddTruncatedFlag() {
   if (!entry_->disk_entry->GetDataSize(kResponseContentIndex))
     return false;
 
+  if (response_.headers->GetContentLength() <= 0 ||
+      response_.headers->HasHeaderValue("Accept-Ranges", "none") ||
+      !response_.headers->HasStrongValidators())
+    return false;
+
   truncated_ = true;
   target_state_ = STATE_NONE;
   next_state_ = STATE_CACHE_WRITE_TRUNCATED_RESPONSE;
@@ -1142,7 +1147,7 @@ bool HttpCache::Transaction::ConditionalizeRequest() {
     custom_request_->extra_headers.append("\r\n");
     // For byte-range requests, make sure that we use only one way to validate
     // the request.
-    if (partial_.get())
+    if (partial_.get() && !partial_->IsCurrentRangeCached())
       return true;
   }
 
@@ -1224,7 +1229,10 @@ bool HttpCache::Transaction::ValidatePartialResponse(
       return true;
     }
 
-    // 304 is not expected here, but we'll spare the entry.
+    // 304 is not expected here, but we'll spare the entry (unless it was
+    // truncated).
+    if (truncated_)
+      failure = true;
   }
 
   if (failure) {
diff --git a/net/http/http_cache_unittest.cc b/net/http/http_cache_unittest.cc
index 71aceca..8a69c65 100644
--- a/net/http/http_cache_unittest.cc
+++ b/net/http/http_cache_unittest.cc
@@ -731,9 +731,6 @@ void RangeTransactionServer::RangeHandler(const net::HttpRequestInfo* request,
       response_status->assign("HTTP/1.1 200 Success");
     }
   } else {
-    // Check that we use only one validation header.
-    EXPECT_EQ(std::string::npos,
-              request->extra_headers.find("If-Modified-Since"));
     response_status->assign("HTTP/1.1 304 Not Modified");
     response_data->clear();
   }
@@ -3266,12 +3263,101 @@ TEST(HttpCache, DoomOnDestruction) {
   EXPECT_EQ(2, cache.disk_cache()->create_count());
 }
 
+// Tests that we delete an entry when the request is cancelled if the response
+// does not have content-length and strong validators.
+TEST(HttpCache, DoomOnDestruction2) {
+  MockHttpCache cache;
+  cache.http_cache()->set_enable_range_support(true);
+
+  MockHttpRequest request(kSimpleGET_Transaction);
+
+  Context* c = new Context();
+  int rv = cache.http_cache()->CreateTransaction(&c->trans);
+  EXPECT_EQ(net::OK, rv);
+
+  rv = c->trans->Start(&request, &c->callback, NULL);
+  if (rv == net::ERR_IO_PENDING)
+    rv = c->callback.WaitForResult();
+
+  EXPECT_EQ(1, cache.network_layer()->transaction_count());
+  EXPECT_EQ(0, cache.disk_cache()->open_count());
+  EXPECT_EQ(1, cache.disk_cache()->create_count());
+
+  // Make sure that the entry has some data stored.
+  scoped_refptr<net::IOBufferWithSize> buf = new net::IOBufferWithSize(10);
+  rv = c->trans->Read(buf, buf->size(), &c->callback);
+  if (rv == net::ERR_IO_PENDING)
+    rv = c->callback.WaitForResult();
+  EXPECT_EQ(buf->size(), rv);
+
+  // Destroy the transaction.
+  delete c;
+
+  RunTransactionTest(cache.http_cache(), kSimpleGET_Transaction);
+
+  EXPECT_EQ(2, cache.network_layer()->transaction_count());
+  EXPECT_EQ(0, cache.disk_cache()->open_count());
+  EXPECT_EQ(2, cache.disk_cache()->create_count());
+}
+
+// Tests that we delete an entry when the request is cancelled if the response
+// has an "Accept-Ranges: none" header.
+TEST(HttpCache, DoomOnDestruction3) {
+  MockHttpCache cache;
+  cache.http_cache()->set_enable_range_support(true);
+
+  MockTransaction transaction(kSimpleGET_Transaction);
+  transaction.response_headers =
+      "Last-Modified: Wed, 28 Nov 2007 00:40:09 GMT\n"
+      "Content-Length: 22\n"
+      "Accept-Ranges: none\n"
+      "Etag: foopy\n";
+  AddMockTransaction(&transaction);
+  MockHttpRequest request(transaction);
+
+  Context* c = new Context();
+  int rv = cache.http_cache()->CreateTransaction(&c->trans);
+  EXPECT_EQ(net::OK, rv);
+
+  rv = c->trans->Start(&request, &c->callback, NULL);
+  if (rv == net::ERR_IO_PENDING)
+    rv = c->callback.WaitForResult();
+
+  EXPECT_EQ(1, cache.network_layer()->transaction_count());
+  EXPECT_EQ(0, cache.disk_cache()->open_count());
+  EXPECT_EQ(1, cache.disk_cache()->create_count());
+
+  // Make sure that the entry has some data stored.
+  scoped_refptr<net::IOBufferWithSize> buf = new net::IOBufferWithSize(10);
+  rv = c->trans->Read(buf, buf->size(), &c->callback);
+  if (rv == net::ERR_IO_PENDING)
+    rv = c->callback.WaitForResult();
+  EXPECT_EQ(buf->size(), rv);
+
+  // Destroy the transaction.
+  delete c;
+
+  RunTransactionTest(cache.http_cache(), kSimpleGET_Transaction);
+
+  EXPECT_EQ(2, cache.network_layer()->transaction_count());
+  EXPECT_EQ(0, cache.disk_cache()->open_count());
+  EXPECT_EQ(2, cache.disk_cache()->create_count());
+
+  RemoveMockTransaction(&transaction);
+}
+
 // Tests that we mark an entry as incomplete when the request is cancelled.
 TEST(HttpCache, Set_Truncated_Flag) {
   MockHttpCache cache;
   cache.http_cache()->set_enable_range_support(true);
 
-  MockHttpRequest request(kSimpleGET_Transaction);
+  MockTransaction transaction(kSimpleGET_Transaction);
+  transaction.response_headers =
+      "Last-Modified: Wed, 28 Nov 2007 00:40:09 GMT\n"
+      "Content-Length: 22\n"
+      "Etag: foopy\n";
+  AddMockTransaction(&transaction);
+  MockHttpRequest request(transaction);
 
   Context* c = new Context();
   int rv = cache.http_cache()->CreateTransaction(&c->trans);
@@ -3304,6 +3390,8 @@ TEST(HttpCache, Set_Truncated_Flag) {
   EXPECT_TRUE(net::HttpCache::ReadResponseInfo(entry, &response, &truncated));
   EXPECT_TRUE(truncated);
   entry->Close();
+
+  RemoveMockTransaction(&transaction);
 }
 
 // Tests that we can continue with a request that was interrupted.
@@ -3317,12 +3405,11 @@ TEST(HttpCache, GET_IncompleteResource) {
   ASSERT_TRUE(cache.disk_cache()->CreateEntry(kRangeGET_TransactionOK.url,
                                               &entry));
 
-  // Content-length will be intentionally bogus.
   std::string raw_headers("HTTP/1.1 200 OK\n"
-                          "Last-Modified: something\n"
+                          "Last-Modified: Sat, 18 Apr 2009 01:10:43 GMT\n"
                           "ETag: \"foo\"\n"
                           "Accept-Ranges: bytes\n"
-                          "Content-Length: 10\n");
+                          "Content-Length: 80\n");
   raw_headers = net::HttpUtil::AssembleRawHeaders(raw_headers.data(),
                                                   raw_headers.size());
 
@@ -3350,7 +3437,7 @@ TEST(HttpCache, GET_IncompleteResource) {
       "Last-Modified: Sat, 18 Apr 2009 01:10:43 GMT\n"
       "Accept-Ranges: bytes\n"
       "ETag: \"foo\"\n"
-      "Content-Length: 10\n");
+      "Content-Length: 80\n");
 
   EXPECT_EQ(expected_headers, headers);
   EXPECT_EQ(2, cache.network_layer()->transaction_count());
@@ -3367,6 +3454,64 @@ TEST(HttpCache, GET_IncompleteResource) {
   entry->Close();
 }
 
+// Tests that we delete truncated entries if the server changes its mind midway.
+TEST(HttpCache, GET_IncompleteResource2) {
+  MockHttpCache cache;
+  cache.http_cache()->set_enable_range_support(true);
+  AddMockTransaction(&kRangeGET_TransactionOK);
+
+  // Create a disk cache entry that stores an incomplete resource.
+  disk_cache::Entry* entry;
+  ASSERT_TRUE(cache.disk_cache()->CreateEntry(kRangeGET_TransactionOK.url,
+                                              &entry));
+
+  // Content-length will be intentionally bad.
+  std::string raw_headers("HTTP/1.1 200 OK\n"
+                          "Last-Modified: Sat, 18 Apr 2009 01:10:43 GMT\n"
+                          "ETag: \"foo\"\n"
+                          "Accept-Ranges: bytes\n"
+                          "Content-Length: 50\n");
+  raw_headers = net::HttpUtil::AssembleRawHeaders(raw_headers.data(),
+                                                  raw_headers.size());
+
+  net::HttpResponseInfo response;
+  response.headers = new net::HttpResponseHeaders(raw_headers);
+  // Set the last argument for this to be an incomplete request.
+  EXPECT_TRUE(net::HttpCache::WriteResponseInfo(entry, &response, true, true));
+
+  scoped_refptr<net::IOBuffer> buf(new net::IOBuffer(100));
+  int len = static_cast<int>(base::strlcpy(buf->data(),
+                                           "rg: 00-09 rg: 10-19 ", 100));
+  EXPECT_EQ(len, entry->WriteData(1, 0, buf, len, NULL, true));
+  entry->Close();
+
+  // Now make a regular request.
+  std::string headers;
+  MockTransaction transaction(kRangeGET_TransactionOK);
+  transaction.request_headers = EXTRA_HEADER;
+  transaction.data = "rg: 00-09 rg: 10-19 ";
+  RunTransactionTestWithResponse(cache.http_cache(), transaction, &headers);
+
+  // We update the headers with the ones received while revalidating.
+  std::string expected_headers(
+      "HTTP/1.1 200 OK\n"
+      "Last-Modified: Sat, 18 Apr 2009 01:10:43 GMT\n"
+      "Accept-Ranges: bytes\n"
+      "ETag: \"foo\"\n"
+      "Content-Length: 50\n");
+
+  EXPECT_EQ(expected_headers, headers);
+  EXPECT_EQ(2, cache.network_layer()->transaction_count());
+  EXPECT_EQ(1, cache.disk_cache()->open_count());
+  EXPECT_EQ(1, cache.disk_cache()->create_count());
+
+  RemoveMockTransaction(&kRangeGET_TransactionOK);
+
+  // Verify that the disk entry was deleted.
+  EXPECT_FALSE(cache.disk_cache()->OpenEntry(kRangeGET_TransactionOK.url,
+                                             &entry));
+}
+
 // Tests that when we cancel a request that was interrupted, we mark it again
 // as truncated.
 TEST(HttpCache, GET_CancelIncompleteResource) {
@@ -3379,12 +3524,11 @@ TEST(HttpCache, GET_CancelIncompleteResource) {
   ASSERT_TRUE(cache.disk_cache()->CreateEntry(kRangeGET_TransactionOK.url,
                                               &entry));
 
-  // Content-length will be intentionally bogus.
   std::string raw_headers("HTTP/1.1 200 OK\n"
-                          "Last-Modified: something\n"
+                          "Last-Modified: Sat, 18 Apr 2009 01:10:43 GMT\n"
                           "ETag: \"foo\"\n"
                           "Accept-Ranges: bytes\n"
-                          "Content-Length: 10\n");
+                          "Content-Length: 80\n");
   raw_headers = net::HttpUtil::AssembleRawHeaders(raw_headers.data(),
                                                   raw_headers.size());
 
diff --git a/net/http/http_response_headers.cc b/net/http/http_response_headers.cc
index e342d3b..82272bb 100644
--- a/net/http/http_response_headers.cc
+++ b/net/http/http_response_headers.cc
@@ -1001,6 +1001,32 @@ bool HttpResponseHeaders::IsKeepAlive() const {
   return keep_alive;
 }
 
+bool HttpResponseHeaders::HasStrongValidators() const {
+  std::string etag_value;
+  EnumerateHeader(NULL, "etag", &etag_value);
+  if (!etag_value.empty()) {
+    size_t slash = etag_value.find('/');
+    if (slash == std::string::npos || slash == 0)
+      return true;
+
+    std::string::const_iterator i = etag_value.begin();
+    std::string::const_iterator j = etag_value.begin() + slash;
+    HttpUtil::TrimLWS(&i, &j);
+    if (!LowerCaseEqualsASCII(i, j, "w"))
+      return true;
+  }
+
+  Time last_modified;
+  if (!GetLastModifiedValue(&last_modified))
+    return false;
+
+  Time date;
+  if (!GetDateValue(&date))
+    return false;
+
+  return ((date - last_modified).InSeconds() >= 60);
+}
+
 // From RFC 2616:
 // Content-Length = "Content-Length" ":" 1*DIGIT
 int64 HttpResponseHeaders::GetContentLength() const {
diff --git a/net/http/http_response_headers.h b/net/http/http_response_headers.h
index c071389..f6656e3 100644
--- a/net/http/http_response_headers.h
+++ b/net/http/http_response_headers.h
@@ -211,6 +211,10 @@ class HttpResponseHeaders
   // Determines if this response indicates a keep-alive connection.
   bool IsKeepAlive() const;
 
+  // Returns true if this response has a strong etag or last-modified header.
+  // See section 13.3.3 of RFC 2616.
+  bool HasStrongValidators() const;
+
   // Extracts the value of the Content-Length header or returns -1 if there is
   // no such header in the response.
   int64 GetContentLength() const;
diff --git a/net/http/http_response_headers_unittest.cc b/net/http/http_response_headers_unittest.cc
index d2ff378..bea4f36 100644
--- a/net/http/http_response_headers_unittest.cc
+++ b/net/http/http_response_headers_unittest.cc
@@ -1427,6 +1427,60 @@ TEST(HttpResponseHeadersTest, IsKeepAlive) {
   }
 }
 
+TEST(HttpResponseHeadersTest, HasStrongValidators) {
+  const struct {
+    const char* headers;
+    bool expected_result;
+  } tests[] = {
+    { "HTTP/0.9 200 OK",
+      false
+    },
+    { "HTTP/0.9 200 OK\n"
+      "Date: Wed, 28 Nov 2007 01:40:10 GMT\n"
+      "Last-Modified: Wed, 28 Nov 2007 00:40:10 GMT\n"
+      "ETag: \"foo\"\n",
+      true
+    },
+    { "HTTP/1.1 200 OK\n"
+      "Date: Wed, 28 Nov 2007 00:41:10 GMT\n"
+      "Last-Modified: Wed, 28 Nov 2007 00:40:10 GMT\n",
+      true
+    },
+    { "HTTP/1.1 200 OK\n"
+      "Date: Wed, 28 Nov 2007 00:41:09 GMT\n"
+      "Last-Modified: Wed, 28 Nov 2007 00:40:10 GMT\n",
+      false
+    },
+    { "HTTP/1.1 200 OK\n"
+      "ETag: \"foo\"\n",
+      true
+    },
+    // This is not really a weak etag:
+    { "HTTP/1.1 200 OK\n"
+      "etag: \"w/foo\"\n",
+      true
+    },
+    // This is a weak etag:
+    { "HTTP/1.1 200 OK\n"
+      "etag: w/\"foo\"\n",
+      false
+    },
+    { "HTTP/1.1 200 OK\n"
+      "etag:    W  /   \"foo\"\n",
+      false
+    }
+  };
+  for (size_t i = 0; i < ARRAYSIZE_UNSAFE(tests); ++i) {
+    string headers(tests[i].headers);
+    HeadersToRaw(&headers);
+    scoped_refptr<HttpResponseHeaders> parsed =
+        new HttpResponseHeaders(headers);
+
+    EXPECT_EQ(tests[i].expected_result, parsed->HasStrongValidators()) <<
+      "Failed test case " << i;
+  }
+}
+
 TEST(HttpResponseHeadersTest, GetStatusText) {
   std::string headers("HTTP/1.1 404 Not Found");
   HeadersToRaw(&headers);
diff --git a/net/http/partial_data.cc b/net/http/partial_data.cc
index 0ef7f6b..f727699 100644
--- a/net/http/partial_data.cc
+++ b/net/http/partial_data.cc
@@ -130,9 +130,16 @@ bool PartialData::UpdateFromStoredHeaders(const HttpResponseHeaders* headers,
     if (byte_range_.IsValid())
       return false;
 
+    // Now we avoid resume if there is no content length, but that was not
+    // always the case so double check here.
+    int64 total_length = headers->GetContentLength();
+    if (total_length <= 0 || !headers->HasStrongValidators())
+      return false;
+
     truncated_ = true;
     sparse_entry_ = false;
     byte_range_.set_first_byte_position(entry->GetDataSize(kDataStream));
+    resource_size_ = total_length;
     current_range_start_ = 0;
     return true;
   }
@@ -144,12 +151,11 @@ bool PartialData::UpdateFromStoredHeaders(const HttpResponseHeaders* headers,
     return true;
   }
 
-  std::string length_value;
-  if (!headers->GetNormalizedHeader(kLengthHeader, &length_value))
+  int64 length_value = headers->GetContentLength();
+  if (length_value <= 0)
     return false;  // We must have stored the resource length.
 
-  if (!StringToInt64(length_value, &resource_size_) || !resource_size_)
-    return false;
+  resource_size_ = length_value;
 
   // Make sure that this is really a sparse entry.
   int64 n;
@@ -214,6 +220,11 @@ bool PartialData::ResponseHeadersOK(const HttpResponseHeaders* headers) {
     return false;
   }
 
+  if (truncated_) {
+    if (!byte_range_.HasLastBytePosition())
+      byte_range_.set_last_byte_position(end);
+  }
+
   if (start != current_range_start_)
     return false;