Fix crash when we delete the group we're requesting a socket for.

BUG=40952

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

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

4 files changed, 55 insertions, 20 deletions

diff --git a/net/socket/client_socket_pool_base.cc b/net/socket/client_socket_pool_base.cc
index 9da9a19..c635a20 100644
--- a/net/socket/client_socket_pool_base.cc
+++ b/net/socket/client_socket_pool_base.cc
@@ -182,6 +182,23 @@ int ClientSocketPoolBaseHelper::RequestSocketInternal(
   CHECK(handle);
   Group& group = group_map_[group_name];
 
+  // Try to reuse a socket.
+  while (!group.idle_sockets.empty()) {
+    IdleSocket idle_socket = group.idle_sockets.back();
+    group.idle_sockets.pop_back();
+    DecrementIdleCount();
+    if (idle_socket.socket->IsConnectedAndIdle()) {
+      // We found one we can reuse!
+      base::TimeDelta idle_time =
+          base::TimeTicks::Now() - idle_socket.start_time;
+      HandOutSocket(
+          idle_socket.socket, idle_socket.used, handle, idle_time, &group,
+          request->net_log());
+      return OK;
+    }
+    delete idle_socket.socket;
+  }
+
   // Can we make another active socket now?
   if (!group.HasAvailableSocketSlot(max_sockets_per_group_)) {
     request->net_log().AddEvent(
@@ -201,23 +218,6 @@ int ClientSocketPoolBaseHelper::RequestSocketInternal(
     }
   }
 
-  // Try to reuse a socket.
-  while (!group.idle_sockets.empty()) {
-    IdleSocket idle_socket = group.idle_sockets.back();
-    group.idle_sockets.pop_back();
-    DecrementIdleCount();
-    if (idle_socket.socket->IsConnectedAndIdle()) {
-      // We found one we can reuse!
-      base::TimeDelta idle_time =
-          base::TimeTicks::Now() - idle_socket.start_time;
-      HandOutSocket(
-          idle_socket.socket, idle_socket.used, handle, idle_time, &group,
-          request->net_log());
-      return OK;
-    }
-    delete idle_socket.socket;
-  }
-
   // See if we already have enough connect jobs or sockets that will be released
   // soon.
   if (group.HasReleasingSockets()) {
diff --git a/net/socket/client_socket_pool_base.h b/net/socket/client_socket_pool_base.h
index 1a2a791..b740067 100644
--- a/net/socket/client_socket_pool_base.h
+++ b/net/socket/client_socket_pool_base.h
@@ -223,7 +223,7 @@ class ClientSocketPoolBaseHelper
     return connect_job_factory_->ConnectionTimeout();
   }
 
-  void enable_backup_jobs() { backup_jobs_enabled_ = true; };
+  void EnableBackupJobs() { backup_jobs_enabled_ = true; };
 
  private:
   friend class base::RefCounted<ClientSocketPoolBaseHelper>;
@@ -578,7 +578,7 @@ class ClientSocketPoolBase {
 
   const std::string& name() const { return name_; }
 
-  void enable_backup_jobs() { helper_->enable_backup_jobs(); };
+  void EnableBackupJobs() { helper_->EnableBackupJobs(); };
 
  private:
   // This adaptor class exists to bridge the
diff --git a/net/socket/client_socket_pool_base_unittest.cc b/net/socket/client_socket_pool_base_unittest.cc
index b0ddf55..bf5a30f 100644
--- a/net/socket/client_socket_pool_base_unittest.cc
+++ b/net/socket/client_socket_pool_base_unittest.cc
@@ -337,6 +337,8 @@ class TestClientSocketPool : public ClientSocketPool {
 
   void CleanupTimedOutIdleSockets() { base_.CleanupIdleSockets(false); }
 
+  void EnableBackupJobs() { base_.EnableBackupJobs(); }
+
  private:
   ~TestClientSocketPool() {}
 
@@ -841,6 +843,39 @@ TEST_F(ClientSocketPoolBaseTest, CloseIdleSocketAtSocketLimit) {
   EXPECT_EQ(OK, callback.WaitForResult());
 }
 
+// Regression test for http://crbug.com/40952.
+TEST_F(ClientSocketPoolBaseTest, CloseIdleSocketAtSocketLimitDeleteGroup) {
+  CreatePool(kDefaultMaxSockets, kDefaultMaxSocketsPerGroup);
+  pool_->EnableBackupJobs();
+  connect_job_factory_->set_job_type(TestConnectJob::kMockJob);
+
+  for (int i = 0; i < kDefaultMaxSockets; ++i) {
+    ClientSocketHandle handle;
+    TestCompletionCallback callback;
+    EXPECT_EQ(OK,
+              InitHandle(&handle, IntToString(i), kDefaultPriority, &callback,
+                         pool_, NULL));
+  }
+
+  // Flush all the DoReleaseSocket tasks.
+  MessageLoop::current()->RunAllPending();
+
+  // Stall a group.  Set a pending job so it'll trigger a backup job if we don't
+  // reuse a socket.
+  connect_job_factory_->set_job_type(TestConnectJob::kMockPendingJob);
+  ClientSocketHandle handle;
+  TestCompletionCallback callback;
+
+  // "0" is special here, since it should be the first entry in the sorted map,
+  // which is the one which we would close an idle socket for.  We shouldn't
+  // close an idle socket though, since we should reuse the idle socket.
+  EXPECT_EQ(OK,
+            InitHandle(&handle, "0", kDefaultPriority, &callback, pool_, NULL));
+
+  EXPECT_EQ(kDefaultMaxSockets, client_socket_factory_.allocation_count());
+  EXPECT_EQ(kDefaultMaxSockets - 1, pool_->IdleSocketCount());
+}
+
 TEST_F(ClientSocketPoolBaseTest, PendingRequests) {
   CreatePool(kDefaultMaxSockets, kDefaultMaxSocketsPerGroup);
 
diff --git a/net/socket/tcp_client_socket_pool.cc b/net/socket/tcp_client_socket_pool.cc
index db8a88a..32952af 100644
--- a/net/socket/tcp_client_socket_pool.cc
+++ b/net/socket/tcp_client_socket_pool.cc
@@ -181,7 +181,7 @@ TCPClientSocketPool::TCPClientSocketPool(
             base::TimeDelta::FromSeconds(kUnusedIdleSocketTimeout),
             base::TimeDelta::FromSeconds(kUsedIdleSocketTimeout),
             new TCPConnectJobFactory(client_socket_factory, host_resolver)) {
-  base_.enable_backup_jobs();
+  base_.EnableBackupJobs();
 }
 
 TCPClientSocketPool::~TCPClientSocketPool() {}