net: Don't store byte-range resources that cannot be revalidated.

This is basically a revert of
https://crrev.com/439a05cd87fe1f880b896bd7b154454299355899
Cr-Commit-Position: refs/heads/master@{#292751}

The main purpose of said change was to enable code to improve seek operations
when the server is not well configured (think videos without the proper
caching headers). That requires at least a load flag to specify the intent.
However, we can wait until there's actually an attempt to implement that code
before we start saving everything.

The downside from saving these resources is that they are basically unused so
they just take space and processing time. They could be used if they have an
expiration date but lack validators, but that combination is probably very
uncommon.

BUG=407923, 442318
TEST=net_unittests
R=davidben@chromium.org

Review URL: https://codereview.chromium.org/834213003

Cr-Commit-Position: refs/heads/master@{#310406}

8 files changed, 72 insertions, 40 deletions

diff --git a/net/http/http_cache.cc b/net/http/http_cache.cc
index a34bef1..a071baa 100644
--- a/net/http/http_cache.cc
+++ b/net/http/http_cache.cc
@@ -454,6 +454,7 @@ HttpCache::HttpCache(const net::HttpNetworkSession::Params& params,
       backend_factory_(backend_factory),
       building_backend_(false),
       bypass_lock_for_test_(false),
+      fail_conditionalization_for_test_(false),
       use_stale_while_revalidate_(params.use_stale_while_revalidate),
       mode_(NORMAL),
       network_layer_(new HttpNetworkLayer(new HttpNetworkSession(params))),
@@ -470,6 +471,7 @@ HttpCache::HttpCache(HttpNetworkSession* session,
       backend_factory_(backend_factory),
       building_backend_(false),
       bypass_lock_for_test_(false),
+      fail_conditionalization_for_test_(false),
       use_stale_while_revalidate_(session->params().use_stale_while_revalidate),
       mode_(NORMAL),
       network_layer_(new HttpNetworkLayer(session)),
@@ -483,6 +485,7 @@ HttpCache::HttpCache(HttpTransactionFactory* network_layer,
       backend_factory_(backend_factory),
       building_backend_(false),
       bypass_lock_for_test_(false),
+      fail_conditionalization_for_test_(false),
       use_stale_while_revalidate_(false),
       mode_(NORMAL),
       network_layer_(network_layer),
@@ -631,6 +634,8 @@ int HttpCache::CreateTransaction(RequestPriority priority,
       new HttpCache::Transaction(priority, this);
    if (bypass_lock_for_test_)
     transaction->BypassLockForTest();
+   if (fail_conditionalization_for_test_)
+     transaction->FailConditionalizationForTest();
 
   trans->reset(transaction);
   return OK;
diff --git a/net/http/http_cache.h b/net/http/http_cache.h
index 1c1338b..8fa1640 100644
--- a/net/http/http_cache.h
+++ b/net/http/http_cache.h
@@ -202,6 +202,12 @@ class NET_EXPORT HttpCache : public HttpTransactionFactory,
     bypass_lock_for_test_ = true;
   }
 
+  // Causes all transactions created after this point to generate a failure
+  // when attempting to conditionalize a network request.
+  void FailConditionalizationForTest() {
+    fail_conditionalization_for_test_ = true;
+  }
+
   bool use_stale_while_revalidate() const {
     return use_stale_while_revalidate_;
   }
@@ -426,6 +432,7 @@ class NET_EXPORT HttpCache : public HttpTransactionFactory,
   scoped_ptr<BackendFactory> backend_factory_;
   bool building_backend_;
   bool bypass_lock_for_test_;
+  bool fail_conditionalization_for_test_;
 
   // true if the implementation of Cache-Control: stale-while-revalidate
   // directive is enabled (either via command-line flag or experiment).
diff --git a/net/http/http_cache_transaction.cc b/net/http/http_cache_transaction.cc
index 43db2cd..0a84680 100644
--- a/net/http/http_cache_transaction.cc
+++ b/net/http/http_cache_transaction.cc
@@ -307,9 +307,7 @@ static bool HeaderMatches(const HttpRequestHeaders& headers,
 
 //-----------------------------------------------------------------------------
 
-HttpCache::Transaction::Transaction(
-    RequestPriority priority,
-    HttpCache* cache)
+HttpCache::Transaction::Transaction(RequestPriority priority, HttpCache* cache)
     : next_state_(STATE_NONE),
       request_(NULL),
       priority_(priority),
@@ -330,6 +328,7 @@ HttpCache::Transaction::Transaction(
       vary_mismatch_(false),
       couldnt_conditionalize_request_(false),
       bypass_lock_for_test_(false),
+      fail_conditionalization_for_test_(false),
       io_buf_len_(0),
       read_offset_(0),
       effective_load_flags_(0),
@@ -1711,6 +1710,16 @@ int HttpCache::Transaction::DoOverwriteCachedResponse() {
     return OK;
   }
 
+  if (handling_206_ && !CanResume(false)) {
+    // There is no point in storing this resource because it will never be used.
+    // This may change if we support LOAD_ONLY_FROM_CACHE with sparse entries.
+    DoneWritingToEntry(false);
+    if (partial_.get())
+      partial_->FixResponseHeaders(response_.headers.get(), true);
+    next_state_ = STATE_PARTIAL_HEADERS_RECEIVED;
+    return OK;
+  }
+
   target_state_ = STATE_TRUNCATE_CACHED_DATA;
   next_state_ = truncated_ ? STATE_CACHE_WRITE_TRUNCATED_RESPONSE :
                              STATE_CACHE_WRITE_RESPONSE;
@@ -2576,10 +2585,11 @@ bool HttpCache::Transaction::ConditionalizeRequest() {
     return false;
   }
 
-  if (response_.headers->response_code() == 206 &&
-      !response_.headers->HasStrongValidators()) {
+  if (fail_conditionalization_for_test_)
     return false;
-  }
+
+  DCHECK(response_.headers->response_code() != 206 ||
+         response_.headers->HasStrongValidators());
 
   // Just use the first available ETag and/or Last-Modified header value.
   // TODO(darin): Or should we use the last?
diff --git a/net/http/http_cache_transaction.h b/net/http/http_cache_transaction.h
index 9f486e9..35ca6e4 100644
--- a/net/http/http_cache_transaction.h
+++ b/net/http/http_cache_transaction.h
@@ -109,6 +109,11 @@ class HttpCache::Transaction : public HttpTransaction {
     bypass_lock_for_test_ = true;
   }
 
+  // Generates a failure when attempting to conditionalize a network request.
+  void FailConditionalizationForTest() {
+    fail_conditionalization_for_test_ = true;
+  }
+
   // HttpTransaction methods:
   int Start(const HttpRequestInfo* request_info,
             const CompletionCallback& callback,
@@ -436,6 +441,7 @@ class HttpCache::Transaction : public HttpTransaction {
   bool vary_mismatch_;  // The request doesn't match the stored vary data.
   bool couldnt_conditionalize_request_;
   bool bypass_lock_for_test_;  // A test is exercising the cache lock.
+  bool fail_conditionalization_for_test_;  // Fail ConditionalizeRequest.
   scoped_refptr<IOBuffer> read_buf_;
   int io_buf_len_;
   int read_offset_;
diff --git a/net/http/http_cache_unittest.cc b/net/http/http_cache_unittest.cc
index 471766c..30720ae 100644
--- a/net/http/http_cache_unittest.cc
+++ b/net/http/http_cache_unittest.cc
@@ -3745,15 +3745,13 @@ TEST(HttpCache, GET_Crazy416) {
   RemoveMockTransaction(&transaction);
 }
 
-// Tests that we store partial responses that can't be validated, as they can
-// be used for requests that don't require revalidation.
+// Tests that we don't store partial responses that can't be validated.
 TEST(HttpCache, RangeGET_NoStrongValidators) {
   MockHttpCache cache;
   std::string headers;
 
-  // Write to the cache (40-49).
-  MockTransaction transaction(kRangeGET_TransactionOK);
-  AddMockTransaction(&transaction);
+  // Attempt to write to the cache (40-49).
+  ScopedMockTransaction transaction(kRangeGET_TransactionOK);
   transaction.response_headers = "Content-Length: 10\n"
                                  "Cache-Control: max-age=3600\n"
                                  "ETag: w/\"foo\"\n";
@@ -3764,29 +3762,26 @@ TEST(HttpCache, RangeGET_NoStrongValidators) {
   EXPECT_EQ(0, cache.disk_cache()->open_count());
   EXPECT_EQ(1, cache.disk_cache()->create_count());
 
-  // Now verify that there's cached data.
+  // Now verify that there's no cached data.
   RunTransactionTestWithResponse(cache.http_cache(), kRangeGET_TransactionOK,
                                  &headers);
 
   Verify206Response(headers, 40, 49);
-  EXPECT_EQ(1, cache.network_layer()->transaction_count());
-  EXPECT_EQ(1, cache.disk_cache()->open_count());
-  EXPECT_EQ(1, cache.disk_cache()->create_count());
-
-  RemoveMockTransaction(&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 failures to validate cache partial responses that lack strong
-// validators.
-TEST(HttpCache, RangeGET_NoValidation) {
+// Tests failures to conditionalize byte range requests.
+TEST(HttpCache, RangeGET_NoConditionalization) {
   MockHttpCache cache;
+  cache.FailConditionalizations();
   std::string headers;
 
   // Write to the cache (40-49).
-  MockTransaction transaction(kRangeGET_TransactionOK);
-  AddMockTransaction(&transaction);
+  ScopedMockTransaction transaction(kRangeGET_TransactionOK);
   transaction.response_headers = "Content-Length: 10\n"
-                                 "ETag: w/\"foo\"\n";
+                                 "ETag: \"foo\"\n";
   RunTransactionTestWithResponse(cache.http_cache(), transaction, &headers);
 
   Verify206Response(headers, 40, 49);
@@ -3802,20 +3797,18 @@ TEST(HttpCache, RangeGET_NoValidation) {
   EXPECT_EQ(2, cache.network_layer()->transaction_count());
   EXPECT_EQ(1, cache.disk_cache()->open_count());
   EXPECT_EQ(2, cache.disk_cache()->create_count());
-
-  RemoveMockTransaction(&transaction);
 }
 
 // Tests that restarting a partial request when the cached data cannot be
 // revalidated logs an event.
 TEST(HttpCache, RangeGET_NoValidation_LogsRestart) {
   MockHttpCache cache;
+  cache.FailConditionalizations();
 
   // Write to the cache (40-49).
   ScopedMockTransaction transaction(kRangeGET_TransactionOK);
-  transaction.response_headers =
-      "Content-Length: 10\n"
-      "ETag: w/\"foo\"\n";
+  transaction.response_headers = "Content-Length: 10\n"
+                                 "ETag: \"foo\"\n";
   RunTransactionTest(cache.http_cache(), transaction);
 
   // Now verify that the cached data is not used.
@@ -3827,17 +3820,17 @@ TEST(HttpCache, RangeGET_NoValidation_LogsRestart) {
       log, net::NetLog::TYPE_HTTP_CACHE_RESTART_PARTIAL_REQUEST));
 }
 
-// Tests that a regular request (no range) with a sparse entry that doesn't have
-// strong validators results in a full response.
-TEST(HttpCache, GET_NoValidation) {
+// Tests that a failure to conditionalize a regular request (no range) with a
+// sparse entry results in a full response.
+TEST(HttpCache, GET_NoConditionalization) {
   MockHttpCache cache;
+  cache.FailConditionalizations();
   std::string headers;
 
   // Write to the cache (40-49).
   ScopedMockTransaction transaction(kRangeGET_TransactionOK);
-  transaction.response_headers =
-      "Content-Length: 10\n"
-      "ETag: w/\"foo\"\n";
+  transaction.response_headers = "Content-Length: 10\n"
+                                 "ETag: \"foo\"\n";
   RunTransactionTestWithResponse(cache.http_cache(), transaction, &headers);
 
   Verify206Response(headers, 40, 49);
@@ -3865,17 +3858,18 @@ TEST(HttpCache, GET_NoValidation) {
   EXPECT_EQ(2, cache.disk_cache()->create_count());
 }
 
-// Verifies that when asking for a range that would require the cache to modify
-// the range to ask, the actual server request matches the user's one.
-TEST(HttpCache, RangeGET_NoValidation2) {
+// Verifies that conditionalization failures when asking for a range that would
+// require the cache to modify the range to ask, result in a network request
+// that matches the user's one.
+TEST(HttpCache, RangeGET_NoConditionalization2) {
   MockHttpCache cache;
+  cache.FailConditionalizations();
   std::string headers;
 
   // Write to the cache (40-49).
   ScopedMockTransaction transaction(kRangeGET_TransactionOK);
-  transaction.response_headers =
-      "Content-Length: 10\n"
-      "ETag: w/\"foo\"\n";
+  transaction.response_headers = "Content-Length: 10\n"
+                                 "ETag: \"foo\"\n";
   RunTransactionTestWithResponse(cache.http_cache(), transaction, &headers);
 
   Verify206Response(headers, 40, 49);
diff --git a/net/http/mock_http_cache.cc b/net/http/mock_http_cache.cc
index 4ca542e..8b8d378 100644
--- a/net/http/mock_http_cache.cc
+++ b/net/http/mock_http_cache.cc
@@ -524,6 +524,10 @@ void MockHttpCache::BypassCacheLock() {
   http_cache_.BypassLockForTest();
 }
 
+void MockHttpCache::FailConditionalizations() {
+  http_cache_.FailConditionalizationForTest();
+}
+
 bool MockHttpCache::ReadResponseInfo(disk_cache::Entry* disk_entry,
                                      net::HttpResponseInfo* response_info,
                                      bool* response_truncated) {
diff --git a/net/http/mock_http_cache.h b/net/http/mock_http_cache.h
index 7e3a67c..bcc7925 100644
--- a/net/http/mock_http_cache.h
+++ b/net/http/mock_http_cache.h
@@ -189,6 +189,9 @@ class MockHttpCache {
   // Wrapper to bypass the cache lock for new transactions.
   void BypassCacheLock();
 
+  // Wrapper to fail request conditionalization for new transactions.
+  void FailConditionalizations();
+
   // Helper function for reading response info from the disk cache.
   static bool ReadResponseInfo(disk_cache::Entry* disk_entry,
                                net::HttpResponseInfo* response_info,
diff --git a/net/http/partial_data.cc b/net/http/partial_data.cc
index 565623b..f0615d5 100644
--- a/net/http/partial_data.cc
+++ b/net/http/partial_data.cc
@@ -291,6 +291,9 @@ bool PartialData::UpdateFromStoredHeaders(const HttpResponseHeaders* headers,
     return true;
   }
 
+  if (!headers->HasStrongValidators())
+    return false;
+
   int64 length_value = headers->GetContentLength();
   if (length_value <= 0)
     return false;  // We must have stored the resource length.