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 e3530ef1bc4c9529434b4b15975b97c7a633b8dd..e6f3bd1682b09f07340cefdca49b8e4df5cf5bba 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"
@@ -115,6 +116,9 @@ void SSLConnectJobMessenger::RemoveSocket(SSLClientSocket* ssl_socket) {
   }
 }
 
+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.
@@ -124,11 +128,14 @@ bool SSLConnectJobMessenger::CanProceed(SSLClientSocket* ssl_socket) {
 void SSLConnectJobMessenger::MonitorConnectionResult(
     SSLClientSocket* ssl_socket) {
   connecting_sockets_.push_back(ssl_socket);
-  // TODO(mshelley): Both of these callbacks will use weak_ptr in future CL.
-  ssl_socket->SetHandshakeFailureCallback(
-      base::Bind(&SSLConnectJobMessenger::OnJobFailed, base::Unretained(this)));
+
+  // SSLConnectJobMessenger weak pointers are used here to ensure that
+  // tasks posted with these callbacks are deregistered if the
+  // SSLConnectJobMessenger should become invalid before they're run.

[Ryan Sleevi, 2014/07/18 23:08:35]

Probably unnecessary; this is generally understood as the point of a WeakPtr.

[mshelley, 2014/07/21 23:08:28]

Done.

+  ssl_socket->SetHandshakeFailureCallback(base::Bind(
+      &SSLConnectJobMessenger::OnJobFailed, weak_factory_.GetWeakPtr()));
   ssl_socket->SetHandshakeSuccessCallback(base::Bind(
-      &SSLConnectJobMessenger::OnJobSucceeded, base::Unretained(this)));
+      &SSLConnectJobMessenger::OnJobSucceeded, weak_factory_.GetWeakPtr()));
 }
 
 void SSLConnectJobMessenger::AddPendingSocket(SSLClientSocket* socket,
@@ -144,23 +151,36 @@ void SSLConnectJobMessenger::OnJobSucceeded() {
 }
 
 void SSLConnectJobMessenger::OnJobFailed() {
+  connecting_sockets_.erase(connecting_sockets_.begin());
+
+  // If there were no valid pending sockets, return.
   if (pending_sockets_and_callbacks_.empty())
     return;
-  base::Closure callback = pending_sockets_and_callbacks_[0].callback;
-  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 =
+      pending_sockets_and_callbacks_.begin();
+
+  SSLClientSocket* ssl_socket = it->socket;
+  base::Closure& callback = it->callback;

[Ryan Sleevi, 2014/07/18 23:08:35]

SECURITY BUG: You're taking a reference to it-> here, and when you run line 165,
it's a use after free bug.

Per the other review, you could look at just

SocketAndCallback socket_and_callback = pending_sockets_and_callbacks_.front();
pending_sockets_and_callbacks_.erase(....begin());

[mshelley, 2014/07/21 23:08:29]

Done.

+
+  pending_sockets_and_callbacks_.erase(it);
+
   MonitorConnectionResult(ssl_socket);
-  callback.Run();
+
+  // PostTask is used to avoid the recursive call sequence caused
+  // by SSLConnectJobMessengers running the callback directly.
+  base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE, callback);
 }
 
 void SSLConnectJobMessenger::RunAllCallbacks(
     const SSLPendingSocketsAndCallbacks& pending_sockets_and_callbacks) {
+  scoped_refptr<base::TaskRunner> task_runner =

[Ryan Sleevi, 2014/07/18 23:08:35]

Here I think you want SequencedTaskRunner

It's more pedantic than anything, but it highlights the expectation that each
task you post should run in the order you posted them (i.e. not in parallel)

[mshelley, 2014/07/21 23:08:28]

Done.

+      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();
+       ++it) {
+    task_runner->PostTask(FROM_HERE, it->callback);
+  }
 }
 
 // Timeout for the SSL handshake portion of the connect.