Add a unit-test for r4624 (tcpclientsocket recycling on CONNECT).

Also corrects a style problem in r4624 (comment order does not match expression order).

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

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

3 files changed, 78 insertions, 3 deletions

diff --git a/net/base/client_socket_pool.h b/net/base/client_socket_pool.h
index 76a3dff..764f859 100644
--- a/net/base/client_socket_pool.h
+++ b/net/base/client_socket_pool.h
@@ -64,6 +64,11 @@ class ClientSocketPool : public base::RefCounted<ClientSocketPool> {
   // Called to close any idle connections held by the connection manager.
   void CloseIdleSockets();
 
+  // The total number of idle sockets in the pool.
+  int idle_socket_count() const {
+    return idle_socket_count_;
+  }
+
  private:
   friend class base::RefCounted<ClientSocketPool>;
 
diff --git a/net/http/http_network_transaction.cc b/net/http/http_network_transaction.cc
index 7beff25..fa4bf73 100644
--- a/net/http/http_network_transaction.cc
+++ b/net/http/http_network_transaction.cc
@@ -747,9 +747,10 @@ int HttpNetworkTransaction::DoReadBodyComplete(int result) {
       done = true;
       keep_alive = response_.headers->IsKeepAlive();
       // We can't reuse the connection if we read more than the advertised
-      // content length, or if the tunnel was not established.
-      if (establishing_tunnel_ || unfiltered_eof ||
-          (content_length_ != -1 && content_read_ > content_length_))
+      // content length, or if the tunnel was not established successfully.
+      if (unfiltered_eof ||
+          (content_length_ != -1 && content_read_ > content_length_) ||
+          establishing_tunnel_)
         keep_alive = false;
     }
   }
diff --git a/net/http/http_network_transaction_unittest.cc b/net/http/http_network_transaction_unittest.cc
index d5f0228..c362e04 100644
--- a/net/http/http_network_transaction_unittest.cc
+++ b/net/http/http_network_transaction_unittest.cc
@@ -856,3 +856,72 @@ TEST_F(HttpNetworkTransactionTest, LargeHeadersNoBody) {
   const net::HttpResponseInfo* response = trans->GetResponseInfo();
   EXPECT_TRUE(response == NULL);
 }
+
+// Make sure that we don't try to reuse a TCPClientSocket when failing to
+// establish tunnel.
+// http://code.google.com/p/chromium/issues/detail?id=3772
+TEST_F(HttpNetworkTransactionTest, DontRecycleTCPSocketForSSLTunnel) {
+  // Configure against proxy server "myproxy:70".
+  net::ProxyInfo proxy_info;
+  proxy_info.UseNamedProxy("myproxy:70");
+
+  scoped_refptr<net::HttpNetworkSession> session(
+      CreateSession(new net::ProxyResolverFixed(proxy_info)));
+
+  scoped_ptr<net::HttpTransaction> trans(new net::HttpNetworkTransaction(
+      session.get(), &mock_socket_factory));
+
+  net::HttpRequestInfo request;
+  request.method = "GET";
+  request.url = GURL("https://www.google.com/");
+  request.load_flags = 0;
+
+  // Since we have proxy, should try to establish tunnel.
+  MockWrite data_writes1[] = {
+    MockWrite("CONNECT www.google.com:443 HTTP/1.1\r\n"
+              "Host: www.google.com\r\n\r\n"),
+  };
+
+  // The proxy responds to the connect with a 404, using a persistent 
+  // connection. Usually a proxy would return 501 (not implemented),
+  // or 200 (tunnel established).
+  MockRead data_reads1[] = {
+    MockRead("HTTP/1.1 404 Not Found\r\n"),
+    MockRead("Content-Length: 10\r\n\r\n"),
+    MockRead("0123456789"),
+    MockRead(false, net::ERR_UNEXPECTED), // Should not be reached.
+  };
+
+  MockSocket data1;
+  data1.writes = data_writes1;
+  data1.reads = data_reads1;
+  mock_sockets[0] = &data1;
+  mock_sockets[1] = NULL;
+
+  TestCompletionCallback callback1;
+
+  int rv = trans->Start(&request, &callback1);
+  EXPECT_EQ(net::ERR_IO_PENDING, rv);
+
+  rv = callback1.WaitForResult();
+  EXPECT_EQ(net::OK, rv);
+
+  const net::HttpResponseInfo* response = trans->GetResponseInfo();
+  EXPECT_FALSE(response == NULL);
+
+  EXPECT_TRUE(response->headers->IsKeepAlive());
+  EXPECT_EQ(404, response->headers->response_code());
+  EXPECT_EQ(10, response->headers->GetContentLength());
+  EXPECT_TRUE(net::HttpVersion(1, 1) == response->headers->GetHttpVersion());
+  
+  std::string response_data;
+  rv = ReadTransaction(trans.get(), &response_data);
+  EXPECT_STREQ("0123456789", response_data.c_str());
+
+  // 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();
+  // Make sure that the socket didn't get recycled after calling the destructor.
+  EXPECT_EQ(0, session->connection_pool()->idle_socket_count());
+}