Fix crash in client_socket_pool_base.cc.

The CHECK that was being hit showed a real problem: group's IsEmpty
should take into account the pending requests queue too.

I also made IsEmpty check for connecting requests queue emptiness,
so we can be sure that IsEmpty really means empty.

TEST=Added a regression test ClientSocketPoolBaseTest.GroupWithPendingRequestsIsNotEmpty to net_unittests.
http://crbug.com/17985

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

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

3 files changed, 75 insertions, 6 deletions

diff --git a/net/socket/client_socket_pool_base.cc b/net/socket/client_socket_pool_base.cc
index d10cf60..db3858d 100644
--- a/net/socket/client_socket_pool_base.cc
+++ b/net/socket/client_socket_pool_base.cc
@@ -146,9 +146,8 @@ int ClientSocketPoolBase::RequestSocket(
       connect_job_map_[handle] = job;
     }
     group.jobs.insert(job);
-  } else {
-    if (group.IsEmpty())
-      group_map_.erase(group_name);
+  } else if (group.IsEmpty()) {
+    group_map_.erase(group_name);
   }
 
   return rv;
@@ -281,7 +280,6 @@ void ClientSocketPoolBase::CleanupIdleSockets(bool force) {
 
     // Delete group if no longer needed.
     if (group.IsEmpty()) {
-      CHECK(group.pending_requests.empty());
       group_map_.erase(i++);
     } else {
       ++i;
@@ -521,7 +519,6 @@ void ClientSocketPoolBase::CancelAllConnectJobs() {
 
     // Delete group if no longer needed.
     if (group.IsEmpty()) {
-      CHECK(group.pending_requests.empty());
       group_map_.erase(i++);
     } else {
       ++i;
diff --git a/net/socket/client_socket_pool_base.h b/net/socket/client_socket_pool_base.h
index 36281b4..21cd642 100644
--- a/net/socket/client_socket_pool_base.h
+++ b/net/socket/client_socket_pool_base.h
@@ -196,7 +196,8 @@ class ClientSocketPoolBase
     Group() : active_socket_count(0) {}
 
     bool IsEmpty() const {
-      return active_socket_count == 0 && idle_sockets.empty() && jobs.empty();
+      return active_socket_count == 0 && idle_sockets.empty() && jobs.empty() &&
+          pending_requests.empty() && connecting_requests.empty();
     }
 
     bool HasAvailableSocketSlot(int max_sockets_per_group) const {
diff --git a/net/socket/client_socket_pool_base_unittest.cc b/net/socket/client_socket_pool_base_unittest.cc
index 730e041..dcae067b 100644
--- a/net/socket/client_socket_pool_base_unittest.cc
+++ b/net/socket/client_socket_pool_base_unittest.cc
@@ -974,6 +974,41 @@ TEST_F(ClientSocketPoolBaseTest, ReleaseSockets) {
   EXPECT_EQ(0, pool_->IdleSocketCountInGroup("a"));
 }
 
+// Regression test for http://crbug.com/17985.
+TEST_F(ClientSocketPoolBaseTest, GroupWithPendingRequestsIsNotEmpty) {
+  const int kMaxSockets = 3;
+  const int kMaxSocketsPerGroup = 2;
+  CreatePool(kMaxSockets, kMaxSocketsPerGroup);
+
+  const int kHighPriority = kDefaultPriority + 100;
+
+  EXPECT_EQ(OK, StartRequest("a", kDefaultPriority));
+  EXPECT_EQ(OK, StartRequest("a", kDefaultPriority));
+
+  // This is going to be a pending request in an otherwise empty group.
+  EXPECT_EQ(ERR_IO_PENDING, StartRequest("a", kDefaultPriority));
+
+  // Reach the maximum socket limit.
+  EXPECT_EQ(OK, StartRequest("b", kDefaultPriority));
+
+  // Create a stalled group with high priorities.
+  EXPECT_EQ(ERR_IO_PENDING, StartRequest("c", kHighPriority));
+  EXPECT_EQ(ERR_IO_PENDING, StartRequest("c", kHighPriority));
+  EXPECT_TRUE(pool_->base()->may_have_stalled_group());
+
+  // Release the first two sockets from "a", which will make room
+  // for requests from "c". After that "a" will have no active sockets
+  // and one pending request.
+  EXPECT_TRUE(ReleaseOneConnection(KEEP_ALIVE));
+  EXPECT_TRUE(ReleaseOneConnection(KEEP_ALIVE));
+
+  // Closing idle sockets should not get us into trouble, but in the bug
+  // we were hitting a CHECK here.
+  EXPECT_EQ(2, pool_->IdleSocketCountInGroup("a"));
+  pool_->CloseIdleSockets();
+  EXPECT_EQ(0, pool_->IdleSocketCountInGroup("a"));
+}
+
 class ClientSocketPoolBaseTest_LateBinding : public ClientSocketPoolBaseTest {
  protected:
   virtual void SetUp() {
@@ -1376,6 +1411,42 @@ TEST_F(ClientSocketPoolBaseTest_LateBinding, DISABLED_LoadState) {
   EXPECT_EQ(LOAD_STATE_WAITING_FOR_CACHE, req2.handle()->GetLoadState());
 }
 
+// Regression test for http://crbug.com/17985.
+TEST_F(ClientSocketPoolBaseTest_LateBinding,
+       GroupWithPendingRequestsIsNotEmpty) {
+  const int kMaxSockets = 3;
+  const int kMaxSocketsPerGroup = 2;
+  CreatePool(kMaxSockets, kMaxSocketsPerGroup);
+
+  const int kHighPriority = kDefaultPriority + 100;
+
+  EXPECT_EQ(OK, StartRequest("a", kDefaultPriority));
+  EXPECT_EQ(OK, StartRequest("a", kDefaultPriority));
+
+  // This is going to be a pending request in an otherwise empty group.
+  EXPECT_EQ(ERR_IO_PENDING, StartRequest("a", kDefaultPriority));
+
+  // Reach the maximum socket limit.
+  EXPECT_EQ(OK, StartRequest("b", kDefaultPriority));
+
+  // Create a stalled group with high priorities.
+  EXPECT_EQ(ERR_IO_PENDING, StartRequest("c", kHighPriority));
+  EXPECT_EQ(ERR_IO_PENDING, StartRequest("c", kHighPriority));
+  EXPECT_TRUE(pool_->base()->may_have_stalled_group());
+
+  // Release the first two sockets from "a", which will make room
+  // for requests from "c". After that "a" will have no active sockets
+  // and one pending request.
+  EXPECT_TRUE(ReleaseOneConnection(KEEP_ALIVE));
+  EXPECT_TRUE(ReleaseOneConnection(KEEP_ALIVE));
+
+  // Closing idle sockets should not get us into trouble, but in the bug
+  // we were hitting a CHECK here.
+  EXPECT_EQ(2, pool_->IdleSocketCountInGroup("a"));
+  pool_->CloseIdleSockets();
+  EXPECT_EQ(0, pool_->IdleSocketCountInGroup("a"));
+}
+
 }  // namespace
 
 }  // namespace net