Release the cache entry on deferred redirect.

net/url_request/url_request_unittest.cc

Index: net/url_request/url_request_unittest.cc
diff --git a/net/url_request/url_request_unittest.cc b/net/url_request/url_request_unittest.cc
index c1178fe04dadabfd1d6b46e6cb670bddf5c8a4ab..598f3f7fdd297fa17f979b04699058d2a78cc433 100644
--- a/net/url_request/url_request_unittest.cc
+++ b/net/url_request/url_request_unittest.cc
@@ -4203,6 +4203,40 @@ TEST_F(URLRequestTestHTTP, EmptyReferrerAfterValidReferrer) {
   EXPECT_EQ(std::string("None"), d.data_received());
 }
 
+

[mmenke, 2013/09/16 20:41:08]

nit:  Remove extra blank line.

[davidben, 2013/09/16 22:00:00]

Done.

+TEST_F(URLRequestTestHTTP, CachedRedirect) {

[mmenke, 2013/09/16 20:41:08]

Maybe a second test where the redirect has a non-empty body?

[davidben, 2013/09/16 22:00:00]

Are you sure it doesn't already have a non-empty body? redirect-test.html says
"hello" in it.

[mmenke, 2013/09/16 22:08:59]

Looks like you're right.  I saw the mock-headers, which don't have a
Content-Length line, and just assumed there was no body.  Our tests violate the
HTTP/1.1 spec (Though they'd still be in violation with a 0-length body, too). 
:(  Server should claim HTTP/1.0 or be sending a Content-Length header,
dagnabit!

+  ASSERT_TRUE(test_server_.Start());
+
+  // Write to the cache.
+  {
+    TestDelegate d;
+    GURL test_url(test_server_.GetURL("files/redirect-test.html"));
+    URLRequest req(test_url, &d, &default_context_);
+
+    req.Start();
+    base::RunLoop().Run();
+
+    EXPECT_EQ(1, d.received_redirect_count());
+    EXPECT_EQ(1, d.response_started_count());
+    EXPECT_FALSE(d.received_data_before_response());
+    EXPECT_EQ(URLRequestStatus::SUCCESS, req.status().status());
+  }
+
+  // Stop at the redirect and make sure it was read from cache.
+  {
+    TestDelegate d;
+    d.set_quit_on_redirect(true);
+    GURL test_url(test_server_.GetURL("files/redirect-test.html"));
+    URLRequest req(test_url, &d, &default_context_);
+
+    req.Start();
+    base::RunLoop().Run();
+
+    EXPECT_EQ(1, d.received_redirect_count());
+    EXPECT_TRUE(req.was_cached());
+  }
+}
+
 TEST_F(URLRequestTestHTTP, CancelRedirect) {
   ASSERT_TRUE(test_server_.Start());
 
@@ -4297,6 +4331,59 @@ TEST_F(URLRequestTestHTTP, DeferredRedirect_GetFullRequestHeaders) {
   }
 }
 
+// Tests that a deferred redirect releases its lock on the cache.
+TEST_F(URLRequestTestHTTP, DeferredRedirect_ReleaseCacheEntry) {
+  ASSERT_TRUE(test_server_.Start());
+
+  // First test the cold cache case.
+  {
+    TestDelegate d_defer, d;

[mmenke, 2013/09/16 20:41:08]

nit:  Define one variable per line.

[mmenke, 2013/09/16 20:41:08]

nit:  While this file name TestDelegates "d" a lot (In violation of Google
style), I think "d_defer" is going a bit too far.  Suggest just writing it out
as "delegate".

[davidben, 2013/09/16 22:00:00]

Done.

[davidben, 2013/09/16 22:00:00]

Done.

+
+    // First, fire a request that defers its redirect.
+    d_defer.set_quit_on_redirect(true);
+    GURL test_url(test_server_.GetURL("files/redirect-test.html"));
+    URLRequest req_defer(test_url, &d_defer, &default_context_);

[mmenke, 2013/09/16 20:41:08]

optional:  Suggest "deferred_req" and "deferring_delegate".  Think it makes the
relationship clearer.

[davidben, 2013/09/16 22:00:00]

Done.

+    req_defer.Start();
+    base::RunLoop().Run();
+    EXPECT_EQ(1, d_defer.received_redirect_count());
+
+    // This should test the cold cache case.
+    EXPECT_FALSE(req_defer.was_cached());
+
+    // Now, fire a second at the same. It should not get stuck.
+    URLRequest req(test_url, &d, &default_context_);
+    req.Start();
+    base::RunLoop().Run();
+    EXPECT_EQ(1, d.response_started_count());
+    EXPECT_FALSE(d.received_data_before_response());
+    EXPECT_EQ(URLRequestStatus::SUCCESS, req.status().status());

[mmenke, 2013/09/16 20:41:08]

May want to mention something about the "deferred" redirect never being result -
think that some contradicts the word "deferred".  Or could rename it to
"indefinitely_defered_redirect" or something - open to better ideas.  Maybe
"paused_"?  "Halted" implies too much of a full stop...

[davidben, 2013/09/16 22:00:00]

Went with paused.

+  }
+
+  // Repeat, now with the cache warmed from the last run.
+  {
+    TestDelegate d_defer, d;
+
+    // First, fire a request that defers its redirect.
+    d_defer.set_quit_on_redirect(true);
+    GURL test_url(test_server_.GetURL("files/redirect-test.html"));
+    URLRequest req_defer(test_url, &d_defer, &default_context_);
+    req_defer.Start();
+    base::RunLoop().Run();
+    EXPECT_EQ(1, d_defer.received_redirect_count());
+
+    // This should test the warm cache case.
+    EXPECT_TRUE(req_defer.was_cached());
+
+    // Now, fire a second at the same. It should not get stuck.
+    URLRequest req(test_url, &d, &default_context_);
+    req.Start();
+    base::RunLoop().Run();
+    EXPECT_EQ(1, d.response_started_count());
+    EXPECT_FALSE(d.received_data_before_response());
+    EXPECT_EQ(URLRequestStatus::SUCCESS, req.status().status());
+  }
+}
+
 TEST_F(URLRequestTestHTTP, CancelDeferredRedirect) {
   ASSERT_TRUE(test_server_.Start());