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..22ea1dd6c9318b7e651c846f9f54dbac678f6d2b 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,9 @@ 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_on_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 +702,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 +764,16 @@ const BoundNetLog& MockClientSocket::NetLog() const {
   return net_log_;
 }
 
+bool MockClientSocket::InSessionCache() const {
+  // Always returns true to prevent sockets that are not
+  // OpenSSL sockets from trying to use SSLConnectJob waiting.
+  return true;

[Ryan Sleevi, 2014/07/25 01:36:34]

This does seem really weird to me. It also couples a little bit of the internal
details (e.g. we ideally wouldn't mention "OpenSSL" here)

[mshelley, 2014/07/26 00:58:27]

Perhaps it would just be better to put: 

                NOTIMPLEMENTED();
                return false;

(You typically return false in not implemented functions with a bool return
type, correct?)

In theory this method should never be called by anything but an
SSLClientSocketOpenSSL or a MockSSLClientSocket.

[Ryan Sleevi, 2014/07/26 02:22:32]

I think that will work, but you have to use appropriate compile-time guards for
the tests, potentially.

That is, your unittests will run on all build types. One way is to compile-guard
them. Another way is to have the test check if something is supported and not
run. I'm just not sure that NOTIMPLEMENTED() will work across the diversity of
bots, for !use_openssl builds

+}
+
+void MockClientSocket::SetHandshakeCompletionCallback(const base::Closure& cb) {
+  NOTIMPLEMENTED();
+}
+
 void MockClientSocket::GetSSLCertRequestInfo(
   SSLCertRequestInfo* cert_request_info) {
 }
@@ -781,6 +797,10 @@ ServerBoundCertService* MockClientSocket::GetServerBoundCertService() const {
   return NULL;
 }
 
+void MockSSLClientSocket::RestartPausedConnect() {
+  Connect(connect_callback_);
+}
+
 SSLClientSocket::NextProtoStatus
 MockClientSocket::GetNextProto(std::string* proto, std::string* server_protos) {
   proto->clear();
@@ -1303,9 +1323,11 @@ void MockSSLClientSocket::ConnectCallback(
     MockSSLClientSocket* ssl_client_socket,
     const CompletionCallback& callback,
     int rv) {
-  if (rv == OK)
-    ssl_client_socket->connected_ = true;
-  callback.Run(rv);
+  if (rv < OK) {
+    callback.Run(rv);
+    return;
+  }
+  ssl_client_socket->Connect(callback);
 }
 
 MockSSLClientSocket::MockSSLClientSocket(
@@ -1314,15 +1336,16 @@ 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),
       new_npn_value_(false),
       is_protocol_negotiated_set_(false),
-      protocol_negotiated_(kProtoUnknown) {
+      protocol_negotiated_(kProtoUnknown),
+      next_connect_state_(STATE_TCP_CONNECT) {
   DCHECK(data_);
   peer_addr_ = data->connect.peer_addr;
 }
@@ -1341,18 +1364,65 @@ int MockSSLClientSocket::Write(IOBuffer* buf, int buf_len,
   return transport_->socket()->Write(buf, buf_len, callback);
 }
 
-int MockSSLClientSocket::Connect(const CompletionCallback& callback) {
+int MockSSLClientSocket::DoTCPConnect(const CompletionCallback& callback) {
+  next_connect_state_ = STATE_SSL_CONNECT;
   int rv = transport_->socket()->Connect(
       base::Bind(&ConnectCallback, base::Unretained(this), callback));
-  if (rv == OK) {
+  return rv;
+}
+
+int MockSSLClientSocket::DoSSLConnect(const CompletionCallback& callback) {
+  next_connect_state_ = STATE_SSL_CONNECT_COMPLETE;
+
+  if (data_->should_block_on_connect) {
+    connect_callback_ = callback;
+    data_->should_block_on_connect = false;
     if (data_->connect.result == OK)
       connected_ = true;
+    return ERR_IO_PENDING;
+  }
+  return OK;
+}
+
+int MockSSLClientSocket::DoSSLConnectComplete(
+    const CompletionCallback& callback) {
+  if (data_->connect.result == OK) {
+    connected_ = true;
+    if (!handshake_completion_callback_.is_null()) {
+      // The presence of |completion_callback_| indicates that SSL Connect Job
+      // Waiting is enabled.
+      base::ResetAndReturn(&handshake_completion_callback_).Run();
+    }
     if (data_->connect.mode == ASYNC) {
       RunCallbackAsync(callback, data_->connect.result);
       return ERR_IO_PENDING;
     }
-    return data_->connect.result;
+  } else if (!handshake_completion_callback_.is_null()) {
+    base::ResetAndReturn(&handshake_completion_callback_).Run();
   }
+  return data_->connect.result;
+}
+
+int MockSSLClientSocket::Connect(const CompletionCallback& callback) {
+  int rv;

[Ryan Sleevi, 2014/07/25 01:36:34]

So this tripped me up when reviewing, and violated my expectations regarding how
our normal callbacks/re-entrant bits work

I would instead recommend this be split into

Connect(const CompletionCallback&)
  int rv = DoConnectLoop(OK);
  if (rv != ERR_IO_PENDING)
    return rv;
  connect_callback_ = cb;
  return rv;
}

int DoConnectLoop(int rv) {
  do {
     ....
  } while (rv != ERR_IO_PENDING && next_connect_state_ != STATE_NONE);
  return rv;
}


void OnSomethingNamedHere(int rv) {
  int rv = DoConnectLoop(rv);
  if (rv == ERR_IO_PENDING)
    return;
  base::ResetAndReturn(&connect_cb_).Run(rv);
}

void RestartPausedConnect() {
  DCHECK_EQ(???, next_state_);
  return OnSomethingNamedHere(OK);
}

+  do {
+    ConnectState state = next_connect_state_;
+    next_connect_state_ = STATE_NONE;
+    switch (state) {
+      case STATE_TCP_CONNECT:
+        rv = DoTCPConnect(callback);
+      case STATE_SSL_CONNECT:
+        rv = DoSSLConnect(callback);
+        break;
+      case STATE_SSL_CONNECT_COMPLETE:
+        rv = DoSSLConnectComplete(callback);
+        break;
+      default:
+        NOTREACHED();
+        break;
+    }
+  } while (rv == OK && next_connect_state_ != STATE_NONE);
+
   return rv;
 }
 
@@ -1363,7 +1433,7 @@ void MockSSLClientSocket::Disconnect() {
 }
 
 bool MockSSLClientSocket::IsConnected() const {
-  return transport_->socket()->IsConnected();
+  return transport_->socket()->IsConnected() && connected_;
 }
 
 bool MockSSLClientSocket::WasEverUsed() const {
@@ -1399,6 +1469,15 @@ void MockSSLClientSocket::GetSSLCertRequestInfo(
   }
 }
 
+bool MockSSLClientSocket::InSessionCache() const {
+  return data_->is_in_session_cache;
+}
+
+void MockSSLClientSocket::SetHandshakeCompletionCallback(
+    const base::Closure& cb) {
+  handshake_completion_callback_ = cb;
+}
+
 SSLClientSocket::NextProtoStatus MockSSLClientSocket::GetNextProto(
     std::string* proto, std::string* server_protos) {
   *proto = data_->next_proto;