Makes waiting SSLConnectJobs use the message loops to resume their connection.

net/socket/ssl_client_socket_pool.cc

Index: net/socket/ssl_client_socket_pool.cc
diff --git a/net/socket/ssl_client_socket_pool.cc b/net/socket/ssl_client_socket_pool.cc
index c424767e82decd0a8efeec2df529b97c2761d7f9..ae1df2a399af94752533e1bab94ab8b4c16d9643 100644
--- a/net/socket/ssl_client_socket_pool.cc
+++ b/net/socket/ssl_client_socket_pool.cc
@@ -9,6 +9,7 @@
 #include "base/metrics/field_trial.h"
 #include "base/metrics/histogram.h"
 #include "base/metrics/sparse_histogram.h"
+#include "base/thread_task_runner_handle.h"
 #include "base/values.h"
 #include "net/base/host_port_pair.h"
 #include "net/base/net_errors.h"
@@ -94,6 +95,9 @@ SSLSocketParams::GetHttpProxyConnectionParams() const {
   return http_proxy_params_;
 }
 
+SSLConnectJobMessenger::SSLConnectJobMessenger() : weak_factory_(this) {
+}
+
 bool SSLConnectJobMessenger::CanProceed(SSLClientSocket* ssl_socket) {
   // If the session is in the session cache, or there are no connecting
   // sockets allow the connection to proceed.
@@ -107,10 +111,13 @@ void SSLConnectJobMessenger::MonitorConnectionResult(
     SSLClientSocket* ssl_socket) {
   connecting_sockets_.push_back(ssl_socket);
   ssl_socket->SetIsLeader();
-  ssl_socket->SetSocketFailureCallback(
-      base::Bind(&SSLConnectJobMessenger::OnJobFailed, base::Unretained(this)));
+  // SSLConnectJobMessenger weak_ptrs are used here to ensure that

[wtc, 2014/07/11 18:55:51]

weak_ptrs => "weak pointers" or "WeakPtrs"

[mshelley, 2014/07/14 20:30:06]

Done.

+  // tasks posted with these callbacks are deregistered if the
+  // SSLConnectJobMessenger should become invalid before they're run.
+  ssl_socket->SetSocketFailureCallback(base::Bind(
+      &SSLConnectJobMessenger::OnJobFailed, weak_factory_.GetWeakPtr()));
   ssl_socket->WatchSessionForCompletion(base::Bind(
-      &SSLConnectJobMessenger::OnJobSucceeded, base::Unretained(this)));
+      &SSLConnectJobMessenger::OnJobSucceeded, weak_factory_.GetWeakPtr()));
 }
 
 void SSLConnectJobMessenger::AddPendingSocket(SSLClientSocket* socket,
@@ -126,23 +133,51 @@ void SSLConnectJobMessenger::OnJobSucceeded() {
 }
 
 void SSLConnectJobMessenger::OnJobFailed() {
-  if (pending_sockets_and_callbacks_.empty())
-    return;
-  base::Closure callback = pending_sockets_and_callbacks_[0].callback;
+  base::Closure callback = base::Bind(&SSLConnectJobMessenger::ConnectNewLeader,
+                                      weak_factory_.GetWeakPtr());
+  base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE, callback);
+}
+
+void SSLConnectJobMessenger::ConnectNewLeader() {
   connecting_sockets_.erase(connecting_sockets_.begin());
-  SSLClientSocket* ssl_socket = pending_sockets_and_callbacks_[0].socket;
-  pending_sockets_and_callbacks_.erase(pending_sockets_and_callbacks_.begin());
+  std::vector<SocketAndCallback>::iterator it;
+  SSLClientSocket* ssl_socket;
+  base::Closure callback;
+
+  // Connect the first pending socket that has not been deleted.
+  for (it = pending_sockets_and_callbacks_.begin();
+       it != pending_sockets_and_callbacks_.end();
+       ++it) {
+    if (it->socket != NULL) {

[wtc, 2014/07/11 18:55:51]

IMPORTANT: this null check for |it->socket| doesn't work because |it->socket| is
not a weak pointer. |it->socket| comes from the messenger_->AddPendingSocket()
on line 438. It won't magically become null.

You'll need to write the code to remove a socket from
pending_sockets_and_callbacks_ before the socket is deleted.

[mshelley, 2014/07/14 20:30:06]

This has been corrected now in https://codereview.chromium.org/353713005/.

+      ssl_socket = it->socket;
+      callback = it->callback;
+      break;
+    }
+  }
+
+  // If there were no valid pending sockets, return.
+  if (it == pending_sockets_and_callbacks_.end())
+    return;
+
+  // Erase all deleted sockets and their callbacks, as well as the socket that
+  // will
+  // be connected next.

[Ryan Sleevi, 2014/07/11 01:25:59]

comment formating

[mshelley, 2014/07/14 20:30:06]

Done.

+  pending_sockets_and_callbacks_.erase(pending_sockets_and_callbacks_.begin(),
+                                       ++it);
+
   MonitorConnectionResult(ssl_socket);
   callback.Run();

[wtc, 2014/07/11 18:55:51]

IMPORTANT: I have to admit I don't understand why ConnectNewLeader should be run
as a task. If it is necessary to do so, it seems that only this very last line
(to run the resumption callback of the new leader) needs to be run as a task.

Contrast this with OnJobSucceeded. OnJobSucceeded does everything directly
except the last line (RunAllJobs), which posts the resumption callbacks of all
pending sockets as tasks. So it seems that OnJobFailed should also be able to do
everything directly except for the resumption callback of the new leader.

[mshelley, 2014/07/14 20:30:06]

The original concern was this: 

Say I have pending sockets a, b, and c. I post a task connecting a. By the time
the task runs, a has been deleted for some reason. This results in the task not
running, and sockets b and c never resuming their connections. So basically I
don't want there to be any delay between accessing a socket from
pending_sockets_and_callbacks_, and running that socket's callback.

With OnJobSucceeded this isn't a concern, because by definition there are not
pending sockets dependent upon the connection of the sockets being connected
with post task in OnJobSucceeded. 

 }
 
 void SSLConnectJobMessenger::RunAllJobs(
     std::vector<SocketAndCallback>& pending_sockets_and_callbacks) {
+  scoped_refptr<base::SingleThreadTaskRunner> task_runner =
+      base::ThreadTaskRunnerHandle::Get();
   for (std::vector<SocketAndCallback>::const_iterator it =
            pending_sockets_and_callbacks.begin();
        it != pending_sockets_and_callbacks.end();
        ++it)
-    it->callback.Run();
+    task_runner->PostTask(FROM_HERE, it->callback);

[wtc, 2014/07/11 18:55:51]

Please add curly braces because the loop constraint part of the for loop spans
multiple lines. This is a local style preference of the network stack code.

[mshelley, 2014/07/14 20:30:06]

Done.

 }
 
 // Timeout for the SSL handshake portion of the connect.