Report the connect status of the oldest connection in the socket pool.

This CL implements a combination of the suggestions 1) and 2)
and comment #1 in crbug.com/462808. This reduces the number of
calls to GetLoadState. However the status might be less accurate
as we are far from guaranteed that the oldest connection is the
one which has come the furthest. This should be acceptable though,
as the state is mainly used report activity or at which stage
connections are hung at.

BUG=462808

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

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

3 files changed, 49 insertions, 40 deletions

diff --git a/net/socket/client_socket_pool_base.cc b/net/socket/client_socket_pool_base.cc
index f6e9671..3d0ff567 100644
--- a/net/socket/client_socket_pool_base.cc
+++ b/net/socket/client_socket_pool_base.cc
@@ -4,6 +4,8 @@
 
 #include "net/socket/client_socket_pool_base.h"
 
+#include <algorithm>
+
 #include "base/compiler_specific.h"
 #include "base/format_macros.h"
 #include "base/logging.h"
@@ -573,13 +575,8 @@ LoadState ClientSocketPoolBaseHelper::GetLoadState(
 
   const Group& group = *group_it->second;
   if (group.HasConnectJobForHandle(handle)) {
-    // Just return the state of the farthest along ConnectJob for the first
-    // group.jobs().size() pending requests.
-    LoadState max_state = LOAD_STATE_IDLE;
-    for (const auto& job : group.jobs()) {
-      max_state = std::max(max_state, job->GetLoadState());
-    }
-    return max_state;
+    // Just return the state of the oldest ConnectJob.
+    return (*group.jobs().begin())->GetLoadState();
   }
 
   if (group.CanUseAdditionalSocketSlot(max_sockets_per_group_))
@@ -629,7 +626,7 @@ base::DictionaryValue* ClientSocketPoolBaseHelper::GetInfoAsValue(
     group_dict->Set("idle_sockets", idle_socket_list);
 
     base::ListValue* connect_jobs_list = new base::ListValue();
-    std::set<ConnectJob*>::const_iterator job = group->jobs().begin();
+    std::list<ConnectJob*>::const_iterator job = group->jobs().begin();
     for (job = group->jobs().begin(); job != group->jobs().end(); job++) {
       int source_id = (*job)->net_log().source().id;
       connect_jobs_list->Append(new base::FundamentalValue(source_id));
@@ -1197,19 +1194,16 @@ void ClientSocketPoolBaseHelper::Group::AddJob(scoped_ptr<ConnectJob> job,
 
   if (is_preconnect)
     ++unassigned_job_count_;
-  jobs_.insert(job.release());
+  jobs_.push_back(job.release());
 }
 
 void ClientSocketPoolBaseHelper::Group::RemoveJob(ConnectJob* job) {
   scoped_ptr<ConnectJob> owned_job(job);
   SanityCheck();
 
-  std::set<ConnectJob*>::iterator it = jobs_.find(job);
-  if (it != jobs_.end()) {
-    jobs_.erase(it);
-  } else {
-    NOTREACHED();
-  }
+  // Check that |job| is in the list.
+  DCHECK_EQ(*std::find(jobs_.begin(), jobs_.end(), job), job);
+  jobs_.remove(job);
   size_t job_count = jobs_.size();
   if (job_count < unassigned_job_count_)
     unassigned_job_count_ = job_count;
diff --git a/net/socket/client_socket_pool_base.h b/net/socket/client_socket_pool_base.h
index 4892780..2e3c05e 100644
--- a/net/socket/client_socket_pool_base.h
+++ b/net/socket/client_socket_pool_base.h
@@ -450,7 +450,7 @@ class NET_EXPORT_PRIVATE ClientSocketPoolBaseHelper
     void DecrementActiveSocketCount() { active_socket_count_--; }
 
     int unassigned_job_count() const { return unassigned_job_count_; }
-    const std::set<ConnectJob*>& jobs() const { return jobs_; }
+    const std::list<ConnectJob*>& jobs() const { return jobs_; }
     const std::list<IdleSocket>& idle_sockets() const { return idle_sockets_; }
     int active_socket_count() const { return active_socket_count_; }
     std::list<IdleSocket>* mutable_idle_sockets() { return &idle_sockets_; }
@@ -479,7 +479,7 @@ class NET_EXPORT_PRIVATE ClientSocketPoolBaseHelper
     size_t unassigned_job_count_;
 
     std::list<IdleSocket> idle_sockets_;
-    std::set<ConnectJob*> jobs_;
+    std::list<ConnectJob*> jobs_;
     RequestQueue pending_requests_;
     int active_socket_count_;  // number of active sockets used by clients
     // A timer for when to start the backup job.
diff --git a/net/socket/client_socket_pool_base_unittest.cc b/net/socket/client_socket_pool_base_unittest.cc
index e54259d..2de67bd 100644
--- a/net/socket/client_socket_pool_base_unittest.cc
+++ b/net/socket/client_socket_pool_base_unittest.cc
@@ -1955,48 +1955,63 @@ TEST_F(ClientSocketPoolBaseTest, LoadStateOneRequest) {
 }
 
 // Test GetLoadState in the case there are two socket requests.
+// Only the first connection in the pool should affect the pool's load status.
 TEST_F(ClientSocketPoolBaseTest, LoadStateTwoRequests) {
   CreatePool(2, 2);
   connect_job_factory_->set_job_type(TestConnectJob::kMockWaitingJob);
 
   ClientSocketHandle handle;
   TestCompletionCallback callback;
-  int rv = handle.Init("a",
-                       params_,
-                       DEFAULT_PRIORITY,
-                       callback.callback(),
-                       pool_.get(),
-                       BoundNetLog());
+  int rv = handle.Init("a", params_, DEFAULT_PRIORITY, callback.callback(),
+                       pool_.get(), BoundNetLog());
   EXPECT_EQ(ERR_IO_PENDING, rv);
+  client_socket_factory_.SetJobLoadState(0, LOAD_STATE_RESOLVING_HOST);
 
   ClientSocketHandle handle2;
   TestCompletionCallback callback2;
-  rv = handle2.Init("a",
-                    params_,
-                    DEFAULT_PRIORITY,
-                    callback2.callback(),
-                    pool_.get(),
-                    BoundNetLog());
+  rv = handle2.Init("a", params_, DEFAULT_PRIORITY, callback2.callback(),
+                    pool_.get(), BoundNetLog());
   EXPECT_EQ(ERR_IO_PENDING, rv);
+  client_socket_factory_.SetJobLoadState(1, LOAD_STATE_RESOLVING_HOST);
 
-  // If the first Job is in an earlier state than the second, the state of
-  // the second job should be used for both handles.
-  client_socket_factory_.SetJobLoadState(0, LOAD_STATE_RESOLVING_HOST);
+  // Check that both handles report the state of the first job.
+  EXPECT_EQ(LOAD_STATE_RESOLVING_HOST, handle.GetLoadState());
+  EXPECT_EQ(LOAD_STATE_RESOLVING_HOST, handle2.GetLoadState());
+
+  client_socket_factory_.SetJobLoadState(0, LOAD_STATE_CONNECTING);
+
+  // Check that both handles change to LOAD_STATE_CONNECTING.
   EXPECT_EQ(LOAD_STATE_CONNECTING, handle.GetLoadState());
   EXPECT_EQ(LOAD_STATE_CONNECTING, handle2.GetLoadState());
+}
 
-  // If the second Job is in an earlier state than the second, the state of
-  // the first job should be used for both handles.
-  client_socket_factory_.SetJobLoadState(0, LOAD_STATE_SSL_HANDSHAKE);
-  // One request is farther
-  EXPECT_EQ(LOAD_STATE_SSL_HANDSHAKE, handle.GetLoadState());
-  EXPECT_EQ(LOAD_STATE_SSL_HANDSHAKE, handle2.GetLoadState());
+// Test that the second connection request does not affect the pool's load
+// status.
+TEST_F(ClientSocketPoolBaseTest, LoadStateTwoRequestsChangeSecondRequestState) {
+  CreatePool(2, 2);
+  connect_job_factory_->set_job_type(TestConnectJob::kMockWaitingJob);
+
+  ClientSocketHandle handle;
+  TestCompletionCallback callback;
+  int rv = handle.Init("a", params_, DEFAULT_PRIORITY, callback.callback(),
+                       pool_.get(), BoundNetLog());
+  EXPECT_EQ(ERR_IO_PENDING, rv);
 
-  // Farthest along job connects and the first request gets the socket.  The
+  ClientSocketHandle handle2;
+  TestCompletionCallback callback2;
+  rv = handle2.Init("a", params_, DEFAULT_PRIORITY, callback2.callback(),
+                    pool_.get(), BoundNetLog());
+  EXPECT_EQ(ERR_IO_PENDING, rv);
+  client_socket_factory_.SetJobLoadState(1, LOAD_STATE_RESOLVING_HOST);
+
+  EXPECT_EQ(LOAD_STATE_CONNECTING, handle.GetLoadState());
+  EXPECT_EQ(LOAD_STATE_CONNECTING, handle2.GetLoadState());
+
+  // First job connects and the first request gets the socket.  The
   // second handle switches to the state of the remaining ConnectJob.
   client_socket_factory_.SignalJob(0);
   EXPECT_EQ(OK, callback.WaitForResult());
-  EXPECT_EQ(LOAD_STATE_CONNECTING, handle2.GetLoadState());
+  EXPECT_EQ(LOAD_STATE_RESOLVING_HOST, handle2.GetLoadState());
 }
 
 // Test GetLoadState in the case the per-group limit is reached.