Don't close the socket in the HttpNetworkTransaction

destructor if we can and should keep the connection alive.
This is necessary because in some situations, DoReadBodyComplete
never gets called; this can happen, for example, when the response
body length is zero, and the caller would be the cache transaction.

BUG=47191
TEST=Added unit test. Existing net_unittests pass.

Review URL: http://codereview.chromium.org/2807014

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

2 files changed, 67 insertions, 3 deletions

diff --git a/net/http/http_network_transaction.cc b/net/http/http_network_transaction.cc
index d65a892..016cd58 100644..100755
--- a/net/http/http_network_transaction.cc
+++ b/net/http/http_network_transaction.cc
@@ -542,9 +542,19 @@ uint64 HttpNetworkTransaction::GetUploadProgress() const {
 
 HttpNetworkTransaction::~HttpNetworkTransaction() {
   // If we still have an open socket, then make sure to disconnect it so it
-  // won't call us back and we don't try to reuse it later on.
-  if (connection_.get() && connection_->is_initialized())
-    connection_->socket()->Disconnect();
+  // won't call us back and we don't try to reuse it later on. However,
+  // don't close the socket if we should keep the connection alive.
+  if (connection_.get() && connection_->is_initialized()) {
+    // The STATE_NONE check guarantees there are no pending socket IOs that
+    // could try to call this object back after it is deleted.
+    bool keep_alive = next_state_ == STATE_NONE &&
+                      http_stream_.get() &&
+                      http_stream_->IsResponseBodyComplete() &&
+                      http_stream_->CanFindEndOfResponse() &&
+                      GetResponseHeaders()->IsKeepAlive();
+    if (!keep_alive)
+      connection_->socket()->Disconnect();
+  }
 
   if (pac_request_)
     session_->proxy_service()->CancelPacRequest(pac_request_);
diff --git a/net/http/http_network_transaction_unittest.cc b/net/http/http_network_transaction_unittest.cc
index 449a966..35ba760 100644..100755
--- a/net/http/http_network_transaction_unittest.cc
+++ b/net/http/http_network_transaction_unittest.cc
@@ -790,6 +790,60 @@ TEST_F(HttpNetworkTransactionTest, NonKeepAliveConnectionEOF) {
   EXPECT_EQ(ERR_EMPTY_RESPONSE, out.rv);
 }
 
+// Test that we correctly reuse a keep-alive connection after receiving a 304.
+TEST_F(HttpNetworkTransactionTest, KeepAliveAfter304) {
+  SessionDependencies session_deps;
+  scoped_refptr<HttpNetworkSession> session = CreateSession(&session_deps);
+
+  HttpRequestInfo request;
+  request.method = "GET";
+  request.url = GURL("http://www.foo.com/");
+  request.load_flags = 0;
+
+  MockRead data1_reads[] = {
+    MockRead("HTTP/1.1 304 Not Modified\r\n\r\n"),
+    MockRead("HTTP/1.1 200 OK\r\nContent-Length: 5\r\n\r\n"),
+    MockRead("hello"),
+  };
+  StaticSocketDataProvider data1(data1_reads, arraysize(data1_reads), NULL, 0);
+  session_deps.socket_factory.AddSocketDataProvider(&data1);
+
+  MockRead data2_reads[] = {
+    MockRead(false, ERR_UNEXPECTED),  // Should not be reached.
+  };
+  StaticSocketDataProvider data2(data2_reads, arraysize(data2_reads), NULL, 0);
+  session_deps.socket_factory.AddSocketDataProvider(&data2);
+
+  for (int i = 0; i < 2; ++i) {
+    TestCompletionCallback callback;
+
+    scoped_ptr<HttpTransaction> trans(new HttpNetworkTransaction(session));
+
+    int rv = trans->Start(&request, &callback, BoundNetLog());
+    EXPECT_EQ(ERR_IO_PENDING, rv);
+
+    rv = callback.WaitForResult();
+    EXPECT_EQ(OK, rv);
+
+    const HttpResponseInfo* response = trans->GetResponseInfo();
+    EXPECT_TRUE(response != NULL);
+
+    EXPECT_TRUE(response->headers != NULL);
+    if (i == 0) {
+      EXPECT_EQ("HTTP/1.1 304 Not Modified",
+                response->headers->GetStatusLine());
+      // We intentionally don't read the response in this case, to reflect how
+      // HttpCache::Transaction uses HttpNetworkTransaction.
+    } else {
+      EXPECT_EQ("HTTP/1.1 200 OK", response->headers->GetStatusLine());
+      std::string response_data;
+      rv = ReadTransaction(trans.get(), &response_data);
+      EXPECT_EQ(OK, rv);
+      EXPECT_EQ("hello", response_data);
+    }
+  }
+}
+
 // Test the request-challenge-retry sequence for basic auth.
 // (basic auth is the easiest to mock, because it has no randomness).
 TEST_F(HttpNetworkTransactionTest, BasicAuth) {