Fix a bug in backup socket cleanup where when we call RemoveConnectJob,

we didn't necessarily cleanup the pending backup socket task.

In addition to this, make the OnBackupSocketTimerFired more robust -
in the case that this ever should happen, it will simply fallout
rather than triggering a CRASH (and burn).

Test case included.

BUG=47375
TEST=ClientSocketPoolBaseTest.BackupSocketCancelAtMaxSockets

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

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

2 files changed, 56 insertions, 11 deletions

diff --git a/net/socket/client_socket_pool_base.cc b/net/socket/client_socket_pool_base.cc
index 40daa35..9bc663f 100644
--- a/net/socket/client_socket_pool_base.cc
+++ b/net/socket/client_socket_pool_base.cc
@@ -318,9 +318,15 @@ void ClientSocketPoolBaseHelper::OnBackupSocketTimerFired(
 
   CHECK(group.backup_job);
 
+  // If there are no more jobs pending, there is no work to do.
+  // If we've done our cleanups correctly, this should not happen.
+  if (group.jobs.empty()) {
+    NOTREACHED();
+    return;
+  }
+
   // If our backup job is waiting on DNS, or if we can't create any sockets
   // right now due to limits, just reset the timer.
-  CHECK(group.jobs.size());
   if (ReachedMaxSocketsLimit() ||
       !group.HasAvailableSocketSlot(max_sockets_per_group_) ||
       (*group.jobs.begin())->GetLoadState() == LOAD_STATE_RESOLVING_HOST) {
@@ -598,10 +604,6 @@ void ClientSocketPoolBaseHelper::OnConnectJobComplete(
   CHECK(group_it != group_map_.end());
   Group& group = group_it->second;
 
-  // We've had a connect on the socket; discard any pending backup job
-  // for this group and kill the pending task.
-  group.CleanupBackupJob();
-
   scoped_ptr<ClientSocket> socket(job->ReleaseSocket());
 
   BoundNetLog job_log = job->net_log();
@@ -646,18 +648,22 @@ void ClientSocketPoolBaseHelper::Flush() {
   CloseIdleSockets();
 }
 
-void ClientSocketPoolBaseHelper::RemoveConnectJob(const ConnectJob *job,
+void ClientSocketPoolBaseHelper::RemoveConnectJob(const ConnectJob* job,
                                                   Group* group) {
   CHECK_GT(connecting_socket_count_, 0);
   connecting_socket_count_--;
 
+  DCHECK(group);
+  DCHECK(ContainsKey(group->jobs, job));
+  group->jobs.erase(job);
+
+  // If we've got no more jobs for this group, then we no longer need a
+  // backup job either.
+  if (group->jobs.empty())
+    group->CleanupBackupJob();
+
   DCHECK(job);
   delete job;
-
-  if (group) {
-    DCHECK(ContainsKey(group->jobs, job));
-    group->jobs.erase(job);
-  }
 }
 
 void ClientSocketPoolBaseHelper::MaybeOnAvailableSocketSlot(
diff --git a/net/socket/client_socket_pool_base_unittest.cc b/net/socket/client_socket_pool_base_unittest.cc
index 7b84d94..1dcf897 100644
--- a/net/socket/client_socket_pool_base_unittest.cc
+++ b/net/socket/client_socket_pool_base_unittest.cc
@@ -1784,6 +1784,45 @@ TEST_F(ClientSocketPoolBaseTest, DoNotReuseSocketAfterFlush) {
   EXPECT_EQ(ClientSocketHandle::UNUSED, handle.reuse_type());
 }
 
+// Cancel a pending socket request while we're at max sockets,
+// and verify that the backup socket firing doesn't cause a crash.
+TEST_F(ClientSocketPoolBaseTest, BackupSocketCancelAtMaxSockets) {
+  // Max 4 sockets globally, max 4 sockets per group.
+  CreatePool(kDefaultMaxSockets, kDefaultMaxSockets);
+  pool_->EnableBackupJobs();
+
+  // Create the first socket and set to ERR_IO_PENDING.  This creates a
+  // backup job.
+  connect_job_factory_->set_job_type(TestConnectJob::kMockWaitingJob);
+  ClientSocketHandle handle;
+  TestCompletionCallback callback;
+  EXPECT_EQ(
+      ERR_IO_PENDING,
+      InitHandle(
+          &handle, "bar", kDefaultPriority, &callback, pool_, BoundNetLog()));
+
+  // Start (MaxSockets - 1) connected sockets to reach max sockets.
+  connect_job_factory_->set_job_type(TestConnectJob::kMockJob);
+  ClientSocketHandle handles[kDefaultMaxSockets];
+  for (int i = 1; i < kDefaultMaxSockets; ++i) {
+    TestCompletionCallback callback;
+    EXPECT_EQ(OK,
+              InitHandle(&handles[i], "bar", kDefaultPriority,
+                         &callback, pool_, BoundNetLog()));
+  }
+
+  MessageLoop::current()->RunAllPending();
+
+  // Cancel the pending request.
+  handle.Reset();
+
+  // Wait for the backup timer to fire (add some slop to ensure it fires)
+  PlatformThread::Sleep(ClientSocketPool::kMaxConnectRetryIntervalMs / 2 * 3);
+
+  MessageLoop::current()->RunAllPending();
+  EXPECT_EQ(kDefaultMaxSockets, client_socket_factory_.allocation_count());
+}
+
 }  // namespace
 
 }  // namespace net