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 65 matching lines @@
   DCHECK_EQ(GetConnectionType(), SOCKS_PROXY);
   return socks_proxy_params_;
 }
 
 const scoped_refptr<HttpProxySocketParams>&
 SSLSocketParams::GetHttpProxyConnectionParams() const {
   DCHECK_EQ(GetConnectionType(), HTTP_PROXY);
   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.
   if (ssl_socket->InSessionCache() || connecting_sockets_.empty()) {
     return true;
   }
   return false;
 }
 
 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,
                                               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();
   RunAllJobs(temp_list);
 }
 
 void SSLConnectJobMessenger::OnJobFailed() {
-  if (pending_sockets_and_callbacks_.empty())
+  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());
+  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;
-  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());
+
+  // 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.
 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 661 matching lines @@
 // static
 bool SSLClientSocketPool::GetEnableConnectJobWaiting() {
   return enable_connect_job_waiting_;
 }
 
 void SSLClientSocketPool::OnSSLConfigChanged() {
   FlushWithError(ERR_NETWORK_CHANGED);
 }
 
 }  // namespace net