Release the cache entry on deferred redirect.

net/http/http_cache_unittest.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/http/http_cache.h"
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/memory/scoped_vector.h"
 #include "base/message_loop/message_loop.h"
@@ skipping 483 matching lines @@
   "Accept-Ranges: bytes\n"
   "Content-Length: 10\n",
   base::Time(),
   "rg: 40-49 ",
   TEST_MODE_NORMAL,
   &RangeTransactionServer::RangeHandler,
   0,
   net::OK
 };
 
+// The cache should return no response, so data is empty, but the
+// network will return data. (For redirects, DoneReading() is used to
+// discard the response.)
+void DiscardedResponseHandler(const net::HttpRequestInfo* request,
+                              std::string* response_status,
+                              std::string* response_headers,
+                              std::string* response_data) {
+  *response_data = "hello";
+}
+
+const MockTransaction kDiscardedResponse_Transaction = {

[rvargas (doing something else), 2013/09/18 18:37:10]

If we keep this transaction, move it to right before the test (there are no
multiple users, and it is simpler to see what the test does)

[davidben, 2013/09/18 19:35:43]

(Removed it)

+  "http://www.google.com/",
+  "GET",
+  base::Time(),
+  "",
+  net::LOAD_NORMAL,
+  "HTTP/1.1 302 Found",
+  "Cache-Control: max-age=10000\n"
+  "Location: http://www.google.com/destination\n",
+  base::Time(),
+  "",
+  TEST_MODE_NORMAL,
+  DiscardedResponseHandler,

[rvargas (doing something else), 2013/09/18 18:37:10]

From the point of view of the test, or the contract change of the transaction,
it really doesn't matter if this is a redirect or not, or if the server can
return a response body or not. Seems better to just use "" here.

[davidben, 2013/09/18 19:35:43]

(Removed it)

+  0,
+  net::OK
+};
+
 // Verifies the response headers (|response|) match a partial content
 // response for the range starting at |start| and ending at |end|.
 void Verify206Response(std::string response, int start, int end) {
   std::string raw_headers(net::HttpUtil::AssembleRawHeaders(response.data(),
                                                             response.size()));
   scoped_refptr<net::HttpResponseHeaders> headers(
       new net::HttpResponseHeaders(raw_headers));
 
   ASSERT_EQ(206, headers->response_code());
 
@@ skipping 4812 matching lines @@
 TEST(HttpCache, CachedRedirect) {
   MockHttpCache cache;
 
   ScopedMockTransaction kTestTransaction(kSimpleGET_Transaction);
   kTestTransaction.status = "HTTP/1.1 301 Moved Permanently";
   kTestTransaction.response_headers = "Location: http://www.bar.com/\n";
 
   MockHttpRequest request(kTestTransaction);
   net::TestCompletionCallback callback;
 
-  // write to the cache
+  // Write to the cache.
   {
     scoped_ptr<net::HttpTransaction> trans;
     int rv = cache.http_cache()->CreateTransaction(
         net::DEFAULT_PRIORITY, &trans, NULL);
     EXPECT_EQ(net::OK, rv);
     ASSERT_TRUE(trans.get());
 
     rv = trans->Start(&request, callback.callback(), net::BoundNetLog());
     if (rv == net::ERR_IO_PENDING)
       rv = callback.WaitForResult();
     ASSERT_EQ(net::OK, rv);
 
     const net::HttpResponseInfo* info = trans->GetResponseInfo();
     ASSERT_TRUE(info);
 
     EXPECT_EQ(info->headers->response_code(), 301);
 
     std::string location;
     info->headers->EnumerateHeader(NULL, "Location", &location);
     EXPECT_EQ(location, "http://www.bar.com/");
 
+    // Mark the transaction as completed so it is cached.
+    trans->DoneReading();

[rvargas (doing something else), 2013/09/18 18:37:10]

This would not be needed.

[davidben, 2013/09/18 19:35:43]

(Left this in here for now pending discussion about the redirect check in the
cache. I'll get rid of this if we put the check back.)

+
     // Destroy transaction when going out of scope. We have not actually
     // read the response body -- want to test that it is still getting cached.
   }
   EXPECT_EQ(1, cache.network_layer()->transaction_count());
   EXPECT_EQ(0, cache.disk_cache()->open_count());
   EXPECT_EQ(1, cache.disk_cache()->create_count());
 
-  // read from the cache
+  // Active entries in the cache are not retired synchronously. Make
+  // sure the next run hits the MockHttpCache and open_count is
+  // correct.
+  base::MessageLoop::current()->RunUntilIdle();
+
+  // Read from the cache.
   {
     scoped_ptr<net::HttpTransaction> trans;
     int rv = cache.http_cache()->CreateTransaction(
         net::DEFAULT_PRIORITY, &trans, NULL);
     EXPECT_EQ(net::OK, rv);
     ASSERT_TRUE(trans.get());
 
     rv = trans->Start(&request, callback.callback(), net::BoundNetLog());
     if (rv == net::ERR_IO_PENDING)
       rv = callback.WaitForResult();
     ASSERT_EQ(net::OK, rv);
 
     const net::HttpResponseInfo* info = trans->GetResponseInfo();
     ASSERT_TRUE(info);
 
     EXPECT_EQ(info->headers->response_code(), 301);
 
     std::string location;
     info->headers->EnumerateHeader(NULL, "Location", &location);
     EXPECT_EQ(location, "http://www.bar.com/");
 
+    // Mark the transaction as completed so it is cached.
+    trans->DoneReading();
+
     // Destroy transaction when going out of scope. We have not actually
     // read the response body -- want to test that it is still getting cached.
   }
   EXPECT_EQ(1, cache.network_layer()->transaction_count());
   EXPECT_EQ(1, cache.disk_cache()->open_count());
   EXPECT_EQ(1, cache.disk_cache()->create_count());
 }
 
 // Verify that no-cache resources are stored in cache, but are not fetched from
 // cache during normal loads.
@@ skipping 434 matching lines @@
   base::MessageLoop::current()->RunUntilIdle();
 
   // Read from the cache.
   RunTransactionTest(cache.http_cache(), kSimpleGET_Transaction);
 
   EXPECT_EQ(1, cache.network_layer()->transaction_count());
   EXPECT_EQ(1, cache.disk_cache()->open_count());
   EXPECT_EQ(1, cache.disk_cache()->create_count());
 }
 
+// Tests that we don't mark entries as truncated and release the cache
+// entry when DoneReading() is called before any Read() calls, such as
+// for a redirect.
+TEST(HttpCache, DiscardedResponse) {

[rvargas (doing something else), 2013/09/18 18:37:10]

nit: DiscardedResponse -> DoneReading ?

[davidben, 2013/09/18 19:35:43]

Done.

+  MockHttpCache cache;
+  net::TestCompletionCallback callback;
+
+  scoped_ptr<net::HttpTransaction> trans;
+  int rv = cache.http_cache()->CreateTransaction(
+      net::DEFAULT_PRIORITY, &trans, NULL);
+  EXPECT_EQ(net::OK, rv);
+
+  MockHttpRequest request(kDiscardedResponse_Transaction);

[rvargas (doing something else), 2013/09/18 18:37:10]

I would prefer using kSimpleGET_Transaction and a ScopedMockTransaction. The
point being to avoid distraction from what is relevant for this particular
test... it makes it clear that the request doesn't matter (POST, GET, redirect
etc). The only thing required for line 5902 to work is an empty response, and
doing that for a ScopedMockTransaction is a single line.

[davidben, 2013/09/18 19:35:43]

Done.

+  rv = trans->Start(&request, callback.callback(), net::BoundNetLog());
+  EXPECT_EQ(net::OK, callback.GetResult(rv));
+
+  trans->DoneReading();
+  // Leave the transaction around.
+
+  // Make sure that the ActiveEntry is gone.
+  base::MessageLoop::current()->RunUntilIdle();
+
+  // Read from the cache. This should not deadlock.
+  RunTransactionTest(cache.http_cache(), kDiscardedResponse_Transaction);
+
+  EXPECT_EQ(1, cache.network_layer()->transaction_count());
+  EXPECT_EQ(1, cache.disk_cache()->open_count());
+  EXPECT_EQ(1, cache.disk_cache()->create_count());
+}
+
 // Tests that we stop caching when told.
 TEST(HttpCache, StopCachingDeletesEntry) {
   MockHttpCache cache;
   net::TestCompletionCallback callback;
   MockHttpRequest request(kSimpleGET_Transaction);
 
   {
     scoped_ptr<net::HttpTransaction> trans;
     int rv = cache.http_cache()->CreateTransaction(
         net::DEFAULT_PRIORITY, &trans, NULL);
@@ skipping 311 matching lines @@
   trans->SetPriority(net::HIGHEST);
   // Should trigger a new network transaction and pick up the new
   // priority.
   ReadAndVerifyTransaction(trans.get(), transaction);
 
   EXPECT_EQ(net::HIGHEST,
             cache.network_layer()->last_create_transaction_priority());
 
   RemoveMockTransaction(&kRangeGET_TransactionOK);
 }