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),
+      block_in_connect_(false),
+      blocked_in_connect_(false),
+      is_in_session_cache_(true) {

[wtc, 2014/07/18 01:17:14]

Why is |is_in_session_cache_| initialized to true?

[mshelley, 2014/07/18 21:08:30]

Done.

   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() {
 }
 
+std::vector<MockSSLClientSocket*>
+MockClientSocketFactory::GetSSLClientSockets() {
+  return ssl_client_sockets_;
+}
+
 const char MockClientSocket::kTlsUnique[] = "MOCK_TLSUNIQ";
 
 MockClientSocket::MockClientSocket(const BoundNetLog& net_log)
     : connected_(false),
       net_log_(net_log),
       weak_factory_(this) {
   IPAddressNumber ip;
   CHECK(ParseIPLiteralToNumber("192.0.2.33", &ip));
   peer_addr_ = IPEndPoint(ip, 0);
 }
 
 int MockClientSocket::SetReceiveBufferSize(int32 size) {
   return OK;
 }
 
 int MockClientSocket::SetSendBufferSize(int32 size) {
   return OK;
 }
 
 void MockClientSocket::Disconnect() {
   connected_ = false;
 }
 
 bool MockClientSocket::IsConnected() const {
   return connected_;
 }
 
+bool MockClientSocket::IsConnectedSSL() const {
+  return connected_;
+}
+
 bool MockClientSocket::IsConnectedAndIdle() const {
   return connected_;
 }
 
 int MockClientSocket::GetPeerAddress(IPEndPoint* address) const {
   if (!IsConnected())
     return ERR_SOCKET_NOT_CONNECTED;
   *address = peer_addr_;
   return OK;
 }
 
 int MockClientSocket::GetLocalAddress(IPEndPoint* address) const {
   IPAddressNumber ip;
   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 {
+  return true;
+}
+
+void MockClientSocket::SetHandshakeSuccessCallback(const base::Closure& cb) {
+}
+
+void MockClientSocket::SetHandshakeFailureCallback(const base::Closure& cb) {
+}

[wtc, 2014/07/18 15:39:16]

You may want to assert NOTREACHED or NOTIMPLEMENTED in these two methods.

[mshelley, 2014/07/18 21:08:30]

Done.

+
 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;
 }
 
+CompletionCallback MockClientSocket::GetResumptionCallback() {
+  return resumption_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 513 matching lines @@
     ssl_client_socket->connected_ = true;
   callback.Run(rv);
 }
 
 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) {
   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 rv = transport_->socket()->Connect(
-      base::Bind(&ConnectCallback, base::Unretained(this), callback));
+  int rv;
+  if (!data_->blocked_in_connect_)
+    rv = transport_->socket()->Connect(
+        base::Bind(&ConnectCallback, base::Unretained(this), callback));
+  else
+    rv = OK;

[wtc, 2014/07/18 01:17:14]

Add curly braces to both the "if" and "else" branches.

The local conventions in our network stack recommends the following:

1. Use curly braces if the condition or the branch spans more than one line.

2. If one branch uses curly braces, the other branch should also use curly
braces.

[mshelley, 2014/07/18 21:08:31]

Done.

+
   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_->connect.mode == ASYNC) {

[wtc, 2014/07/18 15:39:16]

IMPORTANT: in the original code, the "if (data_->connect.result == OK)" and "if
(data_->connect.mode == ASYNC)" statements are peers at the same level. In the
new code, they become nested. Please confirm this is intended.

[mshelley, 2014/07/18 21:08:30]

That was not intended -- I fixed it.

+        if (data_->block_in_connect_) {
+          resumption_callback_ = callback;
+          data_->block_in_connect_ = false;
+          data_->blocked_in_connect_ = true;
+          return ERR_IO_PENDING;
+        } else {

[wtc, 2014/07/18 01:17:14]

We usually omit "else" after a return statement. The benefit is that the code
doesn't need to be indented.

[mshelley, 2014/07/18 21:08:30]

Done.

+          if (!success_callback_.is_null()) {
+            success_callback_.Run();
+          }

[wtc, 2014/07/18 01:17:14]

Omit curly braces.

The local convention in our network stack usually omit curly braces when the
branch is just one line.

Also fix lines 1402-1404.

[mshelley, 2014/07/18 21:08:30]

Done.

+          RunCallbackAsync(callback, data_->connect.result);
+        }
+        return ERR_IO_PENDING;
+      }
+
+      // The presence of |success_callback_| indicates that SSL Connect Job
+      // Waiting is enabled.
+      if (!success_callback_.is_null()) {
+        success_callback_.Run();
+      }
+    } else if (!error_callback_.is_null()) {
+      if (data_->block_in_connect_) {
+        resumption_callback_ = callback;
+        data_->block_in_connect_ = false;
+        data_->blocked_in_connect_ = true;
+        return ERR_IO_PENDING;
+      }
+      error_callback_.Run();

[wtc, 2014/07/18 15:39:16]

BUG: we need to reset a callback when we run it. We also need to reset the other
callback. Please check the whole function.

[mshelley, 2014/07/18 21:08:30]

Done.

     }
+
     return data_->connect.result;
   }
   return rv;
 }
-
 void MockSSLClientSocket::Disconnect() {

[wtc, 2014/07/18 15:39:16]

Resurrect the blank line before this.

[mshelley, 2014/07/18 21:08:30]

Done.

   MockClientSocket::Disconnect();
   if (transport_->socket() != NULL)
     transport_->socket()->Disconnect();
 }
 
 bool MockSSLClientSocket::IsConnected() const {
   return transport_->socket()->IsConnected();
 }
 
+bool MockSSLClientSocket::IsConnectedSSL() const {
+  return connected_;

[wtc, 2014/07/18 01:17:14]

IMPORTANT: it seems better to just use the existing IsConnected() method, and
define it as follows:

  return transport_->socket()->IsConnected() && connected_;

I didn't review the test code, so this suggestion may be wrong.

[mshelley, 2014/07/18 21:08:31]

That makes sense to me. It seemed strange that SSLClientSocket's IsConnected()
method was really only returning whether or not the TCP socket was connected.

+}
+
 bool MockSSLClientSocket::WasEverUsed() const {
   return transport_->socket()->WasEverUsed();
 }
 
 bool MockSSLClientSocket::UsingTCPFastOpen() const {
   return transport_->socket()->UsingTCPFastOpen();
 }
 
 int MockSSLClientSocket::GetPeerAddress(IPEndPoint* address) const {
   return transport_->socket()->GetPeerAddress(address);
@@ skipping 13 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::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;
   *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