Revert "Make TCPClientSocketPool own the ConnectingSockets."

This reverts r18414.
Broke reliability bot.

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

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

net/base/tcp_client_socket_pool.cc

@@ -45,15 +45,20 @@ TCPClientSocketPool::ConnectingSocket::ConnectingSocket(
           callback_(this,
                     &TCPClientSocketPool::ConnectingSocket::OnIOComplete)),
       pool_(pool),
-      resolver_(pool->GetHostResolver()) {}
+      resolver_(pool->GetHostResolver()),
+      canceled_(false) {
+  CHECK(!ContainsKey(pool_->connecting_socket_map_, handle));
+  pool_->connecting_socket_map_[handle] = this;
+}
 
 TCPClientSocketPool::ConnectingSocket::~ConnectingSocket() {
-  // We don't worry about cancelling the host resolution and TCP connect, since
-  // ~SingleRequestHostResolver and ~ClientSocket will take care of it.
+  if (!canceled_)
+    pool_->connecting_socket_map_.erase(handle_);
 }
 
 int TCPClientSocketPool::ConnectingSocket::Connect(
     const HostResolver::RequestInfo& resolve_info) {
+  CHECK(!canceled_);
   int rv = resolver_.Resolve(resolve_info, &addresses_, &callback_);
   if (rv != ERR_IO_PENDING)
     rv = OnIOCompleteInternal(rv, true /* synchronous */);
@@ -68,14 +73,30 @@ int TCPClientSocketPool::ConnectingSocket::OnIOCompleteInternal(
     int result, bool synchronous) {
   CHECK(result != ERR_IO_PENDING);
 
+  if (canceled_) {
+    // We got canceled, so bail out.
+    delete this;
+    return result;
+  }
+
   GroupMap::iterator group_it = pool_->group_map_.find(group_name_);
-  CHECK(group_it != pool_->group_map_.end());
+  if (group_it == pool_->group_map_.end()) {
+    // The request corresponding to this ConnectingSocket has been canceled.
+    // Stop bothering with it.
+    delete this;
+    return result;
+  }
 
   Group& group = group_it->second;
 
   RequestMap* request_map = &group.connecting_requests;
   RequestMap::iterator it = request_map->find(handle_);
-  CHECK(it != request_map->end());
+  if (it == request_map->end()) {
+    // The request corresponding to this ConnectingSocket has been canceled.
+    // Stop bothering with it.
+    delete this;
+    return result;
+  }
 
   if (result == OK && it->second.load_state == LOAD_STATE_RESOLVING_HOST) {
     it->second.load_state = LOAD_STATE_CONNECTING;
@@ -87,7 +108,6 @@ int TCPClientSocketPool::ConnectingSocket::OnIOCompleteInternal(
   }
 
   if (result == OK) {
-    CHECK(it->second.load_state == LOAD_STATE_CONNECTING);
     CHECK(connect_start_time_ != base::Time());
     base::TimeDelta connect_duration =
         base::Time::Now() - connect_start_time_;
@@ -121,10 +141,17 @@ int TCPClientSocketPool::ConnectingSocket::OnIOCompleteInternal(
 
   if (!synchronous)
     request.callback->Run(result);
-  pool_->RemoveConnectingSocket(handle_);  // will delete |this|.
+  delete this;
   return result;
 }
 
+void TCPClientSocketPool::ConnectingSocket::Cancel() {
+  CHECK(!canceled_);
+  CHECK(ContainsKey(pool_->connecting_socket_map_, handle_));
+  pool_->connecting_socket_map_.erase(handle_);
+  canceled_ = true;
+}
+
 TCPClientSocketPool::TCPClientSocketPool(
     int max_sockets_per_group,
     HostResolver* host_resolver,
@@ -141,7 +168,6 @@ TCPClientSocketPool::~TCPClientSocketPool() {
   // to the manager being destroyed.
   CloseIdleSockets();
   DCHECK(group_map_.empty());
-  DCHECK(connecting_socket_map_.empty());
 }
 
 // InsertRequestIntoQueue inserts the request into the queue based on
@@ -193,16 +219,20 @@ int TCPClientSocketPool::RequestSocket(
 
   // We couldn't find a socket to reuse, so allocate and connect a new one.
 
+  // First, we need to make sure we aren't already servicing a request for this
+  // handle (which could happen if we requested, canceled, and then requested
+  // with the same handle).
+  if (ContainsKey(connecting_socket_map_, handle))
+    connecting_socket_map_[handle]->Cancel();
+
   CHECK(callback);
   Request r(handle, callback, priority, resolve_info,
             LOAD_STATE_RESOLVING_HOST);
   group_map_[group_name].connecting_requests[handle] = r;
 
-  CHECK(!ContainsKey(connecting_socket_map_, handle));
-
+  // connecting_socket will delete itself.
   ConnectingSocket* connecting_socket =
       new ConnectingSocket(group_name, handle, client_socket_factory_, this);
-  connecting_socket_map_[handle] = connecting_socket;
   int rv = connecting_socket->Connect(resolve_info);
   return rv;
 }
@@ -227,8 +257,6 @@ void TCPClientSocketPool::CancelRequest(const std::string& group_name,
 
   RequestMap::iterator map_it = group.connecting_requests.find(handle);
   if (map_it != group.connecting_requests.end()) {
-    RemoveConnectingSocket(handle);
-
     group.connecting_requests.erase(map_it);
     group.active_socket_count--;
 
@@ -391,12 +419,4 @@ void TCPClientSocketPool::DoReleaseSocket(const std::string& group_name,
   }
 }
 
-void TCPClientSocketPool::RemoveConnectingSocket(
-    const ClientSocketHandle* handle) {
-  ConnectingSocketMap::iterator it = connecting_socket_map_.find(handle);
-  CHECK(it != connecting_socket_map_.end());
-  delete it->second;
-  connecting_socket_map_.erase(it);
-}
-
 }  // namespace net

net/base/tcp_client_socket_pool.h

@@ -154,6 +154,7 @@ class TCPClientSocketPool : public ClientSocketPool {
     scoped_refptr<TCPClientSocketPool> pool_;
     SingleRequestHostResolver resolver_;
     AddressList addresses_;
+    bool canceled_;
 
     // The time the Connect() method was called (if it got called).
     base::Time connect_start_time_;
@@ -161,9 +162,6 @@ class TCPClientSocketPool : public ClientSocketPool {
     DISALLOW_COPY_AND_ASSIGN(ConnectingSocket);
   };
 
-  typedef std::map<const ClientSocketHandle*, ConnectingSocket*>
-      ConnectingSocketMap;
-
   virtual ~TCPClientSocketPool();
 
   static void InsertRequestIntoQueue(const Request& r,
@@ -186,15 +184,11 @@ class TCPClientSocketPool : public ClientSocketPool {
     CleanupIdleSockets(false);
   }
 
-  // Removes the ConnectingSocket corresponding to |handle| from the
-  // |connecting_socket_map_|.
-  void RemoveConnectingSocket(const ClientSocketHandle* handle);
-
   ClientSocketFactory* const client_socket_factory_;
 
   GroupMap group_map_;
 
-  ConnectingSocketMap connecting_socket_map_;
+  std::map<const ClientSocketHandle*, ConnectingSocket*> connecting_socket_map_;
 
   // Timer used to periodically prune idle sockets that timed out or can't be
   // reused.

net/url_request/url_request_unittest.cc

@@ -45,7 +45,10 @@ namespace {
 class URLRequestHttpCacheContext : public URLRequestContext {
  public:
   URLRequestHttpCacheContext() {
-    host_resolver_ = new net::HostResolver;
+    // TODO(eroman): we turn off host caching to see if synchronous
+    // host resolving interacts poorly with client socket pool. [experiment]
+    // http://crbug.com/13952
+    host_resolver_ = new net::HostResolver(0, 0);
     proxy_service_ = net::ProxyService::CreateNull();
     http_transaction_factory_ =
         new net::HttpCache(
@@ -315,7 +318,8 @@ TEST_F(HTTPSRequestTest, MAYBE_HTTPSExpiredTest) {
   }
 }
 
-TEST_F(URLRequestTest, CancelTest) {
+// http://crbug.com/13952
+TEST_F(URLRequestTest, DISABLED_CancelTest) {
   TestDelegate d;
   {
     TestURLRequest r(GURL("http://www.google.com/"), &d);
@@ -338,7 +342,8 @@ TEST_F(URLRequestTest, CancelTest) {
 #endif
 }
 
-TEST_F(URLRequestTest, CancelTest2) {
+// http://crbug.com/13952
+TEST_F(URLRequestTest, DISABLED_CancelTest2) {
   scoped_refptr<HTTPTestServer> server =
       HTTPTestServer::CreateServer(L"", NULL);
   ASSERT_TRUE(NULL != server.get());
@@ -367,7 +372,8 @@ TEST_F(URLRequestTest, CancelTest2) {
 #endif
 }
 
-TEST_F(URLRequestTest, CancelTest3) {
+// http://crbug.com/13952
+TEST_F(URLRequestTest, DISABLED_CancelTest3) {
   scoped_refptr<HTTPTestServer> server =
       HTTPTestServer::CreateServer(L"", NULL);
   ASSERT_TRUE(NULL != server.get());
@@ -395,7 +401,8 @@ TEST_F(URLRequestTest, CancelTest3) {
 #endif
 }
 
-TEST_F(URLRequestTest, CancelTest4) {
+// http://crbug.com/13952
+TEST_F(URLRequestTest, DISABLED_CancelTest4) {
   scoped_refptr<HTTPTestServer> server =
       HTTPTestServer::CreateServer(L"", NULL);
   ASSERT_TRUE(NULL != server.get());

net/url_request/url_request_unittest.h

@@ -43,7 +43,10 @@ using base::TimeDelta;
 class TestURLRequestContext : public URLRequestContext {
  public:
   TestURLRequestContext() {
-    host_resolver_ = new net::HostResolver;
+    // TODO(eroman): we turn off host caching to see if synchronous
+    // host resolving interacts poorly with client socket pool. [experiment]
+    // http://crbug.com/13952
+    host_resolver_ = new net::HostResolver(0, 0);
     proxy_service_ = net::ProxyService::CreateNull();
     http_transaction_factory_ =
         net::HttpNetworkLayer::CreateFactory(host_resolver_,