Fix preconnect crash when we hit max socket limit.

When we hit the max socket limit, we close an idle socket in order to make space for the new preconnecting socket.  It's possible for the selected socket to belong to the same connection group as the one we're preconnecting a socket for.  This is obviously broken.  The bug currently results in us potentially deleting the ClientSocketPoolHelper::Group associated with that connection group, so the |Group* group| local variable in RequestSocketInternal() is now invalid.  Any access to that variable later on in the function results in badness.

This was safe before because we would never try to close an idle socket in the connection group we're request a socket for, because the first condition in RequestSocketInternal() checks to see if we can reuse an idle socket.  In the preconnect case, since we want to warm up the number of sockets in the connection group, we bypass idle sockets.

The solution is to create a new function: CloseOneIdleSocketExceptInGroup(const Group*).  This way we avoid this problem.  I provide a return value so that the caller can tell whether or not an idle socket was closed.  If it was not closed (it's possible that the connection group we're requesting a socket for is the only one with idle sockets), then the caller can tell, so it can pass up the failure so RequestSockets() knows that we've hit the max socket limit and there's no point in trying to preconnect more sockets.

BUG=64940
TEST=New unittest.

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

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

4 files changed, 100 insertions, 4 deletions

diff --git a/net/base/net_error_list.h b/net/base/net_error_list.h
index 2998a46..636808c 100644
--- a/net/base/net_error_list.h
+++ b/net/base/net_error_list.h
@@ -191,7 +191,9 @@ NET_ERROR(SSL_SNAP_START_NPN_MISPREDICTION, -131)
 // give the user a helpful error message rather than have the connection hang.
 NET_ERROR(ESET_ANTI_VIRUS_SSL_INTERCEPTION, -132)
 
-// Missing -133. Feel free to reuse in the future.
+// We've hit the max socket limit for the socket pool while preconnecting.  We
+// don't bother trying to preconnect more sockets.
+NET_ERROR(PRECONNECT_MAX_SOCKET_LIMIT, -133)
 
 // The permission to use the SSL client certificate's private key was denied.
 NET_ERROR(SSL_CLIENT_AUTH_PRIVATE_KEY_ACCESS_DENIED, -134)
diff --git a/net/socket/client_socket_pool_base.cc b/net/socket/client_socket_pool_base.cc
index 4f042bb..86ba2dd 100644
--- a/net/socket/client_socket_pool_base.cc
+++ b/net/socket/client_socket_pool_base.cc
@@ -250,6 +250,8 @@ void ClientSocketPoolBaseHelper::RequestSockets(
        group->NumActiveSocketSlots() < num_sockets &&
        num_iterations_left > 0 ; num_iterations_left--) {
     int rv = RequestSocketInternal(group_name, &request);
+    // TODO(willchan): Possibly check for ERR_PRECONNECT_MAX_SOCKET_LIMIT so we
+    // can log it into the NetLog.
     if (rv < 0 && rv != ERR_IO_PENDING) {
       // We're encountering a synchronous error.  Give up.
       if (!ContainsKey(group_map_, group_name))
@@ -298,7 +300,9 @@ int ClientSocketPoolBaseHelper::RequestSocketInternal(
 
   if (ReachedMaxSocketsLimit()) {
     if (idle_socket_count() > 0) {
-      CloseOneIdleSocket();
+      bool closed = CloseOneIdleSocketExceptInGroup(group);
+      if (preconnecting && !closed)
+        return ERR_PRECONNECT_MAX_SOCKET_LIMIT;
     } else {
       // We could check if we really have a stalled group here, but it requires
       // a scan of all groups, so just flip a flag here, and do the check later.
@@ -940,10 +944,17 @@ bool ClientSocketPoolBaseHelper::ReachedMaxSocketsLimit() const {
 }
 
 void ClientSocketPoolBaseHelper::CloseOneIdleSocket() {
+  CloseOneIdleSocketExceptInGroup(NULL);
+}
+
+bool ClientSocketPoolBaseHelper::CloseOneIdleSocketExceptInGroup(
+    const Group* exception_group) {
   CHECK_GT(idle_socket_count(), 0);
 
   for (GroupMap::iterator i = group_map_.begin(); i != group_map_.end(); ++i) {
     Group* group = i->second;
+    if (exception_group == group)
+      continue;
     std::list<IdleSocket>* idle_sockets = group->mutable_idle_sockets();
 
     if (!idle_sockets->empty()) {
@@ -953,11 +964,14 @@ void ClientSocketPoolBaseHelper::CloseOneIdleSocket() {
       if (group->IsEmpty())
         RemoveGroup(i);
 
-      return;
+      return true;
     }
   }
 
-  LOG(DFATAL) << "No idle socket found to close!.";
+  if (!exception_group)
+    LOG(DFATAL) << "No idle socket found to close!.";
+
+  return false;
 }
 
 void ClientSocketPoolBaseHelper::InvokeUserCallbackLater(
diff --git a/net/socket/client_socket_pool_base.h b/net/socket/client_socket_pool_base.h
index 2e8c618..8e6eb13 100644
--- a/net/socket/client_socket_pool_base.h
+++ b/net/socket/client_socket_pool_base.h
@@ -480,6 +480,11 @@ class ClientSocketPoolBaseHelper
   // I'm not sure if we hit this situation often.
   void CloseOneIdleSocket();
 
+  // Same as CloseOneIdleSocket() except it won't close an idle socket in
+  // |group|.  If |group| is NULL, it is ignored.  Returns true if it closed a
+  // socket.
+  bool CloseOneIdleSocketExceptInGroup(const Group* group);
+
   // Checks if there are stalled socket groups that should be notified
   // for possible wakeup.
   void CheckForStalledSocketGroups();
diff --git a/net/socket/client_socket_pool_base_unittest.cc b/net/socket/client_socket_pool_base_unittest.cc
index fef0a5e..843b6be 100644
--- a/net/socket/client_socket_pool_base_unittest.cc
+++ b/net/socket/client_socket_pool_base_unittest.cc
@@ -3025,6 +3025,81 @@ TEST_F(ClientSocketPoolBaseTest, PreconnectJobsTakenByNormalRequests) {
   EXPECT_EQ(1, pool_->IdleSocketCountInGroup("a"));
 }
 
+// http://crbug.com/64940 regression test.
+TEST_F(ClientSocketPoolBaseTest, PreconnectClosesIdleSocketRemovesGroup) {
+  const int kMaxTotalSockets = 3;
+  const int kMaxSocketsPerGroup = 2;
+  CreatePool(kMaxTotalSockets, kMaxSocketsPerGroup);
+  connect_job_factory_->set_job_type(TestConnectJob::kMockPendingJob);
+
+  // Note that group name ordering matters here.  "a" comes before "b", so
+  // CloseOneIdleSocket() will try to close "a"'s idle socket.
+
+  // Set up one idle socket in "a".
+  ClientSocketHandle handle1;
+  TestCompletionCallback callback1;
+  EXPECT_EQ(ERR_IO_PENDING, handle1.Init("a",
+                                         params_,
+                                         kDefaultPriority,
+                                         &callback1,
+                                         pool_.get(),
+                                         BoundNetLog()));
+
+  ASSERT_EQ(OK, callback1.WaitForResult());
+  handle1.Reset();
+  EXPECT_EQ(1, pool_->IdleSocketCountInGroup("a"));
+
+  // Set up two active sockets in "b".
+  ClientSocketHandle handle2;
+  TestCompletionCallback callback2;
+  EXPECT_EQ(ERR_IO_PENDING, handle1.Init("b",
+                                         params_,
+                                         kDefaultPriority,
+                                         &callback1,
+                                         pool_.get(),
+                                         BoundNetLog()));
+  EXPECT_EQ(ERR_IO_PENDING, handle2.Init("b",
+                                         params_,
+                                         kDefaultPriority,
+                                         &callback2,
+                                         pool_.get(),
+                                         BoundNetLog()));
+
+  ASSERT_EQ(OK, callback1.WaitForResult());
+  ASSERT_EQ(OK, callback2.WaitForResult());
+  EXPECT_EQ(0, pool_->IdleSocketCountInGroup("b"));
+  EXPECT_EQ(2, pool_->NumActiveSocketsInGroup("b"));
+
+  // Now we have 1 idle socket in "a" and 2 active sockets in "b".  This means
+  // we've maxed out on sockets, since we set |kMaxTotalSockets| to 3.
+  // Requesting 2 preconnected sockets for "a" should fail to allocate any more
+  // sockets for "a", and "b" should still have 2 active sockets.
+
+  pool_->RequestSockets("a", &params_, 2, BoundNetLog());
+  EXPECT_EQ(0, pool_->NumConnectJobsInGroup("a"));
+  EXPECT_EQ(1, pool_->IdleSocketCountInGroup("a"));
+  EXPECT_EQ(0, pool_->NumActiveSocketsInGroup("a"));
+  EXPECT_EQ(0, pool_->NumConnectJobsInGroup("b"));
+  EXPECT_EQ(0, pool_->IdleSocketCountInGroup("b"));
+  EXPECT_EQ(2, pool_->NumActiveSocketsInGroup("b"));
+
+  // Now release the 2 active sockets for "b".  This will give us 1 idle socket
+  // in "a" and 2 idle sockets in "b".  Requesting 2 preconnected sockets for
+  // "a" should result in closing 1 for "b".
+  handle1.Reset();
+  handle2.Reset();
+  EXPECT_EQ(2, pool_->IdleSocketCountInGroup("b"));
+  EXPECT_EQ(0, pool_->NumActiveSocketsInGroup("b"));
+
+  pool_->RequestSockets("a", &params_, 2, BoundNetLog());
+  EXPECT_EQ(1, pool_->NumConnectJobsInGroup("a"));
+  EXPECT_EQ(1, pool_->IdleSocketCountInGroup("a"));
+  EXPECT_EQ(0, pool_->NumActiveSocketsInGroup("a"));
+  EXPECT_EQ(0, pool_->NumConnectJobsInGroup("b"));
+  EXPECT_EQ(1, pool_->IdleSocketCountInGroup("b"));
+  EXPECT_EQ(0, pool_->NumActiveSocketsInGroup("b"));
+}
+
 }  // namespace
 
 }  // namespace net