Implements new, more robust design for communicating between SSLConnectJobs.

net/socket/ssl_client_socket_unittest.cc

Index: net/socket/ssl_client_socket_unittest.cc
diff --git a/net/socket/ssl_client_socket_unittest.cc b/net/socket/ssl_client_socket_unittest.cc
index 3aa445a72f8795eac9bd305e20c203ab41595d64..e9dcaa51e091638c26c48de67aa71b82a449ac20 100644
--- a/net/socket/ssl_client_socket_unittest.cc
+++ b/net/socket/ssl_client_socket_unittest.cc
@@ -658,12 +658,15 @@ class SSLClientSocketTest : public PlatformTest {
   SSLClientSocketTest()
       : socket_factory_(ClientSocketFactory::GetDefaultFactory()),
         cert_verifier_(new MockCertVerifier),
-        transport_security_state_(new TransportSecurityState) {
+        transport_security_state_(new TransportSecurityState),
+        ran_handshake_completion_callback_(false) {
     cert_verifier_->set_default_result(OK);
     context_.cert_verifier = cert_verifier_.get();
     context_.transport_security_state = transport_security_state_.get();
   }
 
+  void RecordCompletedHandshake() { ran_handshake_completion_callback_ = true; }
+
  protected:
   // The address of the spawned test server, after calling StartTestServer().
   const AddressList& addr() const { return addr_; }
@@ -730,6 +733,12 @@ class SSLClientSocketTest : public PlatformTest {
       return false;
     }
 
+#if defined(USE_OPENSSL)
+    sock_->SetHandshakeCompletionCallback(
+        base::Bind(&SSLClientSocketTest::RecordCompletedHandshake,
+                   base::Unretained(this)));
+#endif

[wtc, 2014/08/03 01:49:10]

Please remove this. In the CompletionCallbackIsRun_WithSuccess test, please
inline the CreateAndConnectSSLClientSocket call.

[mshelley, 2014/08/03 23:37:10]

Done.

+
     *result = callback_.GetResult(sock_->Connect(callback_.callback()));
     return true;
   }
@@ -740,6 +749,7 @@ class SSLClientSocketTest : public PlatformTest {
   SSLClientSocketContext context_;
   scoped_ptr<SSLClientSocket> sock_;
   CapturingNetLog log_;
+  bool ran_handshake_completion_callback_;
 
  private:
   scoped_ptr<StreamSocket> transport_;
@@ -2814,4 +2824,159 @@ TEST_F(SSLClientSocketChannelIDTest, FailingChannelIDAsync) {
   EXPECT_FALSE(sock_->IsConnected());
 }
 
+#if defined(USE_OPENSSL)
+
+TEST_F(SSLClientSocketTest, CompletionCallbackIsRun_WithFailure) {

[wtc, 2014/08/03 01:49:10]

In your test names, change "CompletionCallback" to "HandshakeCallback" to avoid
confusion with TestCompletionCallback.

(I think "HandshakeCompletionCallback" would make the test names too long.)

[mshelley, 2014/08/03 23:37:10]

Done.

+  SpawnedTestServer test_server(SpawnedTestServer::TYPE_HTTPS,
+                                SpawnedTestServer::kLocalhost,
+                                base::FilePath());
+  ASSERT_TRUE(test_server.Start());
+
+  AddressList addr;
+  ASSERT_TRUE(test_server.GetAddressList(&addr));
+
+  TestCompletionCallback callback;
+  scoped_ptr<StreamSocket> real_transport(
+      new TCPClientSocket(addr, NULL, NetLog::Source()));
+  scoped_ptr<SynchronousErrorStreamSocket> transport(
+      new SynchronousErrorStreamSocket(real_transport.Pass()));
+  int rv = callback.GetResult(transport->Connect(callback.callback()));
+  EXPECT_EQ(OK, rv);
+
+  // Disable TLS False Start to avoid handshake non-determinism.
+  SSLConfig ssl_config;
+  ssl_config.false_start_enabled = false;
+
+  SynchronousErrorStreamSocket* raw_transport = transport.get();
+  scoped_ptr<SSLClientSocket> sock(
+      CreateSSLClientSocket(transport.PassAs<StreamSocket>(),
+                            test_server.host_port_pair(),
+                            ssl_config));
+
+  sock->SetHandshakeCompletionCallback(base::Bind(
+      &SSLClientSocketTest::RecordCompletedHandshake, base::Unretained(this)));
+
+  rv = callback.GetResult(sock->Connect(callback.callback()));

[wtc, 2014/08/03 01:49:10]

IMPORTANT: please base this unit test on the Connect_WithSynchronousError test,
so that we are failing during the handshake rather than while reading the
application data.

This means we should call
  raw_transport->SetNextWriteError(ERR_CONNECTION_RESET);
before this line, and we should expect rv to be ERR_CONNECTION_RESET and
sock->IsConnected() to be false. Then the only other thing this test needs is
the final EXPECT_TRUE(ran_handshake_completion_callback_) line.

[mshelley, 2014/08/03 23:37:10]

Done.

+  EXPECT_EQ(OK, rv);
+  EXPECT_TRUE(sock->IsConnected());
+
+  const char request_text[] = "GET / HTTP/1.0\r\n\r\n";
+  static const int kRequestTextSize =
+      static_cast<int>(arraysize(request_text) - 1);
+  scoped_refptr<IOBuffer> request_buffer(new IOBuffer(kRequestTextSize));
+  memcpy(request_buffer->data(), request_text, kRequestTextSize);
+
+  rv = callback.GetResult(
+      sock->Write(request_buffer.get(), kRequestTextSize, callback.callback()));
+  EXPECT_EQ(kRequestTextSize, rv);
+
+  // Simulate an unclean/forcible shutdown.
+  raw_transport->SetNextReadError(ERR_CONNECTION_RESET);
+
+  scoped_refptr<IOBuffer> buf(new IOBuffer(4096));
+
+  // Note: This test will hang if this bug has regressed. Simply checking that
+  // rv != ERR_IO_PENDING is insufficient, as ERR_IO_PENDING is a legitimate
+  // result when using a dedicated task runner for NSS.
+  rv = callback.GetResult(sock->Read(buf.get(), 4096, callback.callback()));
+
+  EXPECT_TRUE(ran_handshake_completion_callback_);
+}
+
+TEST_F(SSLClientSocketTest, CompletionCallbackIsRun_WithFalseStartFailure) {

[wtc, 2014/08/03 01:49:11]

IMPORTANT: did you verify that the handshake is actually failing? That is, we
should not receive the server's Finished message before getting the
ERR_CONNECTION_RESET error. I am worried that the we get the
ERR_CONNECTION_RESET error after the handshake has finished successfully.

If you didn't verify that, please remove this unit test from this CL and work on
this unit test in a new CL.

+  SpawnedTestServer test_server(SpawnedTestServer::TYPE_HTTPS,
+                                SpawnedTestServer::kLocalhost,
+                                base::FilePath());
+  ASSERT_TRUE(test_server.Start());
+
+  AddressList addr;
+  ASSERT_TRUE(test_server.GetAddressList(&addr));
+
+  TestCompletionCallback callback;
+  scoped_ptr<StreamSocket> real_transport(
+      new TCPClientSocket(addr, NULL, NetLog::Source()));
+  // Note: |error_socket|'s ownership is handed to |transport|, but a pointer
+  // is retained in order to configure additional errors.
+  scoped_ptr<SynchronousErrorStreamSocket> error_socket(
+      new SynchronousErrorStreamSocket(real_transport.Pass()));
+  SynchronousErrorStreamSocket* raw_error_socket = error_socket.get();
+  scoped_ptr<FakeBlockingStreamSocket> transport(
+      new FakeBlockingStreamSocket(error_socket.PassAs<StreamSocket>()));
+  FakeBlockingStreamSocket* raw_transport = transport.get();
+  int rv = callback.GetResult(transport->Connect(callback.callback()));
+  EXPECT_EQ(OK, rv);
+
+  SSLConfig ssl_config;
+  ssl_config.false_start_enabled = true;
+
+  scoped_ptr<SSLClientSocket> sock(
+      CreateSSLClientSocket(transport.PassAs<StreamSocket>(),
+                            test_server.host_port_pair(),
+                            ssl_config));
+
+  sock->SetHandshakeCompletionCallback(base::Bind(
+      &SSLClientSocketTest::RecordCompletedHandshake, base::Unretained(this)));
+
+  rv = callback.GetResult(sock->Connect(callback.callback()));
+  EXPECT_EQ(OK, rv);
+  EXPECT_TRUE(sock->IsConnected());
+
+  const char request_text[] = "GET / HTTP/1.0\r\n\r\n";
+  static const int kRequestTextSize =
+      static_cast<int>(arraysize(request_text) - 1);
+  scoped_refptr<IOBuffer> request_buffer(new IOBuffer(kRequestTextSize));
+  memcpy(request_buffer->data(), request_text, kRequestTextSize);
+
+  // Simulate an unclean/forcible shutdown on the underlying socket.
+  // However, simulate this error asynchronously.
+  raw_error_socket->SetNextWriteError(ERR_CONNECTION_RESET);
+  raw_transport->BlockWrite();
+
+  // This write should complete synchronously, because the TLS ciphertext
+  // can be created and placed into the outgoing buffers independent of the
+  // underlying transport.
+  rv = callback.GetResult(
+      sock->Write(request_buffer.get(), kRequestTextSize, callback.callback()));
+  EXPECT_EQ(kRequestTextSize, rv);
+
+  scoped_refptr<IOBuffer> buf(new IOBuffer(4096));
+
+  rv = sock->Read(buf.get(), 4096, callback.callback());
+  EXPECT_EQ(ERR_IO_PENDING, rv);
+
+  // Now unblock the outgoing request, having it fail with the connection
+  // being reset.
+  raw_transport->UnblockWrite();
+
+  // Note: This will cause an inifite loop if this bug has regressed. Simply
+  // checking that rv != ERR_IO_PENDING is insufficient, as ERR_IO_PENDING
+  // is a legitimate result when using a dedicated task runner for NSS.

[wtc, 2014/08/03 01:49:10]

1. Remove this comment block. It makes sense in the original tests you copied
this from, but doesn't make sense here.

2. Add
    EXPECT_EQ(ERR_CONNECTION_RESET, rv);
after this line.

+  rv = callback.GetResult(rv);
+
+  EXPECT_TRUE(ran_handshake_completion_callback_);
+}
+
+// Tests that the completion callback is run when an SSL connection
+// completes successfully.
+TEST_F(SSLClientSocketTest, CompletionCallbackIsRun_WithSuccess) {
+  SpawnedTestServer::SSLOptions ssl_options;
+
+  ASSERT_TRUE(ConnectToTestServer(ssl_options));
+
+  base::FilePath certs_dir = GetTestCertsDirectory();
+  SSLConfig ssl_config = kDefaultSSLConfig;
+  ssl_config.send_client_cert = true;
+  ssl_config.client_cert = ImportCertFromFile(certs_dir, "client_1.pem");

[wtc, 2014/08/03 01:49:10]

Please delete these four lines and just use kDefaultSSLConfig on line 2972.

Hmm... we may need to disable False Start in this test to eliminate test
flakiness. If False Start is enabled, when sock_->Connect() is finished, the
handshake may not have completed.

[mshelley, 2014/08/03 23:37:09]

Done.

+
+  int rv;
+  ASSERT_TRUE(CreateAndConnectSSLClientSocket(ssl_config, &rv));
+  EXPECT_EQ(OK, rv);
+
+  EXPECT_TRUE(sock_->IsConnected());
+
+  EXPECT_TRUE(ran_handshake_completion_callback_);
+}
+
+#endif  // defined(USE_OPENSSL)
+
 }  // namespace net