[net] Better StopCaching() handling for HttpCache::Transaction.

net/http/http_cache_unittest.cc

Index: net/http/http_cache_unittest.cc
diff --git a/net/http/http_cache_unittest.cc b/net/http/http_cache_unittest.cc
index f490444356fb1240f22e71fc3f443d0815b3a86b..e09bdb9eb0c387cb76194593764e1f48c50cf920 100644
--- a/net/http/http_cache_unittest.cc
+++ b/net/http/http_cache_unittest.cc
@@ -8,6 +8,7 @@
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
+#include "base/format_macros.h"
 #include "base/memory/scoped_vector.h"
 #include "base/message_loop/message_loop.h"
 #include "base/run_loop.h"
@@ -276,7 +277,6 @@ const MockTransaction kFastNoStoreGET_Transaction = {
     &FastTransactionServer::FastNoStoreHandler,
     nullptr,
     0,
-    0,
     OK};
 
 // This class provides a handler for kRangeGET_TransactionOK so that the range
@@ -342,6 +342,8 @@ void RangeTransactionServer::RangeHandler(const HttpRequestInfo* request,
                                           std::string* response_status,
                                           std::string* response_headers,
                                           std::string* response_data) {
+  SCOPED_TRACE(testing::Message() << "Request headers: \n"
+                                  << request->extra_headers.ToString());
   if (request->extra_headers.IsEmpty()) {
     response_status->assign("HTTP/1.1 416 Requested Range Not Satisfiable");
     response_data->clear();
@@ -449,7 +451,6 @@ const MockTransaction kRangeGET_TransactionOK = {
     &RangeTransactionServer::RangeHandler,
     nullptr,
     0,
-    0,
     OK};
 
 const char kFullRangeData[] =
@@ -6718,8 +6719,9 @@ TEST(HttpCache, DoneReading) {
   EXPECT_EQ(1, cache.disk_cache()->create_count());
 }
 
-// Tests that we stop caching when told.
-TEST(HttpCache, StopCachingDeletesEntry) {
+// Tests that calling StopCaching() causes the cache entry to be deleted if the
+// request cannot be resumed.
+TEST(HttpCache, StopCachingDeletesEntryIfRequestIsNotResumable) {
   MockHttpCache cache;
   TestCompletionCallback callback;
   MockHttpRequest request(kSimpleGET_Transaction);
@@ -6788,7 +6790,8 @@ TEST(HttpCache, StopCachingThenDoneReadingDeletesEntry) {
   // Make sure that the ActiveEntry is gone.
   base::MessageLoop::current()->RunUntilIdle();
 
-  // Verify that the entry is gone.
+  // Verify that the entry is gone. The request is not resumeable, so the cache
+  // entry should be deleted if only part of the response was cached.
   RunTransactionTest(cache.http_cache(), kSimpleGET_Transaction);
 
   EXPECT_EQ(2, cache.network_layer()->transaction_count());
@@ -6832,7 +6835,9 @@ TEST(HttpCache, StopCachingWithAuthDeletesEntry) {
 }
 
 // Tests that when we are told to stop caching we don't throw away valid data.
-TEST(HttpCache, StopCachingSavesEntry) {
+// If the request is resumable (i.e. has strong validators), then the cache
+// should hold on to the partial response.
+TEST(HttpCache, StopCachingSavesEntryIfRequestIsResumable) {
   MockHttpCache cache;
   TestCompletionCallback callback;
   MockHttpRequest request(kSimpleGET_Transaction);
@@ -6904,7 +6909,8 @@ TEST(HttpCache, StopCachingTruncatedEntry) {
     rv = trans->Read(buf.get(), 10, callback.callback());
     EXPECT_EQ(callback.GetResult(rv), 10);
 
-    // This is actually going to do nothing.
+    // This prevents any further writes to the cache entry, but doesn't discard
+    // the existing entry.
     trans->StopCaching();
 
     // We should be able to keep reading.
@@ -6916,19 +6922,167 @@ TEST(HttpCache, StopCachingTruncatedEntry) {
     EXPECT_EQ(callback.GetResult(rv), 0);
   }
 
-  // Verify that the disk entry was updated.
+  // Verify that the disk entry was not updated.
   disk_cache::Entry* entry;
   ASSERT_TRUE(cache.OpenBackendEntry(kRangeGET_TransactionOK.url, &entry));
-  EXPECT_EQ(80, entry->GetDataSize(1));
-  bool truncated = true;
+  EXPECT_EQ(20, entry->GetDataSize(1));
+  bool truncated = false;
   HttpResponseInfo response;
   EXPECT_TRUE(MockHttpCache::ReadResponseInfo(entry, &response, &truncated));
-  EXPECT_FALSE(truncated);
+  EXPECT_TRUE(truncated);
   entry->Close();
 
   RemoveMockTransaction(&kRangeGET_TransactionOK);
 }
 
+namespace {
+
+const int64 kTotalSize = 5000000000LL;  // Five beeeeeelllliooonn bytes!
+
+void ExpectByteRangeTransactionHandler(const net::HttpRequestInfo* request,
+                                       std::string* response_status,
+                                       std::string* response_headers,
+                                       std::string* response_data) {
+  std::string if_range;
+  EXPECT_TRUE(request->extra_headers.GetHeader(
+      net::HttpRequestHeaders::kIfRange, &if_range));
+  EXPECT_EQ("\"foo\"", if_range);
+
+  std::string range_header;
+  EXPECT_TRUE(request->extra_headers.GetHeader(net::HttpRequestHeaders::kRange,
+                                               &range_header));
+  std::vector<net::HttpByteRange> ranges;
+
+  EXPECT_TRUE(net::HttpUtil::ParseRangeHeader(range_header, &ranges));
+  ASSERT_EQ(1u, ranges.size());
+
+  net::HttpByteRange range = ranges[0];
+  EXPECT_TRUE(range.HasFirstBytePosition());
+  EXPECT_TRUE(range.HasLastBytePosition());
+
+  response_status->assign("HTTP/1.1 206 Partial");
+  response_headers->assign(base::StringPrintf(
+      "Content-Range: bytes %" PRId64 "-%" PRId64 "/%" PRId64
+      "\n"
+      "Content-Length: %" PRId64 "\n",
+      range.first_byte_position(), range.last_byte_position(), kTotalSize,
+      range.last_byte_position() - range.first_byte_position() + 1));
+}
+
+int LargeBufferReader(int64 content_length,
+                      int64 offset,
+                      net::IOBuffer* buf,
+                      int buf_len) {
+  EXPECT_LT(0, content_length);
+  EXPECT_LE(offset, content_length);
+  int num = std::min(static_cast<int64>(buf_len), content_length - offset);
+  return num;
+}
+
+void SetFlagOnBeforeNetworkStart(bool* started, bool* /* defer */) {
+  *started = true;
+}
+
+void StopCachingOnBeforeNetworkStart(HttpTransaction* transaction,
+                                     bool* /* defer */) {
+  transaction->StopCaching();
+}
+
+enum TransactionPhases {
+  TRANSACTION_PHASE_BEFORE_FIRST_READ,
+  TRANSACTION_PHASE_ON_BEFORE_NETWORK_START,
+  TRANSACTION_PHASE_AFTER_FIRST_READ,
+  TRANSACTION_PHASE_AFTER_NETWORK_READ
+};
+
+class HttpCacheTestWithPhases
+    : public ::testing::TestWithParam<TransactionPhases> {};
+
+INSTANTIATE_TEST_CASE_P(
+    Phase,
+    HttpCacheTestWithPhases,
+    ::testing::Values(TRANSACTION_PHASE_BEFORE_FIRST_READ,
+                      TRANSACTION_PHASE_ON_BEFORE_NETWORK_START,
+                      TRANSACTION_PHASE_AFTER_FIRST_READ,
+                      TRANSACTION_PHASE_AFTER_NETWORK_READ));
+
+}  // namespace
+
+TEST_P(HttpCacheTestWithPhases,
+       StopCachingFollowedByReadForHugeTruncatedResource) {
+  MockHttpCache cache;
+  MockTransaction transaction(kSimpleGET_Transaction);
+  transaction.url = kRangeGET_TransactionOK.url;
+  transaction.handler = &ExpectByteRangeTransactionHandler;
+  transaction.read_handler = &LargeBufferReader;
+  ScopedMockTransaction scoped_transaction(transaction);
+
+  std::string cached_headers = base::StringPrintf(
+      "HTTP/1.1 200 OK\n"
+      "Last-Modified: Sat, 18 Apr 2007 01:10:43 GMT\n"
+      "ETag: \"foo\"\n"
+      "Accept-Ranges: bytes\n"
+      "Content-Length: %" PRId64 "\n",
+      kTotalSize);
+  CreateTruncatedEntry(cached_headers, &cache);
+
+  MockHttpRequest request(transaction);
+  net::TestCompletionCallback callback;
+  scoped_ptr<net::HttpTransaction> http_transaction;
+  int rv = cache.http_cache()->CreateTransaction(net::DEFAULT_PRIORITY,
+                                                 &http_transaction);
+  ASSERT_EQ(net::OK, rv);
+  ASSERT_TRUE(http_transaction.get());
+
+  if (GetParam() == TRANSACTION_PHASE_ON_BEFORE_NETWORK_START)
+    http_transaction->SetBeforeNetworkStartCallback(
+        base::Bind(&StopCachingOnBeforeNetworkStart, http_transaction.get()));
+
+  bool network_transaction_started = false;
+  if (GetParam() == TRANSACTION_PHASE_AFTER_NETWORK_READ)
+    http_transaction->SetBeforeNetworkStartCallback(
+        base::Bind(&SetFlagOnBeforeNetworkStart, &network_transaction_started));
+
+  rv = http_transaction->Start(&request, callback.callback(),
+                               net::BoundNetLog());
+  rv = callback.GetResult(rv);
+  ASSERT_EQ(net::OK, rv);
+
+  if (GetParam() == TRANSACTION_PHASE_BEFORE_FIRST_READ)
+    http_transaction->StopCaching();
+
+  int64 total_bytes_received = 0;
+
+  EXPECT_EQ(kTotalSize,
+            http_transaction->GetResponseInfo()->headers->GetContentLength());
+  do {
+    // This test simulates reading Gigabytes of data. Buffer size is set to 1MB
+    // to reduce the number of reads and speedup the test.
+    const int kBufferSize = 1024 * 1024;
+    scoped_refptr<net::IOBuffer> buf(new net::IOBuffer(kBufferSize));
+    rv = http_transaction->Read(buf.get(), kBufferSize, callback.callback());
+    rv = callback.GetResult(rv);
+
+    if (GetParam() == TRANSACTION_PHASE_AFTER_FIRST_READ &&
+        total_bytes_received == 0)
+      http_transaction->StopCaching();
+
+    if (rv > 0)
+      total_bytes_received += rv;
+
+    if (network_transaction_started &&
+        GetParam() == TRANSACTION_PHASE_AFTER_NETWORK_READ) {
+      http_transaction->StopCaching();
+      network_transaction_started = false;
+    }
+  } while (rv > 0);
+
+  // The only thing we can test right now. Ideally we would also verify that

[hubbe, 2015/07/16 15:56:12]

This comment makes no sense to me.
Verify A, but currently !A?
If so, perhaps we should file a  bug to fix A and put a link to it here?

[asanka, 2015/07/16 16:16:58]

The comment is explaining why we aren't checking more things.

- The expectation below is the only thing we can currently verify.

- Currently the code makes multiple requests which look like the following:
  * the single byte at the start of the uncached region.
  * the uncached region in 2GB chunks.

  Optimally we'd send a single request for the entire uncached range. But that's
not currently possible until we have plumb int64_t all the way through the
stack, and reworking PartialData implementation. Once that happens, this test
and others could add expectations that the specific predictable and correct
network request was generated by the cache transaction.

  I didn't want to add expectations for the current behavior since that makes it
seem like we want to preserve that behavior. Hence the comment.

Does that make sense?

[hubbe, 2015/07/16 16:57:52]

Yes, this is a much better explanation than what is in the comment, please add
this explanation to the comment.

[asanka, 2015/07/16 17:57:00]

Done.

+  // only a single network request was made etc. But that's not currently
+  // possible since multiple network requests are being made.
+  EXPECT_EQ(kTotalSize, total_bytes_received);
+}
+
 // Tests that we detect truncated resources from the net when there is
 // a Content-Length header.
 TEST(HttpCache, TruncatedByContentLength) {