Add a boolean data member reading_body_from_socket_ to help

DoReadBodyComplete() determine whether a result of 0
actually comes from a socket read, indicating EOF.

In unit tests, need to call RunAllPending() before calling
idle_socket_count() to get an accurate idle socket count
because idle sockets are added to the connection pool with a
PostTask.

R=eroman
BUG=9880
Review URL: http://codereview.chromium.org/63139

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

3 files changed, 90 insertions, 3 deletions

diff --git a/net/http/http_network_transaction.cc b/net/http/http_network_transaction.cc
index 36a752f..e69e8fe 100644
--- a/net/http/http_network_transaction.cc
+++ b/net/http/http_network_transaction.cc
@@ -49,6 +49,7 @@ HttpNetworkTransaction::HttpNetworkTransaction(HttpNetworkSession* session,
       using_proxy_(false),
       using_tunnel_(false),
       establishing_tunnel_(false),
+      reading_body_from_socket_(false),
       request_headers_bytes_sent_(0),
       header_buf_capacity_(0),
       header_buf_len_(0),
@@ -832,6 +833,7 @@ int HttpNetworkTransaction::DoReadBody() {
     return n;
   }
 
+  reading_body_from_socket_ = true;
   return connection_.socket()->Read(read_buf_->data(), read_buf_len_,
                                     &io_callback_);
 }
@@ -841,7 +843,8 @@ int HttpNetworkTransaction::DoReadBodyComplete(int result) {
   DCHECK(!establishing_tunnel_) <<
       "We should never read a response body of a tunnel.";
 
-  bool unfiltered_eof = (result == 0);
+  bool unfiltered_eof = (result == 0 && reading_body_from_socket_);
+  reading_body_from_socket_ = false;
 
   // Filter incoming data if appropriate.  FilterBuf may return an error.
   if (result > 0 && chunked_decoder_.get()) {
@@ -905,7 +908,8 @@ int HttpNetworkTransaction::DoDrainBodyForAuthRestart() {
 // method are almost the same.  Figure out a good way for these two methods
 // to share code.
 int HttpNetworkTransaction::DoDrainBodyForAuthRestartComplete(int result) {
-  bool unfiltered_eof = (result == 0);
+  bool unfiltered_eof = (result == 0 && reading_body_from_socket_);
+  reading_body_from_socket_ = false;
 
   // Filter incoming data if appropriate.  FilterBuf may return an error.
   if (result > 0 && chunked_decoder_.get()) {
diff --git a/net/http/http_network_transaction.h b/net/http/http_network_transaction.h
index a966086..5fb48f1 100644
--- a/net/http/http_network_transaction.h
+++ b/net/http/http_network_transaction.h
@@ -239,6 +239,23 @@ class HttpNetworkTransaction : public HttpTransaction {
   // the real request/response of the transaction.
   bool establishing_tunnel_;
 
+  // Only used between the states
+  // STATE_READ_BODY/STATE_DRAIN_BODY_FOR_AUTH and
+  // STATE_READ_BODY_COMPLETE/STATE_DRAIN_BODY_FOR_AUTH_COMPLETE.
+  //
+  // Set to true when DoReadBody or DoDrainBodyForAuthRestart starts to read
+  // the response body from the socket, and set to false when the socket read
+  // call completes. DoReadBodyComplete and DoDrainBodyForAuthRestartComplete
+  // use this boolean to disambiguate a |result| of 0 between a connection
+  // closure (EOF) and reaching the end of the response body (no more data).
+  //
+  // TODO(wtc): this is similar to the |ignore_ok_result_| member of the
+  // SSLClientSocketWin class.  We may want to add an internal error code, say
+  // ERR_EOF, to indicate a connection closure, so that 0 simply means 0 bytes
+  // or OK.  Note that we already have an ERR_CONNECTION_CLOSED error code,
+  // but it isn't really being used.
+  bool reading_body_from_socket_;
+
   SSLConfig ssl_config_;
 
   std::string request_headers_;
diff --git a/net/http/http_network_transaction_unittest.cc b/net/http/http_network_transaction_unittest.cc
index 05d1ac7..f497957 100644
--- a/net/http/http_network_transaction_unittest.cc
+++ b/net/http/http_network_transaction_unittest.cc
@@ -2188,14 +2188,80 @@ TEST_F(HttpNetworkTransactionTest, DontRecycleTCPSocketForSSLTunnel) {
   const net::HttpResponseInfo* response = trans->GetResponseInfo();
   EXPECT_TRUE(response == NULL);
 
+  // Empty the current queue.  This is necessary because idle sockets are
+  // added to the connection pool asynchronously with a PostTask.
+  MessageLoop::current()->RunAllPending();
+
   // We now check to make sure the TCPClientSocket was not added back to
   // the pool.
   EXPECT_EQ(0, session->connection_pool()->idle_socket_count());
   trans.reset();
+  MessageLoop::current()->RunAllPending();
   // Make sure that the socket didn't get recycled after calling the destructor.
   EXPECT_EQ(0, session->connection_pool()->idle_socket_count());
 }
 
+// Make sure that we recycle a socket after a zero-length response.
+// http://crbug.com/9880
+TEST_F(HttpNetworkTransactionTest, RecycleSocketAfterZeroContentLength) {
+  scoped_ptr<net::ProxyService> proxy_service(CreateNullProxyService());
+  scoped_refptr<net::HttpNetworkSession> session(
+      CreateSession(proxy_service.get()));
+
+  scoped_ptr<net::HttpTransaction> trans(new net::HttpNetworkTransaction(
+      session.get(), &mock_socket_factory));
+
+  net::HttpRequestInfo request;
+  request.method = "GET";
+  request.url = GURL("http://www.google.com/csi?v=3&s=web&action=&"
+                     "tran=undefined&ei=mAXcSeegAo-SMurloeUN&"
+                     "e=17259,18167,19592,19773,19981,20133,20173,20233&"
+                     "rt=prt.2642,ol.2649,xjs.2951");
+  request.load_flags = 0;
+
+  MockRead data_reads[] = {
+    MockRead("HTTP/1.1 204 No Content\r\n"
+             "Content-Length: 0\r\n"
+             "Content-Type: text/html\r\n\r\n"),
+    MockRead("junk"),  // Should not be read!!
+    MockRead(false, net::OK),
+  };
+
+  MockSocket data;
+  data.reads = data_reads;
+  mock_sockets[0] = &data;
+  mock_sockets[1] = NULL;
+
+  TestCompletionCallback callback;
+
+  int rv = trans->Start(&request, &callback);
+  EXPECT_EQ(net::ERR_IO_PENDING, rv);
+
+  rv = callback.WaitForResult();
+  EXPECT_EQ(net::OK, rv);
+
+  const net::HttpResponseInfo* response = trans->GetResponseInfo();
+  EXPECT_TRUE(response != NULL);
+
+  EXPECT_TRUE(response->headers != NULL);
+  std::string status_line = response->headers->GetStatusLine();
+  EXPECT_EQ("HTTP/1.1 204 No Content", status_line);
+
+  EXPECT_EQ(0, session->connection_pool()->idle_socket_count());
+
+  std::string response_data;
+  rv = ReadTransaction(trans.get(), &response_data);
+  EXPECT_EQ(net::OK, rv);
+  EXPECT_EQ("", response_data);
+
+  // Empty the current queue.  This is necessary because idle sockets are
+  // added to the connection pool asynchronously with a PostTask.
+  MessageLoop::current()->RunAllPending();
+
+  // We now check to make sure the socket was added back to the pool.
+  EXPECT_EQ(1, session->connection_pool()->idle_socket_count());
+}
+
 TEST_F(HttpNetworkTransactionTest, ResendRequestOnWriteBodyError) {
   net::HttpRequestInfo request[2];
   // Transaction 1: a GET request that succeeds.  The socket is recycled
@@ -2812,7 +2878,7 @@ TEST_F(HttpNetworkTransactionTest, ResetStateForRestart) {
   trans->response_.auth_challenge = new AuthChallengeInfo();
   trans->response_.ssl_info.cert_status = -15;
   trans->response_.response_time = base::Time::Now();
-  trans->response_.was_cached = true; // (Wouldn't ever actually be true...)
+  trans->response_.was_cached = true;  // (Wouldn't ever actually be true...)
 
   { // Setup state for response_.vary_data
     HttpRequestInfo request;