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

net/socket/ssl_client_socket_pool.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/socket/ssl_client_socket_pool.h"
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #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"
 #include "net/http/http_proxy_client_socket.h"
 #include "net/http/http_proxy_client_socket_pool.h"
 #include "net/socket/client_socket_factory.h"
 #include "net/socket/client_socket_handle.h"
 #include "net/socket/socks_client_socket_pool.h"
 #include "net/socket/ssl_client_socket.h"
 #include "net/socket/transport_client_socket_pool.h"
@@ skipping 86 matching lines @@
            pending_sockets_and_callbacks_.begin();
        it != pending_sockets_and_callbacks_.end();
        ++it) {
     if (it->socket == ssl_socket) {
       pending_sockets_and_callbacks_.erase(it);
       break;
     }
   }
 }
 
+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.
   return ssl_socket->InSessionCache() || connecting_sockets_.empty();
 }
 
 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,
                                               const base::Closure& callback) {
   pending_sockets_and_callbacks_.push_back(SocketAndCallback(socket, callback));
 }
 
 void SSLConnectJobMessenger::OnJobSucceeded() {
   SSLPendingSocketsAndCallbacks temp_list = pending_sockets_and_callbacks_;
   pending_sockets_and_callbacks_.clear();
   connecting_sockets_.clear();
   RunAllCallbacks(temp_list);
 }
 
 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.
 static const int kSSLHandshakeTimeoutInSeconds = 30;
 
 SSLConnectJob::SSLConnectJob(const std::string& group_name,
                              RequestPriority priority,
                              const scoped_refptr<SSLSocketParams>& params,
                              const base::TimeDelta& timeout_duration,
                              TransportClientSocketPool* transport_pool,
@@ skipping 660 matching lines @@
   if (base_.CloseOneIdleSocket())
     return true;
   return base_.CloseOneIdleConnectionInHigherLayeredPool();
 }
 
 void SSLClientSocketPool::OnSSLConfigChanged() {
   FlushWithError(ERR_NETWORK_CHANGED);
 }
 
 }  // namespace net