Revert 62171 - Http cache: make sure that we revalidate a cached entry

when the requested byte range looks wrong.

We postpone the decision about what to do with a cached
entry until we receive confirmation from the server, so
we cannot skip asking the server about it!.

This CL also makes it so that if the server returns 304
(so that our cached copy is curent), we don't destroy the
entry, and we return 416 (bad range request) to the caller.

BUG=58047
TEST=net_unittests


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

TBR=rvargas@google.com
Review URL: http://codereview.chromium.org/3644005

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

5 files changed, 27 insertions, 98 deletions

diff --git a/net/http/http_cache_transaction.cc b/net/http/http_cache_transaction.cc
index 96b1fff..a6ef5e0 100644
--- a/net/http/http_cache_transaction.cc
+++ b/net/http/http_cache_transaction.cc
@@ -672,8 +672,7 @@ int HttpCache::Transaction::DoSuccessfulSendRequest() {
     return OK;
   }
 
-  new_response_ = new_response;
-  if (!ValidatePartialResponse(&server_responded_206_) &&
+  if (!ValidatePartialResponse(new_response->headers, &server_responded_206_) &&
       !auth_response_.headers) {
     // Something went wrong with this request and we have to restart it.
     // If we have an authentication response, we are exposed to weird things
@@ -681,7 +680,6 @@ int HttpCache::Transaction::DoSuccessfulSendRequest() {
     // the new response.
     response_ = HttpResponseInfo();
     network_trans_.reset();
-    new_response_ = NULL;
     next_state_ = STATE_SEND_REQUEST;
     return OK;
   }
@@ -692,14 +690,9 @@ int HttpCache::Transaction::DoSuccessfulSendRequest() {
     DoneWritingToEntry(false);
   }
 
-  if (enable_range_support_ && new_response_->headers->response_code() == 416) {
-    DCHECK_EQ(NONE, mode_);
-    response_ = *new_response_;
-    return OK;
-  }
-
   HistogramHeaders(new_response->headers);
 
+  new_response_ = new_response;
   // Are we expecting a response to a conditional query?
   if (mode_ == READ_WRITE || mode_ == UPDATE) {
     if (new_response->headers->response_code() == 304 ||
@@ -1051,7 +1044,7 @@ int HttpCache::Transaction::DoPartialHeadersReceived() {
   } else if (mode_ != NONE) {
     // We are about to return the headers for a byte-range request to the user,
     // so let's fix them.
-    partial_->FixResponseHeaders(response_.headers, true);
+    partial_->FixResponseHeaders(response_.headers);
   }
   return OK;
 }
@@ -1403,7 +1396,7 @@ int HttpCache::Transaction::BeginCacheValidation() {
   bool skip_validation = effective_load_flags_ & LOAD_PREFERRING_CACHE ||
                          !RequiresValidation();
 
-  if (partial_.get() && !partial_->IsCurrentRangeCached() || invalid_range_)
+  if (partial_.get() && !partial_->IsCurrentRangeCached())
     skip_validation = false;
 
   if (skip_validation) {
@@ -1604,11 +1597,8 @@ bool HttpCache::Transaction::ConditionalizeRequest() {
   }
   DCHECK(custom_request_.get());
 
-  bool use_if_range = partial_.get() && !partial_->IsCurrentRangeCached() &&
-                      !invalid_range_;
-
   if (!etag_value.empty()) {
-    if (use_if_range) {
+    if (partial_.get() && !partial_->IsCurrentRangeCached()) {
       // We don't want to switch to WRITE mode if we don't have this block of a
       // byte-range request because we may have other parts cached.
       custom_request_->extra_headers.SetHeader(
@@ -1624,7 +1614,7 @@ bool HttpCache::Transaction::ConditionalizeRequest() {
   }
 
   if (!last_modified_value.empty()) {
-    if (use_if_range) {
+    if (partial_.get() && !partial_->IsCurrentRangeCached()) {
       custom_request_->extra_headers.SetHeader(
           HttpRequestHeaders::kIfRange, last_modified_value);
     } else {
@@ -1654,8 +1644,8 @@ bool HttpCache::Transaction::ConditionalizeRequest() {
 // WARNING: Whenever this code returns false, it has to make sure that the next
 // time it is called it will return true so that we don't keep retrying the
 // request.
-bool HttpCache::Transaction::ValidatePartialResponse(bool* partial_content) {
-  const HttpResponseHeaders* headers = new_response_->headers;
+bool HttpCache::Transaction::ValidatePartialResponse(
+    const HttpResponseHeaders* headers, bool* partial_content) {
   int response_code = headers->response_code();
   bool partial_response = enable_range_support_ ? response_code == 206 : false;
   *partial_content = false;
@@ -1667,13 +1657,10 @@ bool HttpCache::Transaction::ValidatePartialResponse(bool* partial_content) {
     // We gave up trying to match this request with the stored data. If the
     // server is ok with the request, delete the entry, otherwise just ignore
     // this request
-    DCHECK(!reading_);
-    if (partial_response || response_code == 200) {
+    if (partial_response || response_code == 200 || response_code == 304) {
       DoomPartialEntry(true);
       mode_ = NONE;
     } else {
-      if (response_code == 304)
-        FailRangeRequest();
       IgnoreRangeRequest();
     }
     return true;
@@ -1747,11 +1734,6 @@ void HttpCache::Transaction::IgnoreRangeRequest() {
   mode_ = NONE;
 }
 
-void HttpCache::Transaction::FailRangeRequest() {
-  response_ = *new_response_;
-  partial_->FixResponseHeaders(response_.headers, false);
-}
-
 int HttpCache::Transaction::ReadFromNetwork(IOBuffer* data, int data_len) {
   read_buf_ = data;
   io_buf_len_ = data_len;
@@ -1847,7 +1829,6 @@ void HttpCache::Transaction::DoneWritingToEntry(bool success) {
 }
 
 void HttpCache::Transaction::DoomPartialEntry(bool delete_object) {
-  DVLOG(2) << "DoomPartialEntry";
   int rv = cache_->DoomEntry(cache_key_, NULL);
   DCHECK_EQ(OK, rv);
   cache_->DoneWithEntry(entry_, this, false);
diff --git a/net/http/http_cache_transaction.h b/net/http/http_cache_transaction.h
index 4d3673c..2883282 100644
--- a/net/http/http_cache_transaction.h
+++ b/net/http/http_cache_transaction.h
@@ -267,15 +267,12 @@ class HttpCache::Transaction : public HttpTransaction {
   // Makes sure that a 206 response is expected.  Returns true on success.
   // On success, |partial_content| will be set to true if we are processing a
   // partial entry.
-  bool ValidatePartialResponse(bool* partial_content);
+  bool ValidatePartialResponse(const HttpResponseHeaders* headers,
+                               bool* partial_content);
 
   // Handles a response validation error by bypassing the cache.
   void IgnoreRangeRequest();
 
-  // Changes the response code of a range request to be 416 (Requested range not
-  // satisfiable).
-  void FailRangeRequest();
-
   // Reads data from the network.
   int ReadFromNetwork(IOBuffer* data, int data_len);
 
diff --git a/net/http/http_cache_unittest.cc b/net/http/http_cache_unittest.cc
index eec8098..2c8e16d 100644
--- a/net/http/http_cache_unittest.cc
+++ b/net/http/http_cache_unittest.cc
@@ -863,7 +863,6 @@ void RangeTransactionServer::RangeHandler(const net::HttpRequestInfo* request,
                                           std::string* response_data) {
   if (request->extra_headers.IsEmpty()) {
     response_status->assign("HTTP/1.1 416 Requested Range Not Satisfiable");
-    response_data->clear();
     return;
   }
 
@@ -872,7 +871,6 @@ void RangeTransactionServer::RangeHandler(const net::HttpRequestInfo* request,
 
   if (not_modified_) {
     response_status->assign("HTTP/1.1 304 Not Modified");
-    response_data->clear();
     return;
   }
 
@@ -887,7 +885,6 @@ void RangeTransactionServer::RangeHandler(const net::HttpRequestInfo* request,
   net::HttpByteRange byte_range = ranges[0];
   if (byte_range.first_byte_position() > 79) {
     response_status->assign("HTTP/1.1 416 Requested Range Not Satisfiable");
-    response_data->clear();
     return;
   }
 
@@ -3167,7 +3164,7 @@ TEST(HttpCache, UnknownRangeGET_304) {
 
   // Ask for the end of the file, without knowing the length.
   transaction.request_headers = "Range: bytes = 70-\r\n" EXTRA_HEADER;
-  transaction.data = "";
+  transaction.data = "rg: 70-79 ";
   RunTransactionTestWithResponse(cache.http_cache(), transaction, &headers);
 
   // We just bypass the cache.
@@ -3412,34 +3409,13 @@ TEST(HttpCache, RangeGET_Previous200) {
   // The last transaction has finished so make sure the entry is deactivated.
   MessageLoop::current()->RunAllPending();
 
-  // Make a request for an invalid range.
-  MockTransaction transaction3(kRangeGET_TransactionOK);
-  transaction3.request_headers = "Range: bytes = 80-90\r\n" EXTRA_HEADER;
-  transaction3.data = "";
-  transaction3.load_flags = net::LOAD_PREFERRING_CACHE;
-  RunTransactionTestWithResponse(cache.http_cache(), transaction3, &headers);
-  EXPECT_EQ(2, cache.disk_cache()->open_count());
-  EXPECT_EQ(0U, headers.find("HTTP/1.1 416 "));
-  EXPECT_NE(std::string::npos, headers.find("Content-Range: bytes 0-0/80"));
-  EXPECT_NE(std::string::npos, headers.find("Content-Length: 0"));
-
-  // Make sure the entry is deactivated.
-  MessageLoop::current()->RunAllPending();
-
-  // Even though the request was invalid, we should have the entry.
-  RunTransactionTest(cache.http_cache(), transaction2);
-  EXPECT_EQ(3, cache.disk_cache()->open_count());
-
-  // Make sure the entry is deactivated.
-  MessageLoop::current()->RunAllPending();
-
   // Now we should receive a range from the server and drop the stored entry.
   handler.set_not_modified(false);
   transaction2.request_headers = kRangeGET_TransactionOK.request_headers;
   RunTransactionTestWithResponse(cache.http_cache(), transaction2, &headers);
   Verify206Response(headers, 40, 49);
-  EXPECT_EQ(5, cache.network_layer()->transaction_count());
-  EXPECT_EQ(4, cache.disk_cache()->open_count());
+  EXPECT_EQ(3, cache.network_layer()->transaction_count());
+  EXPECT_EQ(2, cache.disk_cache()->open_count());
   EXPECT_EQ(1, cache.disk_cache()->create_count());
 
   RunTransactionTest(cache.http_cache(), transaction2);
@@ -3508,7 +3484,6 @@ TEST(HttpCache, RangeGET_MoreThanCurrentSize) {
   // A weird request should not delete this entry. Ask for bytes 120-.
   MockTransaction transaction(kRangeGET_TransactionOK);
   transaction.request_headers = "Range: bytes = 120-\r\n" EXTRA_HEADER;
-  transaction.data = "";
   RunTransactionTestWithResponse(cache.http_cache(), transaction, &headers);
 
   EXPECT_EQ(0U, headers.find("HTTP/1.1 416 "));
diff --git a/net/http/partial_data.cc b/net/http/partial_data.cc
index 799c3fa..54900b3 100644
--- a/net/http/partial_data.cc
+++ b/net/http/partial_data.cc
@@ -127,9 +127,6 @@ bool PartialData::Init(const HttpRequestHeaders& headers) {
 
   resource_size_ = 0;
   current_range_start_ = byte_range_.first_byte_position();
-
-  DVLOG(1) << "Range start: " << current_range_start_ << " end: " <<
-               byte_range_.last_byte_position();
   return true;
 }
 
@@ -157,8 +154,6 @@ int PartialData::ShouldValidateCache(disk_cache::Entry* entry,
   if (!len)
     return 0;
 
-  DVLOG(3) << "ShouldValidateCache len: " << len;
-
   if (sparse_entry_) {
     DCHECK(!callback_);
     Core* core = Core::CreateCore(this);
@@ -176,12 +171,6 @@ int PartialData::ShouldValidateCache(disk_cache::Entry* entry,
       cached_start_ = 0;
     }
   } else {
-    if (byte_range_.HasFirstBytePosition() &&
-        byte_range_.first_byte_position() >= resource_size_) {
-      // The caller should take care of this condition because we should have
-      // failed IsRequestedRangeOK(), but it's better to be consistent here.
-      len = 0;
-    }
     cached_min_len_ = len;
     cached_start_ = current_range_start_;
   }
@@ -261,7 +250,6 @@ bool PartialData::UpdateFromStoredHeaders(const HttpResponseHeaders* headers,
     DCHECK(byte_range_.IsValid());
     sparse_entry_ = false;
     resource_size_ = entry->GetDataSize(kDataStream);
-    DVLOG(2) << "UpdateFromStoredHeaders size: " << resource_size_;
     return true;
   }
 
@@ -347,36 +335,28 @@ bool PartialData::ResponseHeadersOK(const HttpResponseHeaders* headers) {
 // We are making multiple requests to complete the range requested by the user.
 // Just assume that everything is fine and say that we are returning what was
 // requested.
-void PartialData::FixResponseHeaders(HttpResponseHeaders* headers,
-                                     bool success) {
+void PartialData::FixResponseHeaders(HttpResponseHeaders* headers) {
   if (truncated_)
     return;
 
   headers->RemoveHeader(kLengthHeader);
   headers->RemoveHeader(kRangeHeader);
 
-  int64 range_len, start, end;
+  int64 range_len;
   if (byte_range_.IsValid()) {
-    if (success) {
-      if (!sparse_entry_)
-        headers->ReplaceStatusLine("HTTP/1.1 206 Partial Content");
-
-      DCHECK(byte_range_.HasFirstBytePosition());
-      DCHECK(byte_range_.HasLastBytePosition());
-      start = byte_range_.first_byte_position();
-      end = byte_range_.last_byte_position();
-      range_len = end - start + 1;
-    } else {
-      headers->ReplaceStatusLine(
-          "HTTP/1.1 416 Requested Range Not Satisfiable");
-      start = 0;
-      end = 0;
-      range_len = 0;
-    }
+    if (!sparse_entry_)
+      headers->ReplaceStatusLine("HTTP/1.1 206 Partial Content");
 
+    DCHECK(byte_range_.HasFirstBytePosition());
+    DCHECK(byte_range_.HasLastBytePosition());
     headers->AddHeader(
         base::StringPrintf("%s: bytes %" PRId64 "-%" PRId64 "/%" PRId64,
-                           kRangeHeader, start, end, resource_size_));
+                           kRangeHeader,
+                           byte_range_.first_byte_position(),
+                           byte_range_.last_byte_position(),
+                           resource_size_));
+    range_len = byte_range_.last_byte_position() -
+                byte_range_.first_byte_position() + 1;
   } else {
     // TODO(rvargas): Is it safe to change the protocol version?
     headers->ReplaceStatusLine("HTTP/1.1 200 OK");
@@ -416,7 +396,6 @@ int PartialData::CacheRead(disk_cache::Entry* entry, IOBuffer* data,
 
 int PartialData::CacheWrite(disk_cache::Entry* entry, IOBuffer* data,
                             int data_len, CompletionCallback* callback) {
-  DVLOG(3) << "To write: " << data_len;
   if (sparse_entry_) {
     return entry->WriteSparseData(current_range_start_, data, data_len,
                                   callback);
@@ -430,7 +409,6 @@ int PartialData::CacheWrite(disk_cache::Entry* entry, IOBuffer* data,
 }
 
 void PartialData::OnCacheReadCompleted(int result) {
-  DVLOG(3) << "Read: " << result;
   if (result > 0) {
     current_range_start_ += result;
     cached_min_len_ -= result;
diff --git a/net/http/partial_data.h b/net/http/partial_data.h
index 45bef31..22e0c8b 100644
--- a/net/http/partial_data.h
+++ b/net/http/partial_data.h
@@ -84,9 +84,7 @@ class PartialData {
   bool ResponseHeadersOK(const HttpResponseHeaders* headers);
 
   // Fixes the response headers to include the right content length and range.
-  // |success| is the result of the whole request so if it's false, we'll change
-  // the result code to be 416.
-  void FixResponseHeaders(HttpResponseHeaders* headers, bool success);
+  void FixResponseHeaders(HttpResponseHeaders* headers);
 
   // Fixes the content length that we want to store in the cache.
   void FixContentLength(HttpResponseHeaders* headers);