Switch on socket late binding - Take 2.

Re-enable socket late binding.  The mac valgrind errors happened due to threading bugs in test_shell_tests.  The ui thread would TearDown() the test object, which deleted the TestURLRequestContext, which eventually deletes the TCPClientSocketPool, which deletes its ConnectJobs.  However, those ConnectJobs might be running simultaneously on the io thread.  Therefore, we have a race condition.  This change fixes that.

Histograms for the 4.0.266.0 dev channel release indicate the following changes for late binding:
(a) Net.TCPSocketType shows a decrease (from 41.85% to 39.29%) in used of newly connected sockets. Part of this decrease is due to using previously used sockets more often (increase from 58.15% to 58.53%), but is primarily due to being able to use sockets that were connected, but not immediately handed over to a socket request (increased from 0 [not supported without late binding] to 2.18%).
(b) Net.SocketIdleTimeBeforeNextUse_ReusedSocket indicates that reused sockets are getting used more quickly than before, with a decrease of mean idle time from 11.65 seconds to 11.34 seconds.
(c) Net.Transaction_Connected_Under_10 indicates shows that the mean for time until the first byte of the transaction response decreased from 1585ms to 1481ms.
The code change deletes the old non socket late binding code paths, cleaning up the code significantly. It also deletes duplicated tests in ClientSocketPoolBase which covered both pathways. A TCPClientSocketPool test had to be updated as well.
BUG=http://crbug.com/30354.

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

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

1 files changed, 37 insertions, 128 deletions

diff --git a/net/socket/client_socket_pool_base.cc b/net/socket/client_socket_pool_base.cc
index 037c9ff..9b14f4b 100644
--- a/net/socket/client_socket_pool_base.cc
+++ b/net/socket/client_socket_pool_base.cc
@@ -32,17 +32,14 @@ const int kMaxNumLoadLogEntries = 50;
 namespace net {
 
 ConnectJob::ConnectJob(const std::string& group_name,
-                       const ClientSocketHandle* key_handle,
                        base::TimeDelta timeout_duration,
                        Delegate* delegate,
                        LoadLog* load_log)
     : group_name_(group_name),
-      key_handle_(key_handle),
       timeout_duration_(timeout_duration),
       delegate_(delegate),
       load_log_(load_log) {
   DCHECK(!group_name.empty());
-  DCHECK(key_handle);
   DCHECK(delegate);
 }
 
@@ -93,8 +90,6 @@ void ConnectJob::OnTimeout() {
 
 namespace internal {
 
-bool ClientSocketPoolBaseHelper::g_late_binding = false;
-
 ClientSocketPoolBaseHelper::ClientSocketPoolBaseHelper(
     int max_sockets,
     int max_sockets_per_group,
@@ -120,14 +115,14 @@ ClientSocketPoolBaseHelper::ClientSocketPoolBaseHelper(
 }
 
 ClientSocketPoolBaseHelper::~ClientSocketPoolBaseHelper() {
-  if (g_late_binding)
-    CancelAllConnectJobs();
+  CancelAllConnectJobs();
+
   // Clean up any idle sockets.  Assert that we have no remaining active
   // sockets or pending requests.  They should have all been cleaned up prior
   // to the manager being destroyed.
   CloseIdleSockets();
   DCHECK(group_map_.empty());
-  DCHECK(connect_job_map_.empty());
+  DCHECK_EQ(0, connecting_socket_count_);
 
   if (network_change_notifier_)
     network_change_notifier_->RemoveObserver(this);
@@ -156,7 +151,7 @@ ClientSocketPoolBaseHelper::RemoveRequestFromQueue(
   const Request* req = *it;
 
   LoadLog::EndEvent(req->load_log(),
-      LoadLog::TYPE_SOCKET_POOL_WAITING_IN_QUEUE);
+                    LoadLog::TYPE_SOCKET_POOL_WAITING_IN_QUEUE);
 
   pending_requests->erase(it);
   return req;
@@ -208,17 +203,13 @@ int ClientSocketPoolBaseHelper::RequestSocket(
 
   // See if we already have enough connect jobs or sockets that will be released
   // soon.
-  if (g_late_binding && group.HasReleasingSockets()) {
+  if (group.HasReleasingSockets()) {
     InsertRequestIntoQueue(request, &group.pending_requests);
     return ERR_IO_PENDING;
   }
 
   // We couldn't find a socket to reuse, so allocate and connect a new one.
-
-  // If we aren't using late binding, the job lines up with a request so
-  // just write directly into the request's LoadLog.
-  scoped_refptr<LoadLog> job_load_log = g_late_binding ?
-      new LoadLog(kMaxNumLoadLogEntries) : request->load_log();
+  scoped_refptr<LoadLog> job_load_log = new LoadLog(kMaxNumLoadLogEntries);
 
   scoped_ptr<ConnectJob> connect_job(
       connect_job_factory_->NewConnectJob(group_name, *request, this,
@@ -226,7 +217,7 @@ int ClientSocketPoolBaseHelper::RequestSocket(
 
   int rv = connect_job->Connect();
 
-  if (g_late_binding && rv != ERR_IO_PENDING && request->load_log())
+  if (rv != ERR_IO_PENDING && request->load_log())
     request->load_log()->Append(job_load_log);
 
   if (rv == OK) {
@@ -236,14 +227,7 @@ int ClientSocketPoolBaseHelper::RequestSocket(
     connecting_socket_count_++;
 
     ConnectJob* job = connect_job.release();
-    if (g_late_binding) {
-      CHECK(!ContainsKey(connect_job_map_, handle));
-      InsertRequestIntoQueue(request, &group.pending_requests);
-    } else {
-      group.connecting_requests[handle] = request;
-      CHECK(!ContainsKey(connect_job_map_, handle));
-      connect_job_map_[handle] = job;
-    }
+    InsertRequestIntoQueue(request, &group.pending_requests);
     group.jobs.insert(job);
   } else if (group.IsEmpty()) {
     group_map_.erase(group_name);
@@ -266,29 +250,14 @@ void ClientSocketPoolBaseHelper::CancelRequest(
       LoadLog::AddEvent(req->load_log(), LoadLog::TYPE_CANCELLED);
       LoadLog::EndEvent(req->load_log(), LoadLog::TYPE_SOCKET_POOL);
       delete req;
-      if (g_late_binding &&
-          group.jobs.size() > group.pending_requests.size() + 1) {
+      if (group.jobs.size() > group.pending_requests.size() + 1) {
         // TODO(willchan): Cancel the job in the earliest LoadState.
-        RemoveConnectJob(handle, *group.jobs.begin(), &group);
+        RemoveConnectJob(*group.jobs.begin(), &group);
         OnAvailableSocketSlot(group_name, &group);
       }
       return;
     }
   }
-
-  if (!g_late_binding) {
-    // It's invalid to cancel a non-existent request.
-    CHECK(ContainsKey(group.connecting_requests, handle));
-
-    RequestMap::iterator map_it = group.connecting_requests.find(handle);
-    if (map_it != group.connecting_requests.end()) {
-      scoped_refptr<LoadLog> log(map_it->second->load_log());
-      LoadLog::AddEvent(log, LoadLog::TYPE_CANCELLED);
-      LoadLog::EndEvent(log, LoadLog::TYPE_SOCKET_POOL);
-      RemoveConnectJob(handle, NULL, &group);
-      OnAvailableSocketSlot(group_name, &group);
-    }
-  }
 }
 
 void ClientSocketPoolBaseHelper::ReleaseSocket(const std::string& group_name,
@@ -327,22 +296,11 @@ LoadState ClientSocketPoolBaseHelper::GetLoadState(
   // Can't use operator[] since it is non-const.
   const Group& group = group_map_.find(group_name)->second;
 
-  // Search connecting_requests for matching handle.
-  RequestMap::const_iterator map_it = group.connecting_requests.find(handle);
-  if (map_it != group.connecting_requests.end()) {
-    ConnectJobMap::const_iterator job_it = connect_job_map_.find(handle);
-    if (job_it == connect_job_map_.end()) {
-      NOTREACHED();
-      return LOAD_STATE_IDLE;
-    }
-    return job_it->second->GetLoadState();
-  }
-
   // Search pending_requests for matching handle.
   RequestQueue::const_iterator it = group.pending_requests.begin();
   for (size_t i = 0; it != group.pending_requests.end(); ++it, ++i) {
     if ((*it)->handle() == handle) {
-      if (g_late_binding && i < group.jobs.size()) {
+      if (i < group.jobs.size()) {
         LoadState max_state = LOAD_STATE_IDLE;
         for (ConnectJobSet::const_iterator job_it = group.jobs.begin();
              job_it != group.jobs.end(); ++job_it) {
@@ -480,67 +438,38 @@ void ClientSocketPoolBaseHelper::OnConnectJobComplete(
   CHECK(group_it != group_map_.end());
   Group& group = group_it->second;
 
-  const ClientSocketHandle* const key_handle = job->key_handle();
   scoped_ptr<ClientSocket> socket(job->ReleaseSocket());
 
-  if (g_late_binding) {
-    scoped_refptr<LoadLog> job_load_log(job->load_log());
-    RemoveConnectJob(key_handle, job, &group);
+  scoped_refptr<LoadLog> job_load_log(job->load_log());
+  RemoveConnectJob(job, &group);
+
+  LoadLog::EndEvent(job_load_log, LoadLog::TYPE_SOCKET_POOL);
 
-    scoped_ptr<const Request> r;
+  if (result == OK) {
+    DCHECK(socket.get());
     if (!group.pending_requests.empty()) {
-      r.reset(RemoveRequestFromQueue(
+      scoped_ptr<const Request> r(RemoveRequestFromQueue(
           group.pending_requests.begin(), &group.pending_requests));
-
       if (r->load_log())
         r->load_log()->Append(job_load_log);
-
-      LoadLog::EndEvent(r->load_log(), LoadLog::TYPE_SOCKET_POOL);
-    }
-
-    if (result == OK) {
-      DCHECK(socket.get());
-      if (r.get()) {
-        HandOutSocket(
-            socket.release(), false /* unused socket */, r->handle(),
-            base::TimeDelta(), &group);
-        r->callback()->Run(result);
-      } else {
-        AddIdleSocket(socket.release(), false /* unused socket */, &group);
-        OnAvailableSocketSlot(group_name, &group);
-      }
+      HandOutSocket(
+          socket.release(), false /* unused socket */, r->handle(),
+          base::TimeDelta(), &group);
+      r->callback()->Run(result);
     } else {
-      DCHECK(!socket.get());
-      if (r.get())
-        r->callback()->Run(result);
-      MaybeOnAvailableSocketSlot(group_name);
+      AddIdleSocket(socket.release(), false /* unused socket */, &group);
+      OnAvailableSocketSlot(group_name, &group);
     }
-
-    return;
-  }
-
-  RequestMap* request_map = &group.connecting_requests;
-  RequestMap::iterator it = request_map->find(key_handle);
-  CHECK(it != request_map->end());
-  const Request* request = it->second;
-  ClientSocketHandle* const handle = request->handle();
-  CompletionCallback* const callback = request->callback();
-
-  LoadLog::EndEvent(request->load_log(), LoadLog::TYPE_SOCKET_POOL);
-
-  RemoveConnectJob(key_handle, job, &group);
-
-  if (result != OK) {
+  } else {
     DCHECK(!socket.get());
-    callback->Run(result);  // |group| is not necessarily valid after this.
-    // |group| may be invalid after the callback, we need to search
-    // |group_map_| again.
+    if (!group.pending_requests.empty()) {
+      scoped_ptr<const Request> r(RemoveRequestFromQueue(
+          group.pending_requests.begin(), &group.pending_requests));
+      if (r->load_log())
+        r->load_log()->Append(job_load_log);
+      r->callback()->Run(result);
+    }
     MaybeOnAvailableSocketSlot(group_name);
-  } else {
-    DCHECK(socket.get());
-    HandOutSocket(socket.release(), false /* not reused */, handle,
-                  base::TimeDelta(), &group);
-    callback->Run(result);
   }
 }
 
@@ -548,30 +477,13 @@ void ClientSocketPoolBaseHelper::OnIPAddressChanged() {
   CloseIdleSockets();
 }
 
-void ClientSocketPoolBaseHelper::EnableLateBindingOfSockets(bool enabled) {
-  g_late_binding = enabled;
-}
-
-void ClientSocketPoolBaseHelper::RemoveConnectJob(
-    const ClientSocketHandle* handle, const ConnectJob *job, Group* group) {
+void ClientSocketPoolBaseHelper::RemoveConnectJob(const ConnectJob *job,
+                                                  Group* group) {
   CHECK(connecting_socket_count_ > 0);
   connecting_socket_count_--;
 
-  if (g_late_binding) {
-    DCHECK(job);
-    delete job;
-  } else {
-    ConnectJobMap::iterator it = connect_job_map_.find(handle);
-    CHECK(it != connect_job_map_.end());
-    job = it->second;
-    delete job;
-    connect_job_map_.erase(it);
-    RequestMap::iterator map_it = group->connecting_requests.find(handle);
-    CHECK(map_it != group->connecting_requests.end());
-    const Request* request = map_it->second;
-    delete request;
-    group->connecting_requests.erase(map_it);
-  }
+  DCHECK(job);
+  delete job;
 
   if (group) {
     DCHECK(ContainsKey(group->jobs, job));
@@ -659,6 +571,7 @@ void ClientSocketPoolBaseHelper::AddIdleSocket(
 void ClientSocketPoolBaseHelper::CancelAllConnectJobs() {
   for (GroupMap::iterator i = group_map_.begin(); i != group_map_.end();) {
     Group& group = i->second;
+    connecting_socket_count_ -= group.jobs.size();
     STLDeleteElements(&group.jobs);
 
     // Delete group if no longer needed.
@@ -679,8 +592,4 @@ bool ClientSocketPoolBaseHelper::ReachedMaxSocketsLimit() const {
 
 }  // namespace internal
 
-void EnableLateBindingOfSockets(bool enabled) {
-  internal::ClientSocketPoolBaseHelper::EnableLateBindingOfSockets(enabled);
-}
-
 }  // namespace net