Implements new, more robust design for communicating between SSLConnectJobs.

net/socket/ssl_client_socket_unittest.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/ssl_client_socket.h"
 
 #include "base/callback_helpers.h"
 #include "base/memory/ref_counted.h"
 #include "base/run_loop.h"
 #include "base/time/time.h"
@@ skipping 640 matching lines @@
       OVERRIDE {}
   virtual int GetChannelIDCount() OVERRIDE { return 0; }
   virtual void SetForceKeepSessionState() OVERRIDE {}
 };
 
 class SSLClientSocketTest : public PlatformTest {
  public:
   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_; }
 
   // The SpawnedTestServer object, after calling StartTestServer().
   const SpawnedTestServer* test_server() const { return test_server_.get(); }
 
   // Starts the test server with SSL configuration |ssl_options|. Returns true
   // on success.
   bool StartTestServer(const SpawnedTestServer::SSLOptions& ssl_options) {
@@ skipping 46 matching lines @@
   // itself was a success.
   bool CreateAndConnectSSLClientSocket(SSLConfig& ssl_config, int* result) {
     sock_ = CreateSSLClientSocket(
         transport_.Pass(), test_server_->host_port_pair(), ssl_config);
 
     if (sock_->IsConnected()) {
       LOG(ERROR) << "SSL Socket prematurely connected";
       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;
   }
 
   ClientSocketFactory* socket_factory_;
   scoped_ptr<MockCertVerifier> cert_verifier_;
   scoped_ptr<TransportSecurityState> transport_security_state_;
   SSLClientSocketContext context_;
   scoped_ptr<SSLClientSocket> sock_;
   CapturingNetLog log_;
+  bool ran_handshake_completion_callback_;
 
  private:
   scoped_ptr<StreamSocket> transport_;
   scoped_ptr<SpawnedTestServer> test_server_;
   TestCompletionCallback callback_;
   AddressList addr_;
 };
 
 // Verifies the correctness of GetSSLCertRequestInfo.
 class SSLClientSocketCertRequestInfoTest : public SSLClientSocketTest {
@@ skipping 1884 matching lines @@
 
   // The socket has now been used.
   EXPECT_TRUE(sock->WasEverUsed());
 
   // TODO(davidben): Read one byte to ensure the test server has responded and
   // then assert IsConnectedAndIdle is false. This currently doesn't work
   // because neither SSLClientSocketNSS nor SSLClientSocketOpenSSL check their
   // SSL implementation's internal buffers. Either call PR_Available and
   // SSL_pending, although the former isn't actually implemented or perhaps
   // attempt to read one byte extra.
 }

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

This is the end of the last SSLClientSocketTest.xxx test in the original code.
Since your new tests are also named SSLClientSocketTest.xxx, I suggest you move
your new tests here.

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

Done.

 
 TEST_F(SSLClientSocketFalseStartTest, FalseStartEnabled) {
   // False Start requires NPN and a forward-secret cipher suite.
   SpawnedTestServer::SSLOptions server_options;
   server_options.key_exchanges =
       SpawnedTestServer::SSLOptions::KEY_EXCHANGE_DHE_RSA;
   server_options.enable_npn = true;
   SSLConfig client_config;
   client_config.next_protos.push_back("http/1.1");
   ASSERT_NO_FATAL_FAILURE(
@@ skipping 149 matching lines @@
   SSLConfig ssl_config = kDefaultSSLConfig;
   ssl_config.channel_id_enabled = true;
 
   int rv;
   ASSERT_TRUE(CreateAndConnectSSLClientSocket(ssl_config, &rv));
 
   EXPECT_EQ(ERR_UNEXPECTED, rv);
   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