Unwind the SSL connection holdback experiment and remove related code

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 791c9f684804dd7fd74ac8adee3b2a5e7d8cc164..cd5f95fff5358c08f012f99b4425bd3c13b2a6fa 100644
--- a/net/socket/socket_test_util.cc
+++ b/net/socket/socket_test_util.cc
@@ -10,7 +10,6 @@
 #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"
@@ -279,9 +278,7 @@ SSLSocketDataProvider::SSLSocketDataProvider(IoMode mode, int result)
       client_cert_sent(false),
       cert_request_info(NULL),
       channel_id_sent(false),
-      connection_status(0),
-      should_pause_on_connect(false),
-      is_in_session_cache(false) {
+      connection_status(0) {
   SSLConnectionStatusSetVersion(SSL_CONNECTION_VERSION_TLS1_2,
                                 &connection_status);
   // Set to TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305
@@ -712,10 +709,8 @@ scoped_ptr<SSLClientSocket> MockClientSocketFactory::CreateSSLClientSocket(
                    next_ssl_data->next_protos_expected_in_ssl_config.end(),
                    ssl_config.next_protos.begin()));
   }
-  scoped_ptr<MockSSLClientSocket> socket(new MockSSLClientSocket(
+  return scoped_ptr<SSLClientSocket>(new MockSSLClientSocket(
       transport_socket.Pass(), host_and_port, ssl_config, next_ssl_data));
-  ssl_client_sockets_.push_back(socket.get());
-  return socket.Pass();
 }
 
 void MockClientSocketFactory::ClearSSLSessionCache() {
@@ -771,20 +766,6 @@ const BoundNetLog& MockClientSocket::NetLog() const {
   return net_log_;
 }
 
-std::string MockClientSocket::GetSessionCacheKey() const {
-  NOTIMPLEMENTED();
-  return std::string();
-}
-
-bool MockClientSocket::InSessionCache() const {
-  NOTIMPLEMENTED();
-  return false;
-}
-
-void MockClientSocket::SetHandshakeCompletionCallback(const base::Closure& cb) {
-  NOTIMPLEMENTED();
-}
-
 void MockClientSocket::GetSSLCertRequestInfo(
   SSLCertRequestInfo* cert_request_info) {
 }
@@ -1324,25 +1305,31 @@ 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);
+}
+
 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()),

[davidben, 2015/03/09 18:02:36]

I think the RHS is correct, right? (Eh, git cl format will have an opinion.)

+         // Have to use the right BoundNetLog for LoadTimingInfo regression
+         // tests.
+         transport_socket->socket()->NetLog()),
       transport_(transport_socket.Pass()),
-      host_port_pair_(host_port_pair),
       data_(data),
       is_npn_state_set_(false),
       new_npn_value_(false),
       is_protocol_negotiated_set_(false),
-      protocol_negotiated_(kProtoUnknown),
-      next_connect_state_(STATE_NONE),
-      reached_connect_(false),
-      weak_factory_(this) {
+      protocol_negotiated_(kProtoUnknown) {
   DCHECK(data_);
   peer_addr_ = data->connect.peer_addr;
 }
@@ -1362,23 +1349,28 @@ int MockSSLClientSocket::Write(IOBuffer* buf, int buf_len,
 }
 
 int MockSSLClientSocket::Connect(const CompletionCallback& callback) {
-  next_connect_state_ = STATE_SSL_CONNECT;
-  reached_connect_ = true;
-  int rv = DoConnectLoop(OK);
-  if (rv == ERR_IO_PENDING)
-    connect_callback_ = callback;
+  int rv = transport_->socket()->Connect(
+      base::Bind(&ConnectCallback, base::Unretained(this), callback));

[davidben, 2015/03/09 18:02:36]

I'm slightly confused about what happened here. The LHS never calls Connect on
the transport, as far as I can tell. Oversight when adding the other features?

The WeakPtr is probably worth retaining (hah). I'm guessing some test crashed if
the MockSSLClientSocket was destroyed before Connect completed.

[Ryan Sleevi, 2015/03/09 21:23:56]

If it does, then the underlying transport isn't adhering to the mock contract
(it's legal to delete while pending)

[davidben, 2015/03/11 01:05:13]

Acknowledged.

+  if (rv == OK) {
+    if (data_->connect.result == OK)
+      connected_ = true;
+    if (data_->connect.mode == ASYNC) {
+      RunCallbackAsync(callback, data_->connect.result);
+      return ERR_IO_PENDING;
+    }
+    return data_->connect.result;
+  }
   return rv;
 }
 
 void MockSSLClientSocket::Disconnect() {
-  weak_factory_.InvalidateWeakPtrs();
   MockClientSocket::Disconnect();
   if (transport_->socket() != NULL)
     transport_->socket()->Disconnect();
 }
 
 bool MockSSLClientSocket::IsConnected() const {
-  return transport_->socket()->IsConnected() && connected_;
+  return transport_->socket()->IsConnected();
 }
 
 bool MockSSLClientSocket::WasEverUsed() const {
@@ -1402,21 +1394,6 @@ bool MockSSLClientSocket::GetSSLInfo(SSLInfo* ssl_info) {
   return true;
 }
 
-std::string MockSSLClientSocket::GetSessionCacheKey() const {
-  // For the purposes of these tests, |host_and_port| will serve as the
-  // cache key.
-  return host_port_pair_.ToString();
-}
-
-bool MockSSLClientSocket::InSessionCache() const {
-  return data_->is_in_session_cache;
-}
-
-void MockSSLClientSocket::SetHandshakeCompletionCallback(
-    const base::Closure& cb) {
-  handshake_completion_callback_ = cb;
-}
-
 void MockSSLClientSocket::GetSSLCertRequestInfo(
     SSLCertRequestInfo* cert_request_info) {
   DCHECK(cert_request_info);
@@ -1478,69 +1455,6 @@ void MockSSLClientSocket::OnConnectComplete(const MockConnect& data) {
   NOTIMPLEMENTED();
 }
 
-void MockSSLClientSocket::RestartPausedConnect() {
-  DCHECK(data_->should_pause_on_connect);
-  DCHECK_EQ(next_connect_state_, STATE_SSL_CONNECT_COMPLETE);
-  OnIOComplete(data_->connect.result);
-}
-
-void MockSSLClientSocket::OnIOComplete(int result) {
-  int rv = DoConnectLoop(result);
-  if (rv != ERR_IO_PENDING)
-    base::ResetAndReturn(&connect_callback_).Run(rv);
-}
-
-int MockSSLClientSocket::DoConnectLoop(int result) {
-  DCHECK_NE(next_connect_state_, STATE_NONE);
-
-  int rv = result;
-  do {
-    ConnectState state = next_connect_state_;
-    next_connect_state_ = STATE_NONE;
-    switch (state) {
-      case STATE_SSL_CONNECT:
-        rv = DoSSLConnect();
-        break;
-      case STATE_SSL_CONNECT_COMPLETE:
-        rv = DoSSLConnectComplete(rv);
-        break;
-      default:
-        NOTREACHED() << "bad state";
-        rv = ERR_UNEXPECTED;
-        break;
-    }
-  } while (rv != ERR_IO_PENDING && next_connect_state_ != STATE_NONE);
-
-  return rv;
-}
-
-int MockSSLClientSocket::DoSSLConnect() {
-  next_connect_state_ = STATE_SSL_CONNECT_COMPLETE;
-
-  if (data_->should_pause_on_connect)
-    return ERR_IO_PENDING;
-
-  if (data_->connect.mode == ASYNC) {
-    base::MessageLoop::current()->PostTask(
-        FROM_HERE,
-        base::Bind(&MockSSLClientSocket::OnIOComplete,
-                   weak_factory_.GetWeakPtr(),
-                   data_->connect.result));
-    return ERR_IO_PENDING;
-  }
-
-  return data_->connect.result;
-}
-
-int MockSSLClientSocket::DoSSLConnectComplete(int result) {
-  if (result == OK)
-    connected_ = true;
-
-  if (!handshake_completion_callback_.is_null())
-    base::ResetAndReturn(&handshake_completion_callback_).Run();
-  return result;
-}
-
 MockUDPClientSocket::MockUDPClientSocket(SocketDataProvider* data,
                                          net::NetLog* net_log)
     : connected_(false),