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

net/socket/ssl_client_socket_openssl.h

 // 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.
 
 #ifndef NET_SOCKET_SSL_CLIENT_SOCKET_OPENSSL_H_
 #define NET_SOCKET_SSL_CLIENT_SOCKET_OPENSSL_H_
 
 #include <string>
 
 #include "base/compiler_specific.h"
@@ skipping 88 matching lines @@
   // SSLClientSocket implementation.
   virtual scoped_refptr<X509Certificate> GetUnverifiedServerCertificateChain()
       const OVERRIDE;
 
  private:
   class PeerCertificateChain;
   class SSLContext;
   friend class SSLClientSocket;
   friend class SSLContext;
 
+  // Callback that is run by OpenSSL to obtain information about the

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

obtain => "report" or "provide"

Alternatively, say:

  // Callback that is used to obtain information about the state of the SSL
handshake.

+  // state of the SSL handshake.
+  static void 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". These parameter names come from the OpenSSL source code.

Note: The OpenSSL man page uses different parameter names:
https://www.openssl.org/docs/ssl/SSL_CTX_set_info_callback.html

  The callback function is called as callback(SSL *ssl, int where, int ret).

+
   int Init();
   void DoReadCallback(int result);
   void DoWriteCallback(int result);
 
   // Compute a unique key string for the SSL session cache.
   std::string GetSessionCacheKey() const;
   void OnHandshakeCompletion();
 
   bool DoTransportIO();
   int DoHandshake();
@@ skipping 37 matching lines @@
   // Called during an operation on |transport_bio_|'s peer. Checks saved
   // transport error state and, if appropriate, returns an error through
   // OpenSSL's error system.
   long MaybeReplayTransportError(BIO *bio,
                                  int cmd,
                                  const char *argp, int argi, long argl,
                                  long retvalue);
 
   // Callback from the SSL layer when an operation is performed on
   // |transport_bio_|'s peer.
   static long BIOCallback(BIO *bio,

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

Nit: I suggest moving the InfoCallback function here, so that our OpenSSL
callback functions are declared together. Make the corresponding change to the
.cc file.

                           int cmd,
                           const char *argp, int argi, long argl,
                           long retvalue);
 
+  void CheckIfHandshakeFinished();
+
   bool transport_send_busy_;
   bool transport_recv_busy_;
 
   scoped_refptr<DrainableIOBuffer> send_buffer_;
   scoped_refptr<IOBuffer> recv_buffer_;
 
   CompletionCallback user_connect_callback_;
   CompletionCallback user_read_callback_;
   CompletionCallback user_write_callback_;
 
@@ skipping 21 matching lines @@
   int transport_read_error_;
 
   // Used by TransportWriteComplete() and TransportReadComplete() to signify an
   // error writing to the transport socket. A value of OK indicates no error.
   int transport_write_error_;
 
   // Set when handshake finishes.
   scoped_ptr<PeerCertificateChain> server_cert_chain_;
   scoped_refptr<X509Certificate> server_cert_;
   CertVerifyResult server_cert_verify_result_;
   bool completed_handshake_;

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

Hmmm... I wonder if the comment on line 214 and this bool member should be
updated to avoid confusion with the new handshake finished code.

I suggest changing the comment to say:

  // Set when Connect finishes.

and renaming the bool member

  bool completed_connect_;

 
   // Set when Read() or Write() successfully reads or writes data to or from the
   // network.
   bool was_ever_used_;
 
   // Stores client authentication information between ClientAuthHandler and
   // GetSSLCertRequestInfo calls.
   bool client_auth_cert_needed_;
   // List of DER-encoded X.509 DistinguishedName of certificate authorities
   // allowed by the server.
@@ skipping 38 matching lines @@
     STATE_VERIFY_CERT_COMPLETE,
   };
   State next_handshake_state_;
   NextProtoStatus npn_status_;
   std::string npn_proto_;
   // Written by the |channel_id_service_|.
   std::string channel_id_private_key_;
   std::string channel_id_cert_;
   // True if channel ID extension was negotiated.
   bool channel_id_xtn_negotiated_;
+  // True if InfoCallback has been run with result = SSL_CB_HANDSHAKE_DONE.
+  bool ran_handshake_finished_callback_;

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

Nit: this data member name is a little confusing because the callback is now
named "InfoCallback".

We should rename this data member, but I can't come up with a good name. Perhaps
"handshake_succeeded"?

+  // True if MarkSSLSessionAsGood has been called for this socket's
+  // connection's SSL session.

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

Nit: shorten "this socket's connection's SSL session" to "this socket's SSL
session".

+  bool marked_session_as_good_;
   // The request handle for |channel_id_service_|.
   ChannelIDService::RequestHandle channel_id_request_handle_;
   BoundNetLog net_log_;
 };
 
 }  // namespace net
 
 #endif  // NET_SOCKET_SSL_CLIENT_SOCKET_OPENSSL_H_