Implements new, more robust design for communicating between SSLConnectJobs.

net/socket/socket_test_util.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/socket_test_util.h"
 
 #include <algorithm>
 #include <vector>
 
 #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"
 #include "base/time/time.h"
 #include "net/base/address_family.h"
 #include "net/base/address_list.h"
 #include "net/base/auth.h"
 #include "net/base/load_timing_info.h"
 #include "net/http/http_network_session.h"
 #include "net/http/http_request_headers.h"
@@ skipping 247 matching lines @@
 }
 
 SSLSocketDataProvider::SSLSocketDataProvider(IoMode mode, int result)
     : connect(mode, result),
       next_proto_status(SSLClientSocket::kNextProtoUnsupported),
       was_npn_negotiated(false),
       protocol_negotiated(kProtoUnknown),
       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
   SSLConnectionStatusSetCipherSuite(0xcc14, &connection_status);
 }
 
 SSLSocketDataProvider::~SSLSocketDataProvider() {
 }
 
 void SSLSocketDataProvider::SetNextProto(NextProto proto) {
@@ skipping 401 matching lines @@
       new MockTCPClientSocket(addresses, net_log, data_provider));
   data_provider->set_socket(socket.get());
   return socket.PassAs<StreamSocket>();
 }
 
 scoped_ptr<SSLClientSocket> MockClientSocketFactory::CreateSSLClientSocket(
     scoped_ptr<ClientSocketHandle> transport_socket,
     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() {
 }
 
 const char MockClientSocket::kTlsUnique[] = "MOCK_TLSUNIQ";
 
 MockClientSocket::MockClientSocket(const BoundNetLog& net_log)
     : connected_(false),
       net_log_(net_log),
@@ skipping 35 matching lines @@
   bool rv = ParseIPLiteralToNumber("192.0.2.33", &ip);
   CHECK(rv);
   *address = IPEndPoint(ip, 123);
   return OK;
 }
 
 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) {
 }
 
 int MockClientSocket::ExportKeyingMaterial(const base::StringPiece& label,
                                            bool has_context,
                                            const base::StringPiece& context,
                                            unsigned char* out,
                                            unsigned int outlen) {
   memset(out, 'A', outlen);
   return OK;
 }
 
 int MockClientSocket::GetTLSUniqueChannelBinding(std::string* out) {
   out->assign(MockClientSocket::kTlsUnique);
   return OK;
 }
 
 ServerBoundCertService* MockClientSocket::GetServerBoundCertService() const {
   NOTREACHED();
   return NULL;
 }
 
+void MockSSLClientSocket::RestartPausedConnect() {
+  Connect(connect_callback_);
+}
+
 SSLClientSocket::NextProtoStatus
 MockClientSocket::GetNextProto(std::string* proto, std::string* server_protos) {
   proto->clear();
   server_protos->clear();
   return SSLClientSocket::kNextProtoUnsupported;
 }
 
 scoped_refptr<X509Certificate>
 MockClientSocket::GetUnverifiedServerCertificateChain() const {
   NOTREACHED();
@@ skipping 502 matching lines @@
 void DeterministicMockTCPClientSocket::OnReadComplete(const MockRead& data) {}
 
 void DeterministicMockTCPClientSocket::OnConnectComplete(
     const MockConnect& data) {}
 
 // static
 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(
     scoped_ptr<ClientSocketHandle> transport_socket,
     const HostPortPair& host_port_pair,
     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;
 }
 
 MockSSLClientSocket::~MockSSLClientSocket() {
   Disconnect();
 }
 
 int MockSSLClientSocket::Read(IOBuffer* buf, int buf_len,
                               const CompletionCallback& callback) {
   return transport_->socket()->Read(buf, buf_len, callback);
 }
 
 int MockSSLClientSocket::Write(IOBuffer* buf, int buf_len,
                                const CompletionCallback& callback) {
   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;
 }
 
 void MockSSLClientSocket::Disconnect() {
   MockClientSocket::Disconnect();
   if (transport_->socket() != NULL)
     transport_->socket()->Disconnect();
 }
 
 bool MockSSLClientSocket::IsConnected() const {
-  return transport_->socket()->IsConnected();
+  return transport_->socket()->IsConnected() && connected_;
 }
 
 bool MockSSLClientSocket::WasEverUsed() const {
   return transport_->socket()->WasEverUsed();
 }
 
 bool MockSSLClientSocket::UsingTCPFastOpen() const {
   return transport_->socket()->UsingTCPFastOpen();
 }
 
@@ skipping 15 matching lines @@
   DCHECK(cert_request_info);
   if (data_->cert_request_info) {
     cert_request_info->host_and_port =
         data_->cert_request_info->host_and_port;
     cert_request_info->client_certs = data_->cert_request_info->client_certs;
   } else {
     cert_request_info->Reset();
   }
 }
 
+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;
   *server_protos = data_->server_protos;
   return data_->next_proto_status;
 }
 
 bool MockSSLClientSocket::set_was_npn_negotiated(bool negotiated) {
   is_npn_state_set_ = true;
   return new_npn_value_ = negotiated;
@@ skipping 494 matching lines @@
 
 const char kSOCKS5OkRequest[] =
     { 0x05, 0x01, 0x00, 0x03, 0x04, 'h', 'o', 's', 't', 0x00, 0x50 };
 const int kSOCKS5OkRequestLength = arraysize(kSOCKS5OkRequest);
 
 const char kSOCKS5OkResponse[] =
     { 0x05, 0x00, 0x00, 0x01, 127, 0, 0, 1, 0x00, 0x50 };
 const int kSOCKS5OkResponseLength = arraysize(kSOCKS5OkResponse);
 
 }  // namespace net