Fix bug that would create unnecessary ConnectJobs in some cases.

When a ConnectJob failed and there was another socket request pending
for the same group, a new ConnectJob would always be created, even if
the pending request already had its own ConnectJob.

BUG=463960

Review URL: https://codereview.chromium.org/981643002

Cr-Commit-Position: refs/heads/master@{#319750}

3 files changed, 34 insertions, 8 deletions

diff --git a/net/socket/client_socket_pool_base.cc b/net/socket/client_socket_pool_base.cc
index 4bc44f5..628fdd0 100644
--- a/net/socket/client_socket_pool_base.cc
+++ b/net/socket/client_socket_pool_base.cc
@@ -226,7 +226,7 @@ bool ClientSocketPoolBaseHelper::IsStalled() const {
   // which does not count.)
   for (GroupMap::const_iterator it = group_map_.begin();
        it != group_map_.end(); ++it) {
-    if (it->second->IsStalledOnPoolMaxSockets(max_sockets_per_group_))
+    if (it->second->CanUseAdditionalSocketSlot(max_sockets_per_group_))
       return true;
   }
   return false;
@@ -278,7 +278,7 @@ int ClientSocketPoolBaseHelper::RequestSocket(
     // call back in to |this|, which will cause all sorts of fun and exciting
     // re-entrancy issues if the socket pool is doing something else at the
     // time.
-    if (group->IsStalledOnPoolMaxSockets(max_sockets_per_group_)) {
+    if (group->CanUseAdditionalSocketSlot(max_sockets_per_group_)) {
       base::MessageLoop::current()->PostTask(
           FROM_HERE,
           base::Bind(
@@ -578,7 +578,7 @@ LoadState ClientSocketPoolBaseHelper::GetLoadState(
     return max_state;
   }
 
-  if (group.IsStalledOnPoolMaxSockets(max_sockets_per_group_))
+  if (group.CanUseAdditionalSocketSlot(max_sockets_per_group_))
     return LOAD_STATE_WAITING_FOR_STALLED_SOCKET_POOL;
   return LOAD_STATE_WAITING_FOR_AVAILABLE_SOCKET;
 }
@@ -632,9 +632,8 @@ base::DictionaryValue* ClientSocketPoolBaseHelper::GetInfoAsValue(
     }
     group_dict->Set("connect_jobs", connect_jobs_list);
 
-    group_dict->SetBoolean("is_stalled",
-                           group->IsStalledOnPoolMaxSockets(
-                               max_sockets_per_group_));
+    group_dict->SetBoolean("is_stalled", group->CanUseAdditionalSocketSlot(
+                                             max_sockets_per_group_));
     group_dict->SetBoolean("backup_job_timer_is_running",
                            group->BackupJobTimerIsRunning());
 
@@ -836,7 +835,7 @@ bool ClientSocketPoolBaseHelper::FindTopStalledGroup(
     Group* curr_group = i->second;
     if (!curr_group->has_pending_requests())
       continue;
-    if (curr_group->IsStalledOnPoolMaxSockets(max_sockets_per_group_)) {
+    if (curr_group->CanUseAdditionalSocketSlot(max_sockets_per_group_)) {
       if (!group)
         return true;
       has_stalled_group = true;
@@ -960,6 +959,15 @@ void ClientSocketPoolBaseHelper::ProcessPendingRequest(
     const std::string& group_name, Group* group) {
   const Request* next_request = group->GetNextPendingRequest();
   DCHECK(next_request);
+
+  // If the group has no idle sockets, and can't make use of an additional slot,
+  // either because it's at the limit or because it's at the socket per group
+  // limit, then there's nothing to do.
+  if (group->idle_sockets().empty() &&
+      !group->CanUseAdditionalSocketSlot(max_sockets_per_group_)) {
+    return;
+  }
+
   int rv = RequestSocketInternal(group_name, *next_request);
   if (rv != ERR_IO_PENDING) {
     scoped_ptr<const Request> request = group->PopNextPendingRequest();
diff --git a/net/socket/client_socket_pool_base.h b/net/socket/client_socket_pool_base.h
index a7dcbf5..76ae919 100644
--- a/net/socket/client_socket_pool_base.h
+++ b/net/socket/client_socket_pool_base.h
@@ -383,7 +383,9 @@ class NET_EXPORT_PRIVATE ClientSocketPoolBaseHelper
           static_cast<int>(idle_sockets_.size());
     }
 
-    bool IsStalledOnPoolMaxSockets(int max_sockets_per_group) const {
+    // Returns true if the group could make use of an additional socket slot, if
+    // it were given one.
+    bool CanUseAdditionalSocketSlot(int max_sockets_per_group) const {
       return HasAvailableSocketSlot(max_sockets_per_group) &&
           pending_requests_.size() > jobs_.size();
     }
diff --git a/net/socket/client_socket_pool_base_unittest.cc b/net/socket/client_socket_pool_base_unittest.cc
index c4a2845..c7cbda1 100644
--- a/net/socket/client_socket_pool_base_unittest.cc
+++ b/net/socket/client_socket_pool_base_unittest.cc
@@ -1750,6 +1750,22 @@ TEST_F(ClientSocketPoolBaseTest,
       entries, 2, NetLog::TYPE_SOCKET_POOL));
 }
 
+// Check that an async ConnectJob failure does not result in creation of a new
+// ConnectJob when there's another pending request also waiting on its own
+// ConnectJob.  See http://crbug.com/463960.
+TEST_F(ClientSocketPoolBaseTest, AsyncFailureWithPendingRequestWithJob) {
+  CreatePool(2, 2);
+  connect_job_factory_->set_job_type(TestConnectJob::kMockPendingFailingJob);
+
+  EXPECT_EQ(ERR_IO_PENDING, StartRequest("a", DEFAULT_PRIORITY));
+  EXPECT_EQ(ERR_IO_PENDING, StartRequest("a", DEFAULT_PRIORITY));
+
+  EXPECT_EQ(ERR_CONNECTION_FAILED, request(0)->WaitForResult());
+  EXPECT_EQ(ERR_CONNECTION_FAILED, request(1)->WaitForResult());
+
+  EXPECT_EQ(2, client_socket_factory_.allocation_count());
+}
+
 TEST_F(ClientSocketPoolBaseTest, TwoRequestsCancelOne) {
   // TODO(eroman): Add back the log expectations! Removed them because the
   //               ordering is difficult, and some may fire during destructor.