This CL corrects a bug in which the OnHandshakeComplete callback for an ssl session was never called

net/socket/ssl_client_socket_openssl.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.
 
 // OpenSSL binding for SSLClientSocket. The class layout and general principle
 // of operation is derived from SSLClientSocketNSS.
 
 #include "net/socket/ssl_client_socket_openssl.h"
 
 #include <errno.h>
@@ skipping 345 matching lines @@
       ssl_(NULL),
       transport_bio_(NULL),
       transport_(transport_socket.Pass()),
       host_and_port_(host_and_port),
       ssl_config_(ssl_config),
       ssl_session_cache_shard_(context.ssl_session_cache_shard),
       trying_cached_session_(false),
       next_handshake_state_(STATE_NONE),
       npn_status_(kNextProtoUnsupported),
       channel_id_xtn_negotiated_(false),
+      ran_handshake_finished_callback_(false),
+      marked_session_as_good_(false),
       net_log_(transport_->socket()->NetLog()) {
 }
 
 SSLClientSocketOpenSSL::~SSLClientSocketOpenSSL() {
   Disconnect();
 }
 
 bool SSLClientSocketOpenSSL::InSessionCache() const {
   SSLContext* context = SSLContext::GetInstance();
   std::string cache_key = GetSessionCacheKey();
@@ skipping 52 matching lines @@
 int SSLClientSocketOpenSSL::Connect(const CompletionCallback& callback) {
   net_log_.BeginEvent(NetLog::TYPE_SSL_CONNECT);
 
   // Set up new ssl object.
   int rv = Init();
   if (rv != OK) {
     net_log_.EndEventWithNetErrorCode(NetLog::TYPE_SSL_CONNECT, rv);
     return rv;
   }
 
-  if (!handshake_completion_callback_.is_null()) {
-    SSLContext* context = SSLContext::GetInstance();
-    context->session_cache()->SetSessionAddedCallback(
-        ssl_,
-        base::Bind(&SSLClientSocketOpenSSL::OnHandshakeCompletion,
-                   base::Unretained(this)));
-  }
-
   // Set SSL to client mode. Handshake happens in the loop below.
   SSL_set_connect_state(ssl_);
 
   GotoState(STATE_HANDSHAKE);
   rv = DoHandshakeLoop(OK);
   if (rv == ERR_IO_PENDING) {
     user_connect_callback_ = callback;
   } else {
     net_log_.EndEventWithNetErrorCode(NetLog::TYPE_SSL_CONNECT, rv);
     if (rv < OK)
       OnHandshakeCompletion();
   }
 
   return rv > OK ? OK : rv;
 }
 
 void SSLClientSocketOpenSSL::Disconnect() {
   // If a handshake was pending (Connect() had been called), notify interested
   // parties that it's been aborted now. If the handshake had already
   // completed, this is a no-op.
   OnHandshakeCompletion();
   if (ssl_) {
-    SSLContext* context = SSLContext::GetInstance();
-    context->session_cache()->RemoveSessionAddedCallback(ssl_);
     // Calling SSL_shutdown prevents the session from being marked as
     // unresumable.
     SSL_shutdown(ssl_);
     SSL_free(ssl_);
     ssl_ = NULL;
   }
   if (transport_bio_) {
     BIO_free_all(transport_bio_);
     transport_bio_ = NULL;
   }
@@ skipping 200 matching lines @@
 }
 
 int SSLClientSocketOpenSSL::SetReceiveBufferSize(int32 size) {
   return transport_->socket()->SetReceiveBufferSize(size);
 }
 
 int SSLClientSocketOpenSSL::SetSendBufferSize(int32 size) {
   return transport_->socket()->SetSendBufferSize(size);
 }
 
+// static
+void SSLClientSocketOpenSSL::InfoCallback(const SSL* ssl,
+                                          int result,
+                                          int /*unused*/) {

[wtc, 2014/08/07 20:04:04]

The second parameter should be named "type". The third parameter should be named
"val".

+  SSLClientSocketOpenSSL* ssl_socket =
+      SSLContext::GetInstance()->GetClientSocketFromSSL(ssl);

[wtc, 2014/08/07 20:04:03]

Move this into the if block because it is only used inside that block.

+  if (result == SSL_CB_HANDSHAKE_DONE) {
+    ssl_socket->ran_handshake_finished_callback_ = true;
+    ssl_socket->CheckIfHandshakeFinished();
+  }
+}
+
 int SSLClientSocketOpenSSL::Init() {
   DCHECK(!ssl_);
   DCHECK(!transport_bio_);
 
   SSLContext* context = SSLContext::GetInstance();
   crypto::OpenSSLErrStackTracer err_tracer(FROM_HERE);
 
   ssl_ = SSL_new(context->ssl_ctx());
   if (!ssl_ || !context->SetClientSocketForSSL(ssl_, this))
     return ERR_UNEXPECTED;
 
   if (!SSL_set_tlsext_host_name(ssl_, host_and_port_.host().c_str()))
     return ERR_UNEXPECTED;
 
+  // Set an OpenSSL callback to monitor this SSL*'s connection.
+  SSL_set_info_callback(ssl_, &InfoCallback);
+
   trying_cached_session_ = context->session_cache()->SetSSLSessionWithKey(
       ssl_, GetSessionCacheKey());
 
   BIO* ssl_bio = NULL;
   // 0 => use default buffer sizes.
   if (!BIO_new_bio_pair(&ssl_bio, 0, &transport_bio_, 0))
     return ERR_UNEXPECTED;
   DCHECK(ssl_bio);
   DCHECK(transport_bio_);
 
   // Install a callback on OpenSSL's end to plumb transport errors through.
   BIO_set_callback(ssl_bio, &SSLClientSocketOpenSSL::BIOCallback);

[wtc, 2014/08/07 20:04:03]

Nit: Could you delete "SSLClientSocketOpenSSL::" ?

   BIO_set_callback_arg(ssl_bio, reinterpret_cast<char*>(this));
 
   SSL_set_bio(ssl_, ssl_bio, ssl_bio);
 
   // OpenSSL defaults some options to on, others to off. To avoid ambiguity,
   // set everything we care about to an absolute value.
   SslSetClearMask options;
   options.ConfigureFlag(SSL_OP_NO_SSLv2, true);
   bool ssl3_enabled = (ssl_config_.version_min == SSL_PROTOCOL_VERSION_SSL3);
   options.ConfigureFlag(SSL_OP_NO_SSLv3, !ssl3_enabled);
@@ skipping 299 matching lines @@
       net_log_);
 }
 
 int SSLClientSocketOpenSSL::DoVerifyCertComplete(int result) {
   verifier_.reset();
 
   if (result == OK) {
     // TODO(joth): Work out if we need to remember the intermediate CA certs
     // when the server sends them to us, and do so here.
     SSLContext::GetInstance()->session_cache()->MarkSSLSessionAsGood(ssl_);
+    marked_session_as_good_ = true;
+    CheckIfHandshakeFinished();
   } else {
     DVLOG(1) << "DoVerifyCertComplete error " << ErrorToString(result)
              << " (" << result << ")";
   }
 
   completed_handshake_ = true;
   // Exit DoHandshakeLoop and return the result to the caller to Connect.
   DCHECK_EQ(STATE_NONE, next_handshake_state_);
   return result;
 }
@@ skipping 525 matching lines @@
     // write payload. If the current payload fails to write, the error will be
     // reported in a future write or read to |bio|.
     if (transport_write_error_ != OK) {
       OpenSSLPutNetError(FROM_HERE, transport_write_error_);
       return -1;
     }
   }
   return retvalue;
 }
 
+// Determines if the session for |ssl_| is in the cache, and calls the

[wtc, 2014/08/07 20:04:03]

"the session for |ssl_| is in the cache" is not true if the server returns an
empty session ID in ServerHello. So this comment isn't accurate. I guess we can
say:

  // Determines if both the handshake and certificate verification have
completed successfully, and and calls the handshake completion callback if that
is the case.

+// handshake completion callback if that is the case.
+//
+// CheckIfHandshakeFinished is called twice per connection: once after
+// MarkSSLSessionAsGood, when the certificate has been verified, and
+// once via an OpenSSL callback when the handshake has completed. On the
+// second call, when the certificate has been verified and the handshake
+// has completed, the connection's handshake completion callback is run.
+void SSLClientSocketOpenSSL::CheckIfHandshakeFinished() {

[wtc, 2014/08/07 20:04:03]

Nit: to match the declaration order in the .h file, this method should be
defined after BIOCallback.

+  if (ran_handshake_finished_callback_ && marked_session_as_good_)
+    OnHandshakeCompletion();
+}
+
 // static
 long SSLClientSocketOpenSSL::BIOCallback(
     BIO *bio,
     int cmd,
     const char *argp, int argi, long argl,
     long retvalue) {
   SSLClientSocketOpenSSL* socket = reinterpret_cast<SSLClientSocketOpenSSL*>(
       BIO_get_callback_arg(bio));
   CHECK(socket);
   return socket->MaybeReplayTransportError(
       bio, cmd, argp, argi, argl, retvalue);
 }
 
 scoped_refptr<X509Certificate>
 SSLClientSocketOpenSSL::GetUnverifiedServerCertificateChain() const {
   return server_cert_;
 }
 
 }  // namespace net