Implements new, more robust design for communicating between SSLConnectJobs.

net/socket/ssl_client_socket.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_H_
 #define NET_SOCKET_SSL_CLIENT_SOCKET_H_
 
 #include <string>
 
 #include "base/gtest_prod_util.h"
@@ skipping 63 matching lines @@
     kNextProtoUnsupported = 0,  // The server doesn't support NPN.
     kNextProtoNegotiated = 1,   // We agreed on a protocol.
     kNextProtoNoOverlap = 2,    // No protocols in common. We requested
                                 // the first protocol in our list.
   };
 
   // StreamSocket:
   virtual bool WasNpnNegotiated() const OVERRIDE;
   virtual NextProto GetNegotiatedProtocol() const OVERRIDE;
 
+  // Format a unique key for the SSL session cache. This method

[wtc, 2014/07/11 00:48:53]

Nit: Format => Formats

See the Style Guide:

  These comments should be descriptive ("Opens the file") rather than
  imperative ("Open the file") ...

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Comments

Please check the other function comments in this CL.

[mshelley, 2014/07/11 23:26:26]

Done.

+  // is necessary so that all classes create cache keys in a consistent
+  // manner.
+  static std::string FormatSessionCacheKey(std::string host_and_port,

[wtc, 2014/07/11 00:48:53]

IMPORTANT: this method should ideally be a virtual method of SSLClientSocket or
ClientSocketFactory because the session cache key depends on the implementation
of the SSLClientSocket.

If this method is a static method of SSLClientSocket, it implies all subclasses
of SSLClientSocket should use this method. In that case, we should also modify
SSLClientSocketNSS to use this method. The code is in
SSLClientSocketNSS::InitializeSSLPeerName.

[mshelley, 2014/07/11 23:26:26]

So basically I added this method because I needed to create a session cache key
outside of the SSLCLientSocket
so that I could index the messenger_map_ by cache key. I figured that creating
this static function would standardize cache key creation between the connect
job factory and the client socket. 

However, I've realized that after my changes in
https://codereview.chromium.org/384873002/ I will no longer need to do this,
because I don't add messengers to messenger_map_ until after the socket is
created. 

As a result, I plan to remove this method entirely in my next patch to that CL.

+                                           std::string ssl_session_cache_shard);

[wtc, 2014/07/11 00:48:53]

These two std::string input parameters should be const references to avoid
copying.

[mshelley, 2014/07/11 23:26:26]

Done.

+
+  // Returns true if there is a cache entry in the ssl session cache
+  // for the given cache key.

[wtc, 2014/07/11 00:48:53]

Nit: the given cache key => the cache key of the SSL socket

[mshelley, 2014/07/11 23:26:26]

Done.

+  //
+  // The cache key consists of a host and port concatenated with a session
+  // cache shard.

[wtc, 2014/07/11 00:48:54]

Please point out that these two pieces of info are passed to the constructor of
most SSLClientSocket subclasses (except for the mocks).

[mshelley, 2014/07/11 23:26:26]

Done.

+  virtual bool InSessionCache() const = 0;
+
+  // Tells the session cache to monitor this socket's session and inform
+  // its SSLConnectJobMessenger upon the session's connection's completion.
+  virtual void WatchSessionForCompletion(const base::Closure& callback) = 0;
+
+  // Sets the callback to be used if the socket's connection should fail.
+  virtual void SetSocketFailureCallback(const base::Closure& callback) = 0;

[wtc, 2014/07/11 00:48:53]

IMPORTANT: WatchSessionForCompletion and SetSocketFailureCallback are similar
methods: both of them set a callback function. So their names should suggest
this similarity. I suggest renaming WatchSessionForCompletion to
SetHandshakeSuccessCallback or SetSessionAddedCallback.

I think SetSocketFailureCallback should be renamed SetHandshakeFailureCallback
because "socket failure" is broader than what this callback is for.

Note: I suggest "Handshake" instead of "Connect" in the method names because
Connect() returns prematurely when False Start is used. Your comments should
also point out this issue.

[mshelley, 2014/07/11 23:26:26]

Done.

+
+  // Marks this socket as the leading connection.
+  virtual void SetIsLeader() = 0;

[wtc, 2014/07/11 00:48:53]

IMPORTANT: Remove this method. It is not necessary. (SSLClientSocket is also the
wrong place for this method, but we can simply remove this method.)

The three functions WatchSessionForCompletion, SetSocketFailureCallback, and
SetIsLeader are always called together. Therefore, in the SSLClientSocketOpenSSL
class, the error_callback_ and is_leader_ members are set together and cleared
together. So you just need the error_callback_ member.

[mshelley, 2014/07/11 23:26:26]

Done.

+
+  // In the case of a failed connection, run the appropriate callback.
+  virtual void OnSocketFailure() = 0;

[wtc, 2014/07/11 00:48:53]

IMPORTANT: Remove OnSocketFailure from this header. OnSocketFailure is only used
internal to SSLClientSocketOpenSSL, so OnSocketFailure should be a private,
non-virtual method of SSLClientSocketOpenSSL.

[mshelley, 2014/07/11 23:26:26]

Done.

+
   // Gets the SSL CertificateRequest info of the socket after Connect failed
   // with ERR_SSL_CLIENT_AUTH_CERT_NEEDED.
   virtual void GetSSLCertRequestInfo(
       SSLCertRequestInfo* cert_request_info) = 0;
 
   // Get the application level protocol that we negotiated with the server.
   // *proto is set to the resulting protocol (n.b. that the string may have
   // embedded NULs).
   //   kNextProtoUnsupported: *proto is cleared.
   //   kNextProtoNegotiated:  *proto is set to the negotiated protocol.
@@ skipping 89 matching lines @@
   bool channel_id_sent_;
   // True if SCTs were received via a TLS extension.
   bool signed_cert_timestamps_received_;
   // True if a stapled OCSP response was received.
   bool stapled_ocsp_response_received_;
 };
 
 }  // namespace net
 
 #endif  // NET_SOCKET_SSL_CLIENT_SOCKET_H_