Release the cache entry on deferred redirect.

If a redirect is deferred in the warm cache case, the HTTP transaction
currently doesn't release the cache entry until after the redirect continues.
This prevents other requests from accessing the cache entry.

Reuse the DoneReading() signal for the URLRequestJob to notify the transaction
that the response body is to be discarded. This matches the redirect-specific
logic in HttpCache::Transaction which does not cache redirect bodies.

In addition, now that this signal exists at a higher level, remove that logic
in HttpCache::Transaction. It is now the caller's job to decide which response
bodies are and aren't truncated away.

Fixup and add new tests for this behavior.

BUG=292879
TEST=HttpCache.CachedRedirect,
     HttpCache.DoneReading,
     URLRequestJob.RedirectTransactionNotifiedWhenDone

Review URL: https://chromiumcodereview.appspot.com/23710059

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

net/data/url_request_unittest/redirect-test.html.mock-http-headers

@@ -1,2 +1,3 @@
 HTTP/1.1 302 Redirect
 Location: with-headers.html
+Cache-Control: max-age=10000

net/http/http_cache.cc

@@ -392,7 +392,7 @@ void HttpCache::CloseAllConnections() {
   HttpNetworkSession* session = network->GetSession();
   if (session)
     session->CloseAllConnections();
-  }
+}
 
 void HttpCache::CloseIdleConnections() {
   net::HttpNetworkLayer* network =

net/http/http_cache_transaction.cc

@@ -449,10 +449,13 @@ bool HttpCache::Transaction::GetFullRequestHeaders(
 
 void HttpCache::Transaction::DoneReading() {
   if (cache_.get() && entry_) {
-    DCHECK(reading_);
     DCHECK_NE(mode_, UPDATE);
-    if (mode_ & WRITE)
+    if (mode_ & WRITE) {
       DoneWritingToEntry(true);
+    } else {
+      cache_->DoneReadingFromEntry(entry_, this);
+      entry_ = NULL;
+    }
   }
 }
 
@@ -1340,10 +1343,6 @@ int HttpCache::Transaction::DoTruncateCachedMetadataComplete(int result) {
     }
   }
 
-  // If this response is a redirect, then we can stop writing now.  (We don't
-  // need to cache the response body of a redirect.)
-  if (response_.headers->IsRedirect(NULL))
-    DoneWritingToEntry(true);
   next_state_ = STATE_PARTIAL_HEADERS_RECEIVED;
   return OK;
 }

net/http/http_cache_unittest.cc

@@ -5394,7 +5394,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(
@@ -5416,14 +5416,22 @@ 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.
   }
   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(
@@ -5445,6 +5453,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.
   }
@@ -5899,6 +5910,39 @@ 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, DoneReading) {
+  MockHttpCache cache;
+  net::TestCompletionCallback callback;
+
+  ScopedMockTransaction transaction(kSimpleGET_Transaction);
+  transaction.data = "";
+
+  scoped_ptr<net::HttpTransaction> trans;
+  int rv = cache.http_cache()->CreateTransaction(
+      net::DEFAULT_PRIORITY, &trans, NULL);
+  EXPECT_EQ(net::OK, rv);
+
+  MockHttpRequest request(transaction);
+  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(), 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/http/http_transaction.h

@@ -109,6 +109,9 @@ class NET_EXPORT_PRIVATE HttpTransaction {
   // of the stream. This is equivalent to performing an extra Read() at the end
   // that should return 0 bytes. This method should not be called if the
   // transaction is busy processing a previous operation (like a pending Read).
+  //
+  // DoneReading may also be called before the first Read() to notify that the
+  // entire response body is to be ignored (e.g., in a redirect).
   virtual void DoneReading() = 0;
 
   // Returns the response info for this transaction or NULL if the response

net/url_request/url_request_job.cc

@@ -319,6 +319,10 @@ void URLRequestJob::NotifyHeadersComplete() {
       new_location = new_location.ReplaceComponents(replacements);
     }
 
+    // Redirect response bodies are not read. Notify the transaction
+    // so it does not treat being stopped as an error.
+    DoneReading();
+
     bool defer_redirect = false;
     request_->NotifyReceivedRedirect(new_location, &defer_redirect);
 

net/url_request/url_request_job_unittest.cc

@@ -37,6 +37,24 @@ const MockTransaction kGZip_Transaction = {
   net::OK
 };
 
+const MockTransaction kRedirect_Transaction = {
+  "http://www.google.com/redirect",
+  "GET",
+  base::Time(),
+  "",
+  net::LOAD_NORMAL,
+  "HTTP/1.1 302 Found",
+  "Cache-Control: max-age=10000\n"
+  "Location: http://www.google.com/destination\n"
+  "Content-Length: 5\n",
+  base::Time(),
+  "hello",
+  TEST_MODE_NORMAL,
+  NULL,
+  0,
+  net::OK
+};
+
 }  // namespace
 
 TEST(URLRequestJob, TransactionNotifiedWhenDone) {
@@ -78,3 +96,22 @@ TEST(URLRequestJob, SyncTransactionNotifiedWhenDone) {
 
   RemoveMockTransaction(&transaction);
 }
+
+TEST(URLRequestJob, RedirectTransactionNotifiedWhenDone) {
+  MockNetworkLayer network_layer;
+  net::TestURLRequestContext context;
+  context.set_http_transaction_factory(&network_layer);
+
+  net::TestDelegate d;
+  net::TestURLRequest req(GURL(kRedirect_Transaction.url), &d, &context, NULL);
+  AddMockTransaction(&kRedirect_Transaction);
+
+  req.set_method("GET");
+  req.Start();
+
+  base::MessageLoop::current()->Run();
+
+  EXPECT_TRUE(network_layer.done_reading_called());
+
+  RemoveMockTransaction(&kRedirect_Transaction);
+}