Fix a crash in ClientSocketPoolBaseHelper where we double erase a Group from the GroupMap.

BUG=49254

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

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

3 files changed, 54 insertions, 4 deletions

diff --git a/net/socket/client_socket_pool_base.cc b/net/socket/client_socket_pool_base.cc
index 2e8534e..3c2a9c7 100644
--- a/net/socket/client_socket_pool_base.cc
+++ b/net/socket/client_socket_pool_base.cc
@@ -401,6 +401,10 @@ void ClientSocketPoolBaseHelper::CancelRequest(
   }
 }
 
+bool ClientSocketPoolBaseHelper::HasGroup(const std::string& group_name) const {
+  return ContainsKey(group_map_, group_name);
+}
+
 void ClientSocketPoolBaseHelper::CloseIdleSockets() {
   CleanupIdleSockets(true);
 }
@@ -691,11 +695,11 @@ void ClientSocketPoolBaseHelper::RemoveConnectJob(const ConnectJob* job,
 
 void ClientSocketPoolBaseHelper::OnAvailableSocketSlot(
     const std::string& group_name, Group* group) {
-  if (!group->pending_requests.empty())
-    ProcessPendingRequest(group_name, group);
-
+  DCHECK(ContainsKey(group_map_, group_name));
   if (group->IsEmpty())
     group_map_.erase(group_name);
+  else if (!group->pending_requests.empty())
+    ProcessPendingRequest(group_name, group);
 }
 
 void ClientSocketPoolBaseHelper::ProcessPendingRequest(
@@ -705,6 +709,8 @@ void ClientSocketPoolBaseHelper::ProcessPendingRequest(
   if (rv != ERR_IO_PENDING) {
     scoped_ptr<const Request> request(RemoveRequestFromQueue(
           group->pending_requests.begin(), &group->pending_requests));
+    if (group->IsEmpty())
+      group_map_.erase(group_name);
 
     scoped_refptr<NetLog::EventParameters> params;
     if (rv != OK)
diff --git a/net/socket/client_socket_pool_base.h b/net/socket/client_socket_pool_base.h
index ee727f0..b1ea6e3 100644
--- a/net/socket/client_socket_pool_base.h
+++ b/net/socket/client_socket_pool_base.h
@@ -231,6 +231,8 @@ class ClientSocketPoolBaseHelper
     return group_map_.find(group_name)->second.jobs.size();
   }
 
+  bool HasGroup(const std::string& group_name) const;
+
   // Closes all idle sockets if |force| is true.  Else, only closes idle
   // sockets that timed out or can't be reused.  Made public for testing.
   void CleanupIdleSockets(bool force);
@@ -584,6 +586,10 @@ class ClientSocketPoolBase {
     return helper_->NumConnectJobsInGroup(group_name);
   }
 
+  bool HasGroup(const std::string& group_name) const {
+    return helper_->HasGroup(group_name);
+  }
+
   void CleanupIdleSockets(bool force) {
     return helper_->CleanupIdleSockets(force);
   }
diff --git a/net/socket/client_socket_pool_base_unittest.cc b/net/socket/client_socket_pool_base_unittest.cc
index d8ea1d9..89db61d 100644
--- a/net/socket/client_socket_pool_base_unittest.cc
+++ b/net/socket/client_socket_pool_base_unittest.cc
@@ -420,6 +420,10 @@ class TestClientSocketPool : public ClientSocketPool {
     return base_.NumConnectJobsInGroup(group_name);
   }
 
+  bool HasGroup(const std::string& group_name) const {
+    return base_.HasGroup(group_name);
+  }
+
   void CleanupTimedOutIdleSockets() { base_.CleanupIdleSockets(false); }
 
   void EnableConnectBackupJobs() { base_.EnableConnectBackupJobs(); }
@@ -2057,7 +2061,7 @@ TEST_F(ClientSocketPoolBaseTest, DelayedSocketBindingAtGroupCapacity) {
 
 // Test out the case where we have one socket connected, one
 // connecting, when the first socket finishes and goes idle.
-// Although the second connection is pending, th second request
+// Although the second connection is pending, the second request
 // should complete, by taking the first socket's idle socket.
 TEST_F(ClientSocketPoolBaseTest, DelayedSocketBindingAtStall) {
   CreatePool(kDefaultMaxSockets, kDefaultMaxSocketsPerGroup);
@@ -2105,6 +2109,40 @@ TEST_F(ClientSocketPoolBaseTest, DelayedSocketBindingAtStall) {
   MessageLoop::current()->RunAllPending();
 }
 
+// Cover the case where on an available socket slot, we have one pending
+// request that completes synchronously, thereby making the Group empty.
+TEST_F(ClientSocketPoolBaseTest, SynchronouslyProcessOnePendingRequest) {
+  const int kUnlimitedSockets = 100;
+  const int kOneSocketPerGroup = 1;
+  CreatePool(kUnlimitedSockets, kOneSocketPerGroup);
+
+  // Make the first request asynchronous fail.
+  // This will free up a socket slot later.
+  connect_job_factory_->set_job_type(TestConnectJob::kMockPendingFailingJob);
+
+  ClientSocketHandle handle1;
+  TestCompletionCallback callback1;
+  EXPECT_EQ(ERR_IO_PENDING, handle1.Init("a", params_, kDefaultPriority,
+                                         &callback1, pool_, BoundNetLog()));
+  EXPECT_EQ(1, pool_->NumConnectJobsInGroup("a"));
+
+  // Make the second request synchronously fail.  This should make the Group
+  // empty.
+  connect_job_factory_->set_job_type(TestConnectJob::kMockFailingJob);
+  ClientSocketHandle handle2;
+  TestCompletionCallback callback2;
+  // It'll be ERR_IO_PENDING now, but the TestConnectJob will synchronously fail
+  // when created.
+  EXPECT_EQ(ERR_IO_PENDING, handle2.Init("a", params_, kDefaultPriority,
+                                         &callback2, pool_, BoundNetLog()));
+
+  EXPECT_EQ(1, pool_->NumConnectJobsInGroup("a"));
+
+  EXPECT_EQ(ERR_CONNECTION_FAILED, callback1.WaitForResult());
+  EXPECT_EQ(ERR_CONNECTION_FAILED, callback2.WaitForResult());
+  EXPECT_FALSE(pool_->HasGroup("a"));
+}
+
 }  // namespace
 
 }  // namespace net