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..93b7c3f97df4b9d0fe6cfc402515f4f4750a8011 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 pointers are used here to ensure that
+  // 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,39 @@ 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);

[wtc, 2014/07/14 22:39:18]

I still think we can run ConnectNewLeader directly, and ConnectNewLeader just
needs to post the callback of the new leader as a task.

[Ryan Sleevi, 2014/07/15 00:48:11]

Wan-Teh: I'm not sure we can. Happy to follow-up offline about this, to avoid
mixed-communication.

[wtc, 2014/07/15 21:27:08]

Ryan and I discussed this in person. You can go ahead and make my suggested
change. The code that runs error_callback_ in SSLClientSocket::Disconnect() will
address the issue that Ryan was worried about.

[wtc, 2014/07/16 21:48:12]

The purpose of the PostTask should be explained with a comment. It is not
obvious.

[mshelley, 2014/07/17 16:28:03]

Done.

+}
+
+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 =
+      pending_sockets_and_callbacks_.begin();
+
+  SSLClientSocket* ssl_socket = it->socket;
+  base::Closure& callback = it->callback;
+
+  // If there were no valid pending sockets, return.
+  if (it == pending_sockets_and_callbacks_.end())
+    return;

[wtc, 2014/07/14 22:39:18]

BUG: Check |it| first, before dereferencing |it|:

  std::vector<SocketAndCallback>::iterator it =
      pending_sockets_and_callbacks_.begin();

  // If there were no valid pending sockets, return.
  if (it == pending_sockets_and_callbacks_.end())
    return;

  SSLClientSocket* ssl_socket = it->socket;
  base::Closure& callback = it->callback;

  pending_sockets_and_callbacks_.erase(it);

  MonitorConnectionResult(ssl_socket);
  callback.Run();

Alternatively you can use the empty() method:

  // If there were no valid pending sockets, return.
  if (pending_sockets_and_callbacks_.empty())
    return;

  std::vector<SocketAndCallback>::iterator it =
      pending_sockets_and_callbacks_.begin();
  SSLClientSocket* ssl_socket = it->socket;
  base::Closure& callback = it->callback;
  pending_sockets_and_callbacks_.erase(it);

  MonitorConnectionResult(ssl_socket);
  callback.Run();

[mshelley, 2014/07/17 16:28:03]

Done.

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

[wtc, 2014/07/14 23:57:31]

I believe that if the SSLClientSocket destructor (or rather,
SSLClientSocket::Disconnect) calls the SocketFailure callback, then it is safe
to post |callback| as a task here.

[mshelley, 2014/07/17 16:28:03]

Done.

 }
 
 void SSLConnectJobMessenger::RunAllJobs(
     std::vector<SocketAndCallback>& pending_sockets_and_callbacks) {
+  scoped_refptr<base::SingleThreadTaskRunner> task_runner =
+      base::ThreadTaskRunnerHandle::Get();

[Ryan Sleevi, 2014/07/16 21:54:43]

Use a SequencedTaskRunner here instead. Or just TaskRunner. There's no need to
"couple" to the STTR here, even if that's what TTRH::Get returns

(Think of the type expression as a set of "Here are my requirements". Your only
requirement here is that tasks run)

[mshelley, 2014/07/17 16:28:03]

Done.

   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);

[wtc, 2014/07/16 21:48:12]

Based on my understanding of the problem (the somewhat recursive call sequence),
I think it is fine for RunAllJobs to run the callbacks directly. The reason is
that the sockets in question don't have any SocketSuccess or SocketFailure
callbacks set, so they won't call back to the Messenger.

Since this is the common code path (called by OnJobSucceeded), running the
callbacks directly will improve performance.

[Ryan Sleevi, 2014/07/16 21:54:43]

I think this requires some clarifications, and as I mentioned to wtc, I do find
this slightly easier to reason about with threading, even though it's slightly
less performant.

You can end up deleting the Messenger, by virtue of a socket operation then
causing the pool to be deleted, which then deletes the messenger.

If some code in Messenger was responsible for calling RunAllTasks (meaning it's
not the top of the MessageLoop task execution), then that code might access
member variables, and thus crash (because the Messenger was since deleted).

This is not to say it's unsolvable, but it does mean that one has a harder time
reasoning about the safety of this implementation, in that you have to then go
validate these invariants.

[mshelley, 2014/07/17 16:28:03]

So if I understand correctly, the reason to continue using PostTask here is as
follows: 

Say I have 2 pending sockets. I directly run the callback of the first socket.
Somehow running that callback deletes the messenger. I try to return to my
for-loop after running that callback so that I can run the next callback, but
the messenger has already been deleted and things crash. 

If I use post task, this won't be an issue because I won't actually be executing
the callbacks inside of the for loop, so they won't be able to delete the
messenger until after the messenger has finished doing everything that it needs
to do.

Based upon that understanding, it makes more sense to me to continue using
PostTask despite the performance drawbacks. Thoughts?

[wtc, 2014/07/18 01:31:57]

It turns out that you can do "delete this;" in a method as long as you don't
access any member afterwards. This is why we take care to copy the contents of
every member we need to local variables and setting the data members to the
desired new values (often NULL or empty) before running the callbacks. We do
this a lot in our network stack code.

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