Implements new, more robust design for communicating between SSLConnectJobs.

net/socket/socket_test_util.cc

Index: net/socket/socket_test_util.cc
diff --git a/net/socket/socket_test_util.cc b/net/socket/socket_test_util.cc
index f993801adeb2814aede788bcf82c693bfa99696f..1522a1ee7323befe0bc15bbc9efb321b56944306 100644
--- a/net/socket/socket_test_util.cc
+++ b/net/socket/socket_test_util.cc
@@ -10,6 +10,7 @@
 #include "base/basictypes.h"
 #include "base/bind.h"
 #include "base/bind_helpers.h"
+#include "base/callback_helpers.h"
 #include "base/compiler_specific.h"
 #include "base/message_loop/message_loop.h"
 #include "base/run_loop.h"
@@ -277,7 +278,10 @@ SSLSocketDataProvider::SSLSocketDataProvider(IoMode mode, int result)
       client_cert_sent(false),
       cert_request_info(NULL),
       channel_id_sent(false),
-      connection_status(0) {
+      connection_status(0),
+      should_block_in_connect(false),
+      blocked_in_connect(false),
+      is_in_session_cache(false) {
   SSLConnectionStatusSetVersion(SSL_CONNECTION_VERSION_TLS1_2,
                                 &connection_status);
   // Set to TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305
@@ -699,10 +703,13 @@ scoped_ptr<SSLClientSocket> MockClientSocketFactory::CreateSSLClientSocket(
     const HostPortPair& host_and_port,
     const SSLConfig& ssl_config,
     const SSLClientSocketContext& context) {
-  return scoped_ptr<SSLClientSocket>(
+  scoped_ptr<MockSSLClientSocket> socket(
       new MockSSLClientSocket(transport_socket.Pass(),
-                              host_and_port, ssl_config,
+                              host_and_port,
+                              ssl_config,
                               mock_ssl_data_.GetNext()));
+  ssl_client_sockets_.push_back(socket.get());
+  return socket.PassAs<SSLClientSocket>();
 }
 
 void MockClientSocketFactory::ClearSSLSessionCache() {
@@ -758,6 +765,18 @@ const BoundNetLog& MockClientSocket::NetLog() const {
   return net_log_;
 }
 
+bool MockClientSocket::InSessionCache() const {
+  return true;
+}
+
+void MockClientSocket::SetHandshakeSuccessCallback(const base::Closure& cb) {
+  NOTIMPLEMENTED();
+}
+
+void MockClientSocket::SetHandshakeFailureCallback(const base::Closure& cb) {
+  NOTIMPLEMENTED();
+}
+
 void MockClientSocket::GetSSLCertRequestInfo(
   SSLCertRequestInfo* cert_request_info) {
 }
@@ -781,6 +800,10 @@ ServerBoundCertService* MockClientSocket::GetServerBoundCertService() const {
   return NULL;
 }
 
+CompletionCallback MockClientSocket::connect_callback() {
+  return connect_callback_;
+}
+
 SSLClientSocket::NextProtoStatus
 MockClientSocket::GetNextProto(std::string* proto, std::string* server_protos) {
   proto->clear();
@@ -1314,9 +1337,9 @@ MockSSLClientSocket::MockSSLClientSocket(
     const SSLConfig& ssl_config,
     SSLSocketDataProvider* data)
     : MockClientSocket(
-         // Have to use the right BoundNetLog for LoadTimingInfo regression
-         // tests.
-         transport_socket->socket()->NetLog()),
+          // Have to use the right BoundNetLog for LoadTimingInfo regression
+          // tests.
+          transport_socket->socket()->NetLog()),
       transport_(transport_socket.Pass()),
       data_(data),
       is_npn_state_set_(false),
@@ -1342,14 +1365,39 @@ int MockSSLClientSocket::Write(IOBuffer* buf, int buf_len,
 }
 
 int MockSSLClientSocket::Connect(const CompletionCallback& callback) {
-  int rv = transport_->socket()->Connect(
-      base::Bind(&ConnectCallback, base::Unretained(this), callback));
+  int rv;
+  if (!data_->blocked_in_connect) {

[Ryan Sleevi, 2014/07/18 22:01:16]

When would this ever be true in a well-behaved client?

That is, Connect() can only be called before Disconnect(), and can be called
afterwards.

This sort of goes back to my "don't stick state in data providers"

+    rv = transport_->socket()->Connect(
+        base::Bind(&ConnectCallback, base::Unretained(this), callback));
+  } else {
+    rv = OK;
+  }
+
   if (rv == OK) {
-    if (data_->connect.result == OK)
+    if (data_->connect.result == OK) {
       connected_ = true;
-    if (data_->connect.mode == ASYNC) {
-      RunCallbackAsync(callback, data_->connect.result);
-      return ERR_IO_PENDING;
+      if (data_->should_block_in_connect) {
+        connect_callback_ = callback;
+        data_->should_block_in_connect = false;
+        data_->blocked_in_connect = true;
+        return ERR_IO_PENDING;

[Ryan Sleevi, 2014/07/18 22:01:16]

This doesn't look right to me.

We don't need to expose connect_callback_ to callers, we should instead have a
method on the MockSSLClientSocket that says something like
RestartPausedConnect(), which will then run connect_callback_ itself (and thus
doesn't need to be on the base mock socket, and can exist only in the
MockSSLClientSocket, and as a private member)

[mshelley, 2014/07/21 23:00:07]

Done.

+      } else if (!success_callback_.is_null()) {
+        // The presence of |success_callback_| indicates that SSL Connect Job
+        // Waiting is enabled.
+        base::ResetAndReturn(&success_callback_).Run();
+      }
+      if (data_->connect.mode == ASYNC) {
+        RunCallbackAsync(callback, data_->connect.result);
+        return ERR_IO_PENDING;
+      }
+    } else if (!error_callback_.is_null()) {
+      if (data_->should_block_in_connect) {
+        connect_callback_ = callback;
+        data_->should_block_in_connect = false;
+        data_->blocked_in_connect = true;
+        return ERR_IO_PENDING;
+      }
+      base::ResetAndReturn(&error_callback_).Run();

[Ryan Sleevi, 2014/07/18 22:01:16]

We may want to restructure this all into a more defined state machine

DoConnect()
DoConnectComplete()

etc.

The transitions are as follows:
transport_->Connect() succeeds -> enters into the "CONNECT" state
- Shouldn't block on connect
  - Immediately transitions to the ConnectComplete stage
  - ConnectComplete handles running error_callback_ vs connect_callback
- Should block on Connect
  - Sets next stage to ConnectComplete, returns ERR_IO_PENDING, is up to the
caller to call like RestartConnect
  - RestartConnect restarts the handshake loop
  - ConnectComplete again handles errors vs connect
- ConnectComplete also is responsible for RunCallbackAsync

[mshelley, 2014/07/21 23:00:07]

Done.

     }
     return data_->connect.result;
   }
@@ -1363,7 +1411,7 @@ void MockSSLClientSocket::Disconnect() {
 }
 
 bool MockSSLClientSocket::IsConnected() const {
-  return transport_->socket()->IsConnected();
+  return transport_->socket()->IsConnected() && connected_;
 }
 
 bool MockSSLClientSocket::WasEverUsed() const {
@@ -1399,6 +1447,18 @@ void MockSSLClientSocket::GetSSLCertRequestInfo(
   }
 }
 
+bool MockSSLClientSocket::InSessionCache() const {
+  return data_->is_in_session_cache;
+}
+
+void MockSSLClientSocket::SetHandshakeSuccessCallback(const base::Closure& cb) {
+  success_callback_ = cb;
+}
+
+void MockSSLClientSocket::SetHandshakeFailureCallback(const base::Closure& cb) {
+  error_callback_ = cb;
+}
+
 SSLClientSocket::NextProtoStatus MockSSLClientSocket::GetNextProto(
     std::string* proto, std::string* server_protos) {
   *proto = data_->next_proto;