Fix crash in ClientSocketPoolBase.

If a ConnectingSocket fails, we need to try to process a pending request.
BUG=http://crbug.com/14814
TEST=See bug for repro steps.

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

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

3 files changed, 62 insertions, 48 deletions

diff --git a/net/socket/tcp_client_socket_pool.cc b/net/socket/tcp_client_socket_pool.cc
index 534d10a..e0530c3b 100644
--- a/net/socket/tcp_client_socket_pool.cc
+++ b/net/socket/tcp_client_socket_pool.cc
@@ -233,25 +233,9 @@ void ClientSocketPoolBase::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--;
-
-    if (!group.pending_requests.empty()) {
-      ProcessPendingRequest(group_name, &group);
-      return;  // |group| may be invalid after this, so return to be safe.
-    }
-
-    // Delete group if no longer needed.
-    if (group.active_socket_count == 0 && group.idle_sockets.empty()) {
-      CHECK(group.pending_requests.empty());
-      CHECK(group.connecting_requests.empty());
-      group_map_.erase(group_name);
-      return;  // |group| is invalid after this, so return to be safe.
-    }
+    RemoveActiveSocket(group_name, &group);
   }
-
-  CheckSocketCounts(group);
 }
 
 void ClientSocketPoolBase::ReleaseSocket(const std::string& group_name,
@@ -373,7 +357,6 @@ void ClientSocketPoolBase::DoReleaseSocket(const std::string& group_name,
   CHECK(group.active_socket_count > 0);
   CheckSocketCounts(group);
 
-  group.active_socket_count--;
   group.sockets_handed_out_count--;
 
   const bool can_reuse = socket->IsConnectedAndIdle();
@@ -388,20 +371,7 @@ void ClientSocketPoolBase::DoReleaseSocket(const std::string& group_name,
     delete socket;
   }
 
-  // Process one pending request.
-  if (!group.pending_requests.empty()) {
-    ProcessPendingRequest(group_name, &group);
-    return;
-  }
-
-  // Delete group if no longer needed.
-  if (group.active_socket_count == 0 && group.idle_sockets.empty()) {
-    CHECK(group.pending_requests.empty());
-    CHECK(group.connecting_requests.empty());
-    group_map_.erase(i);
-  } else {
-    CheckSocketCounts(group);
-  }
+  RemoveActiveSocket(group_name, &group);
 }
 
 ClientSocketPoolBase::Request* ClientSocketPoolBase::GetConnectingRequest(
@@ -441,16 +411,7 @@ CompletionCallback* ClientSocketPoolBase::OnConnectingRequestComplete(
   DCHECK_EQ(request.handle, handle);
 
   if (deactivate) {
-    group.active_socket_count--;
-
-    // Delete group if no longer needed.
-    if (group.active_socket_count == 0 && group.idle_sockets.empty()) {
-      DCHECK(group.pending_requests.empty());
-      DCHECK(group.connecting_requests.empty());
-      group_map_.erase(group_name);
-    } else {
-      CheckSocketCounts(group);
-    }
+    RemoveActiveSocket(group_name, &group);
   } else {
     request.handle->set_socket(socket);
     request.handle->set_is_reused(false);
@@ -482,6 +443,23 @@ void ClientSocketPoolBase::RemoveConnectingSocket(
   connecting_socket_map_.erase(it);
 }
 
+void ClientSocketPoolBase::RemoveActiveSocket(const std::string& group_name,
+                                              Group* group) {
+  group->active_socket_count--;
+
+  if (!group->pending_requests.empty()) {
+    ProcessPendingRequest(group_name, group);
+    // |group| may no longer be valid after this point.  Be careful not to
+    // access it again.
+  } else if (group->active_socket_count == 0 && group->idle_sockets.empty()) {
+    // Delete |group| if no longer needed.  |group| will no longer be valid.
+    DCHECK(group->connecting_requests.empty());
+    group_map_.erase(group_name);
+  } else {
+    CheckSocketCounts(*group);
+  }
+}
+
 void ClientSocketPoolBase::ProcessPendingRequest(const std::string& group_name,
                                                  Group* group) {
   Request r = group->pending_requests.front();
diff --git a/net/socket/tcp_client_socket_pool.h b/net/socket/tcp_client_socket_pool.h
index e237be5..421ea9a 100644
--- a/net/socket/tcp_client_socket_pool.h
+++ b/net/socket/tcp_client_socket_pool.h
@@ -240,6 +240,9 @@ class ClientSocketPoolBase : public base::RefCounted<ClientSocketPoolBase> {
 
   static void CheckSocketCounts(const Group& group);
 
+  // Remove an active socket.
+  void RemoveActiveSocket(const std::string& group_name, Group* group);
+
   // Process a request from a group's pending_requests queue.
   void ProcessPendingRequest(const std::string& group_name, Group* group);
 
diff --git a/net/socket/tcp_client_socket_pool_unittest.cc b/net/socket/tcp_client_socket_pool_unittest.cc
index 2bcc244..a76a439 100644
--- a/net/socket/tcp_client_socket_pool_unittest.cc
+++ b/net/socket/tcp_client_socket_pool_unittest.cc
@@ -95,8 +95,10 @@ class MockFailingClientSocket : public ClientSocket {
 
 class MockPendingClientSocket : public ClientSocket {
  public:
-  MockPendingClientSocket()
-      : method_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)) {}
+  MockPendingClientSocket(bool should_connect)
+      : method_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)),
+        should_connect_(should_connect),
+        is_connected_(false) {}
 
   // ClientSocket methods:
   virtual int Connect(CompletionCallback* callback) {
@@ -110,10 +112,10 @@ class MockPendingClientSocket : public ClientSocket {
   virtual void Disconnect() {}
 
   virtual bool IsConnected() const {
-    return false;
+    return is_connected_;
   }
   virtual bool IsConnectedAndIdle() const {
-    return false;
+    return is_connected_;
   }
 
   // Socket methods:
@@ -129,10 +131,18 @@ class MockPendingClientSocket : public ClientSocket {
 
  private:
   void DoCallback(CompletionCallback* callback) {
-    callback->Run(OK);
+    if (should_connect_) {
+      is_connected_ = true;
+      callback->Run(OK);
+    } else {
+      is_connected_ = false;
+      callback->Run(ERR_CONNECTION_FAILED);
+    }
   }
 
   ScopedRunnableMethodFactory<MockPendingClientSocket> method_factory_;
+  bool should_connect_;
+  bool is_connected_;
 };
 
 class MockClientSocketFactory : public ClientSocketFactory {
@@ -141,6 +151,7 @@ class MockClientSocketFactory : public ClientSocketFactory {
     MOCK_CLIENT_SOCKET,
     MOCK_FAILING_CLIENT_SOCKET,
     MOCK_PENDING_CLIENT_SOCKET,
+    MOCK_PENDING_FAILING_CLIENT_SOCKET,
   };
 
   MockClientSocketFactory()
@@ -154,7 +165,9 @@ class MockClientSocketFactory : public ClientSocketFactory {
       case MOCK_FAILING_CLIENT_SOCKET:
         return new MockFailingClientSocket();
       case MOCK_PENDING_CLIENT_SOCKET:
-        return new MockPendingClientSocket();
+        return new MockPendingClientSocket(true);
+      case MOCK_PENDING_FAILING_CLIENT_SOCKET:
+        return new MockPendingClientSocket(false);
       default:
         NOTREACHED();
         return new MockClientSocket();
@@ -583,6 +596,26 @@ TEST_F(TCPClientSocketPoolTest, CancelActiveRequestWithPendingRequests) {
   EXPECT_EQ(kNumPendingRequests, TestSocketRequest::completion_count);
 }
 
+// Make sure that pending requests get serviced after active requests fail.
+TEST_F(TCPClientSocketPoolTest, FailingActiveRequestWithPendingRequests) {
+  client_socket_factory_.set_client_socket_type(
+      MockClientSocketFactory::MOCK_PENDING_FAILING_CLIENT_SOCKET);
+
+  scoped_ptr<TestSocketRequest> reqs[kMaxSocketsPerGroup * 2 + 1];
+
+  // Queue up all the requests
+
+  HostResolver::RequestInfo info("www.google.com", 80);
+  for (size_t i = 0; i < arraysize(reqs); ++i) {
+    reqs[i].reset(new TestSocketRequest(pool_.get(), &request_order_));
+    int rv = reqs[i]->handle.Init("a", info, 5, reqs[i].get());
+    EXPECT_EQ(ERR_IO_PENDING, rv);
+  }
+
+  for (size_t i = 0; i < arraysize(reqs); ++i)
+    EXPECT_EQ(ERR_CONNECTION_FAILED, reqs[i]->WaitForResult());
+}
+
 }  // namespace
 
 }  // namespace net