Release the cache entry on deferred redirect.

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 01909ae3c505828c566cde302498e07e4c7e4410..070163a390eb9d571769956d625724c8b986696c 100644
--- a/net/http/http_cache_unittest.cc
+++ b/net/http/http_cache_unittest.cc
@@ -501,6 +501,33 @@ const MockTransaction kRangeGET_TransactionOK = {
   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) {
@@ -5333,7 +5360,7 @@ TEST(HttpCache, CachedRedirect) {
   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(
@@ -5355,6 +5382,9 @@ TEST(HttpCache, CachedRedirect) {
     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.
   }
@@ -5362,7 +5392,12 @@ TEST(HttpCache, CachedRedirect) {
   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(
@@ -5384,6 +5419,9 @@ TEST(HttpCache, CachedRedirect) {
     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.
   }
@@ -5838,6 +5876,36 @@ TEST(HttpCache, FilterCompletion) {
   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;