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

This CL corrects a bug in which the OnHandshakeComplete callback for an ssl session was never
called if that session had an empty session id (i.e. the session wasn't added to the cache).

This CL sets the OpenSSL info_callback such that CheckIfSessionAdded will be run whenever
an OpenSSL session has finished its handshake. Previously, CheckIfSessionAdded was called in
NewSessionCallbackStatic. NewSessionCallbackStatic is only called when a session is
added to the cache. Thus, leading connections with sessions that were not added to the cache would
force their pending jobs to wait indefinitely instead of letting the jobs proceed at the completion
of the leading job's connection.

R=mmenke@chromium.org,rsleevi@chromium.org,wtc@chromium.org, davidben@chromium.org


BUG=398967
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=288024

[Ryan Sleevi, 2014-07-25 21:42:38]

Adding DavidBen for BoringSSL, although he did steer you this way.

This is what I was mentioning though, in terms of "setting an openssl callback"
to know when things finished. Guess there was a use case for the complexity
after all!

(I think this looks good, but David's probably better to catch if anything was
missed)

[davidben, 2014-07-29 19:16:02]

Sorry about the delay. This lgtm. A couple of comments:

- Is there a bug # to attach this change to?

- This sounds like something that would be worth a test for, either in this CL
or a follow-up. (I'll defer to Ryan there.)

Testing thoughts:

You can look in ssl_client_socket_unittest.cc for some of our existing
SSLClientSocket unit tests. From the email, it sounded like the case you cared
about was full handshakes when the server did not give you a session to cache
(empty session id in ServerHello). I glanced at tlslite (the Python TLS
implementation we test against) and it looks like giving it a sessionCache of
None will do the trick.

This will take a little bit of plumbing to configure it. The test server lives
in net/tools/testserver/testserver.py and takes command-line flags. You'll want
to add a flag that makes it pass a None session cache (doesn't look like we have
such a flag right now). Then you'll want to modify net/test/spawned_test_server/
to have a similar option in BaseTestServer::SSLOptions. And with that you can
write tests that configure the test server in that way. I'd write tests that
connect both to the normal session-caching test server and one with your flag
and, in both cases, make sure that the handshake callback eventually runs.

May also be worth testing interaction with False Start as that will make
Connect() complete before the handshake does. There are some tests for False
Start in that file.

https://codereview.chromium.org/416683002/diff/20001/net/socket/ssl_session_c...
File net/socket/ssl_session_cache_openssl.cc (right):

https://codereview.chromium.org/416683002/diff/20001/net/socket/ssl_session_c...
net/socket/ssl_session_cache_openssl.cc:260: 
The completion count thing is a little non-obvious on first read. Maybe add a
comment like:

// CheckIfSessionFinished is called twice per connection, once in
MarkSSLSessionAsGood, when the certificate has been verified, and once when the
handshake has completed. On the second call, the connection's handshake
completion callback is run.

https://codereview.chromium.org/416683002/diff/20001/net/socket/ssl_session_c...
net/socket/ssl_session_cache_openssl.cc:531: SSLToCallbackMap
ssl_to_callback_map_;
Now that the completion hook comes from SSLClientSocketOpenSSL, instead of
having the callback live in ssl_session_cache_openssl.cc, perhaps the callback
and state could live in SSLClientSocketOpenSSL. Then you don't need the map. So
you'd call CheckIfSessionFinished right after calling MarkSessionAsGood in
DoVerifyComplete and then also call it directly in OnSessionFinishedCallback.
Might be simpler; you wouldn't have to go through this session cache object at
all.

I'll defer to Ryan as to whether that should be done here or in a follow-up.

[Ryan Sleevi, 2014-07-29 23:21:04]

I agree with David on both points, and I think this CL is a good place to fix
them.

[mshelley, 2014-08-05 23:17:11]

Sorry for the delay -- I wanted to get another CL that this CL is dependent upon
committed before fixing this one.

https://codereview.chromium.org/416683002/diff/20001/net/socket/ssl_session_c...
File net/socket/ssl_session_cache_openssl.cc (right):

https://codereview.chromium.org/416683002/diff/20001/net/socket/ssl_session_c...
net/socket/ssl_session_cache_openssl.cc:260: 
On 2014/07/29 19:16:01, David Benjamin wrote:
> The completion count thing is a little non-obvious on first read. Maybe add a
> comment like:
> 
> // CheckIfSessionFinished is called twice per connection, once in
> MarkSSLSessionAsGood, when the certificate has been verified, and once when
the
> handshake has completed. On the second call, the connection's handshake
> completion callback is run.

Done.

https://codereview.chromium.org/416683002/diff/20001/net/socket/ssl_session_c...
net/socket/ssl_session_cache_openssl.cc:531: SSLToCallbackMap
ssl_to_callback_map_;
On 2014/07/29 19:16:01, David Benjamin wrote:
> Now that the completion hook comes from SSLClientSocketOpenSSL, instead of
> having the callback live in ssl_session_cache_openssl.cc, perhaps the callback
> and state could live in SSLClientSocketOpenSSL. Then you don't need the map.
So
> you'd call CheckIfSessionFinished right after calling MarkSessionAsGood in
> DoVerifyComplete and then also call it directly in OnSessionFinishedCallback.
> Might be simpler; you wouldn't have to go through this session cache object at
> all.
> 
> I'll defer to Ryan as to whether that should be done here or in a follow-up.

Done.

[Ryan Sleevi, 2014-08-06 01:31:30]

https://codereview.chromium.org/416683002/diff/120001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_openssl.cc (right):

https://codereview.chromium.org/416683002/diff/120001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_openssl.cc:1594: if (++session_completion_count_ ==
2) {
Is this still necessary / the right approach, now that you're within the socket?

It seems like you have access to all the information you need internally, and
can make this more explicit. For example, rather than this count (which has
implicit state), you could set a bool when the cb was called.

if (cb_called_ &&
SSLContext::GetInstance()->session_cache()->SessionIsGood(ssl_))
  ...

https://codereview.chromium.org/416683002/diff/120001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_openssl.h (right):

https://codereview.chromium.org/416683002/diff/120001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_openssl.h:98: void CheckIfSessionFinished();
This doesn't need to be public, does it? The static method can call this w/o it
needing to be.

https://codereview.chromium.org/416683002/diff/120001/net/test/spawned_test_s...
File net/test/spawned_test_server/base_test_server.h (right):

https://codereview.chromium.org/416683002/diff/120001/net/test/spawned_test_s...
net/test/spawned_test_server/base_test_server.h:208: bool none_session_cache;
This isn't really descriptive in the context of just this header file.

// Whether to enable TLS session caching. When session caching is
// disabled, the server will generate a new Session ID for every connection.
bool enable_session_cache;

(with a default of true)

[mshelley, 2014-08-06 03:09:31]

https://codereview.chromium.org/416683002/diff/120001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_openssl.cc (right):

https://codereview.chromium.org/416683002/diff/120001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_openssl.cc:1594: if (++session_completion_count_ ==
2) {
On 2014/08/06 01:31:30, Ryan Sleevi wrote:
> Is this still necessary / the right approach, now that you're within the
socket?
> 
> It seems like you have access to all the information you need internally, and
> can make this more explicit. For example, rather than this count (which has
> implicit state), you could set a bool when the cb was called.
> 
> if (cb_called_ &&
> SSLContext::GetInstance()->session_cache()->SessionIsGood(ssl_))
>   ...

Done.

https://codereview.chromium.org/416683002/diff/120001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_openssl.h (right):

https://codereview.chromium.org/416683002/diff/120001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_openssl.h:98: void CheckIfSessionFinished();
On 2014/08/06 01:31:30, Ryan Sleevi wrote:
> This doesn't need to be public, does it? The static method can call this w/o
it
> needing to be.

Done.

https://codereview.chromium.org/416683002/diff/120001/net/test/spawned_test_s...
File net/test/spawned_test_server/base_test_server.h (right):

https://codereview.chromium.org/416683002/diff/120001/net/test/spawned_test_s...
net/test/spawned_test_server/base_test_server.h:208: bool none_session_cache;
On 2014/08/06 01:31:30, Ryan Sleevi wrote:
> This isn't really descriptive in the context of just this header file.
> 
> // Whether to enable TLS session caching. When session caching is
> // disabled, the server will generate a new Session ID for every connection.
> bool enable_session_cache;
> 
> (with a default of true)

Done.

[Ryan Sleevi, 2014-08-06 23:42:04]

Mostly LG, but several (hopefully quick/easy) cleanups

https://codereview.chromium.org/416683002/diff/180001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_openssl.h (right):

https://codereview.chromium.org/416683002/diff/180001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_openssl.h:99: void SetSSLHandshakeComplete();
This doesn't need to be a public method

Explanation: OnSessionFinishedCallback is a static member method, which means it
has full access to the SSLClientSocketOpenSSL's members

That is, you could write
SSLClientSocketOpenSSL* socket =
SSLContext::GetInstance()->GetClientSocketFromSSL(ssl)
socket->handshake_completed_ = true;

from OnSessionFinishedCallback

https://codereview.chromium.org/416683002/diff/180001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_unittest.cc (right):

https://codereview.chromium.org/416683002/diff/180001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_unittest.cc:2765: LOG(ERROR) << "SSL Socket
prematurely connected";
1) Why this check? Seems unrelated to what you're testing
2) If an invariant is violated, ASSERT_FALSE(sock->IsConnected())

[I realize it's copied from 2724/2725, but that test is bugged/unnecessary too,
just missed it. Let's fix that in this CL]

https://codereview.chromium.org/416683002/diff/180001/net/socket/ssl_session_...
File net/socket/ssl_session_cache_openssl.cc (right):

https://codereview.chromium.org/416683002/diff/180001/net/socket/ssl_session_...
net/socket/ssl_session_cache_openssl.cc:267: return reinterpret_cast<int64>(
BUG: Why do you cast to int64?

Just return SSL_session_get_ex_data(session, ...)

(pointer -> bool conversion is defined - see line 249-252)

https://codereview.chromium.org/416683002/diff/180001/net/tools/testserver/te...
File net/tools/testserver/testserver.py (right):

https://codereview.chromium.org/416683002/diff/180001/net/tools/testserver/te...
net/tools/testserver/testserver.py:209: self.session_cache = None
This doesn't seem to reflect your documentation

Seems like

if none_session_cache:
  self.session_cache = None
elif record_resume_info:
  ...

https://codereview.chromium.org/416683002/diff/180001/net/tools/testserver/te...
net/tools/testserver/testserver.py:2095: 'session cache.')
need to update these command-line flags too, to match your variable rename

namely, disable-session-cache

[mshelley, 2014-08-07 00:22:00]

https://codereview.chromium.org/416683002/diff/180001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_openssl.h (right):

https://codereview.chromium.org/416683002/diff/180001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_openssl.h:99: void SetSSLHandshakeComplete();
On 2014/08/06 23:42:03, Ryan Sleevi wrote:
> This doesn't need to be a public method
> 
> Explanation: OnSessionFinishedCallback is a static member method, which means
it
> has full access to the SSLClientSocketOpenSSL's members
> 
> That is, you could write
> SSLClientSocketOpenSSL* socket =
> SSLContext::GetInstance()->GetClientSocketFromSSL(ssl)
> socket->handshake_completed_ = true;
> 
> from OnSessionFinishedCallback

Done.

https://codereview.chromium.org/416683002/diff/180001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_unittest.cc (right):

https://codereview.chromium.org/416683002/diff/180001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_unittest.cc:2765: LOG(ERROR) << "SSL Socket
prematurely connected";
On 2014/08/06 23:42:03, Ryan Sleevi wrote:
> 1) Why this check? Seems unrelated to what you're testing
> 2) If an invariant is violated, ASSERT_FALSE(sock->IsConnected())
> 
> [I realize it's copied from 2724/2725, but that test is bugged/unnecessary
too,
> just missed it. Let's fix that in this CL]

Done.

https://codereview.chromium.org/416683002/diff/180001/net/socket/ssl_session_...
File net/socket/ssl_session_cache_openssl.cc (right):

https://codereview.chromium.org/416683002/diff/180001/net/socket/ssl_session_...
net/socket/ssl_session_cache_openssl.cc:267: return reinterpret_cast<int64>(
On 2014/08/06 23:42:03, Ryan Sleevi wrote:
> BUG: Why do you cast to int64?
> 
> Just return SSL_session_get_ex_data(session, ...)
> 
> (pointer -> bool conversion is defined - see line 249-252)

Done.

https://codereview.chromium.org/416683002/diff/180001/net/tools/testserver/te...
File net/tools/testserver/testserver.py (right):

https://codereview.chromium.org/416683002/diff/180001/net/tools/testserver/te...
net/tools/testserver/testserver.py:209: self.session_cache = None
On 2014/08/06 23:42:04, Ryan Sleevi wrote:
> This doesn't seem to reflect your documentation
> 
> Seems like
> 
> if none_session_cache:
>   self.session_cache = None
> elif record_resume_info:
>   ...

Done.

https://codereview.chromium.org/416683002/diff/180001/net/tools/testserver/te...
net/tools/testserver/testserver.py:2095: 'session cache.')
On 2014/08/06 23:42:04, Ryan Sleevi wrote:
> need to update these command-line flags too, to match your variable rename
> 
> namely, disable-session-cache

Done.

[Ryan Sleevi, 2014-08-07 00:27:42]

https://codereview.chromium.org/416683002/diff/200001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_unittest.cc (right):

https://codereview.chromium.org/416683002/diff/200001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_unittest.cc:2725: LOG(ERROR) << "SSL Socket
prematurely connected";
Pst: Nuke these two lines :)

https://codereview.chromium.org/416683002/diff/200001/net/test/spawned_test_s...
File net/test/spawned_test_server/base_test_server.cc (right):

https://codereview.chromium.org/416683002/diff/200001/net/test/spawned_test_s...
net/test/spawned_test_server/base_test_server.cc:482:
arguments->Set("none-session-cache", base::Value::CreateNullValue());
BUG

https://codereview.chromium.org/416683002/diff/200001/net/tools/testserver/te...
File net/tools/testserver/testserver.py (right):

https://codereview.chromium.org/416683002/diff/200001/net/tools/testserver/te...
net/tools/testserver/testserver.py:2096: 'session cache.')
1) Description still needs updating
2) Did you mean to call this --disable-session-cache?

If I set --session-cache, that actually disables it, which doesn't seem right.

[mshelley, 2014-08-07 00:55:05]

https://codereview.chromium.org/416683002/diff/200001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_unittest.cc (right):

https://codereview.chromium.org/416683002/diff/200001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_unittest.cc:2725: LOG(ERROR) << "SSL Socket
prematurely connected";
On 2014/08/07 00:27:41, Ryan Sleevi wrote:
> Pst: Nuke these two lines :)

Done.

https://codereview.chromium.org/416683002/diff/200001/net/test/spawned_test_s...
File net/test/spawned_test_server/base_test_server.cc (right):

https://codereview.chromium.org/416683002/diff/200001/net/test/spawned_test_s...
net/test/spawned_test_server/base_test_server.cc:482:
arguments->Set("none-session-cache", base::Value::CreateNullValue());
On 2014/08/07 00:27:41, Ryan Sleevi wrote:
> BUG

Done.

https://codereview.chromium.org/416683002/diff/200001/net/tools/testserver/te...
File net/tools/testserver/testserver.py (right):

https://codereview.chromium.org/416683002/diff/200001/net/tools/testserver/te...
net/tools/testserver/testserver.py:2096: 'session cache.')
On 2014/08/07 00:27:41, Ryan Sleevi wrote:
> 1) Description still needs updating
> 2) Did you mean to call this --disable-session-cache?
> 
> If I set --session-cache, that actually disables it, which doesn't seem right.

Ahh yes should have been disable, sorry about that.

[Ryan Sleevi, 2014-08-07 01:08:34]

LGTM, mod one last comment nit.

https://codereview.chromium.org/416683002/diff/220001/net/tools/testserver/te...
File net/tools/testserver/testserver.py (right):

https://codereview.chromium.org/416683002/diff/220001/net/tools/testserver/te...
net/tools/testserver/testserver.py:2097: 'session cache.')
'TLS session cache'

[mshelley, 2014-08-07 01:59:23]

The CQ bit was checked by mshelley@chromium.org

[commit-bot: I haz the power, 2014-08-07 02:00:10]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mshelley@chromium.org/416683002/240001

[mshelley, 2014-08-07 02:01:11]

The CQ bit was unchecked by mshelley@chromium.org

[Ryan Sleevi, 2014-08-07 02:06:53]

Oh, also, update description/commit note before committing

eg:

"Ensure OnHandshakeComplete is called if the TLS server sends an empty session
ID"

[Ryan Sleevi, 2014-08-07 02:08:03]

On 2014/08/07 02:06:53, Ryan Sleevi wrote:
> Oh, also, update description/commit note before committing
> 
> eg:
> 
> "Ensure OnHandshakeComplete is called if the TLS server sends an empty session
> ID"

Having the description in the body is fine, but it's good to have a small (< 75
character) explanation, and then dig in

http://git-scm.com/book/ch5-2.html :)

[wtc, 2014-08-07 02:10:13]

Review comments on patch set 5:

1. I believe this CL can be further simplified. See the comment marked with
"IMPORTANT".

2. Nit: I don't like the term "session finished" because it is nonstandard. But
I don't have a good suggestion for an alternative now.

https://codereview.chromium.org/416683002/diff/220001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_openssl.cc (right):

https://codereview.chromium.org/416683002/diff/220001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_openssl.cc:684: int unused) {

The Style Guide recommends commenting out the name of the unused variable, like
this:

  int /*unused*/

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

https://codereview.chromium.org/416683002/diff/220001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_openssl.cc:1587: // Determines if the session for
|ssl| is in the cache, and calls the

|ssl| => |ssl_|

https://codereview.chromium.org/416683002/diff/220001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_openssl.cc:1588: // appropriate callback if that is
the case.

Nit: be more specific than "the appropriate callback".

https://codereview.chromium.org/416683002/diff/220001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_openssl.cc:1595: void
SSLClientSocketOpenSSL::CheckIfSessionFinished() {

This method should be renamed, but I don't have a good suggestion right now.

Maybe "CheckIfHandshakeCompleted"?

https://codereview.chromium.org/416683002/diff/220001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_openssl.cc:1597:
SSLContext::GetInstance()->session_cache()->SessionIsGood(ssl_)) {

IMPORTANT: It seems that we are doing this the hard way.

SessionIsGood() returns true iff we have called MarkSSLSessionAsGood().

Therefore, all we need is a boolean member to remember if we have called
MarkSSLSessionAsGood(), or you can use your "completion count" solution here.

https://codereview.chromium.org/416683002/diff/220001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_openssl.h (right):

https://codereview.chromium.org/416683002/diff/220001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_openssl.h:110: static void
OnSessionFinishedCallback(const SSL* ssl, int result, int unused);

This callback should be named "InfoCallback", and the comment should be updated
accordingly. The info callback is called not just when the handshake is
finished.

https://codereview.chromium.org/416683002/diff/220001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_openssl.h:174: void CheckIfSessionFinished();

SessionFinished => HandshakeFinished

https://codereview.chromium.org/416683002/diff/220001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_openssl.h:276: // True if OnSessionFinishedCallback
has been run.

This comment is wrong. It should say something like:

  // True if OnSessionFinishedCallback has been run with
result=SSL_CB_HANDSHAKE_DONE.

The data member should be renamed accordingly.

https://codereview.chromium.org/416683002/diff/220001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_openssl.h:277: bool ran_session_finished_callback_;

Change "session finished" to "handshake finished" in all comments and identifier
names. "session finished" doesn't make sense.

https://codereview.chromium.org/416683002/diff/220001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_unittest.cc (right):

https://codereview.chromium.org/416683002/diff/220001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_unittest.cc:2733: TEST_F(SSLClientSocketTest,
HandshakeCallbackIsRun_WithNoneSessionCache) {

Nit: what does "none session cache" mean?

https://codereview.chromium.org/416683002/diff/220001/net/socket/ssl_session_...
File net/socket/ssl_session_cache_openssl.cc (left):

https://codereview.chromium.org/416683002/diff/220001/net/socket/ssl_session_...
net/socket/ssl_session_cache_openssl.cc:240: // Return true iff a cached session
was associated with the given |cache_key|.

Did you remove this comment by mistake (a merge/rebase error)?

[mshelley, 2014-08-07 02:57:01]

The CQ bit was checked by mshelley@chromium.org

[commit-bot: I haz the power, 2014-08-07 03:00:58]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mshelley@chromium.org/416683002/280001

[commit-bot: I haz the power, 2014-08-07 10:07:27]

Change committed as 288024

[wtc, 2014-08-07 20:04:05]

Patch set 7 LGTM. Please create a new CL to make the the suggested changes.
Thanks.

https://codereview.chromium.org/416683002/diff/280001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_openssl.cc (right):

https://codereview.chromium.org/416683002/diff/280001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_openssl.cc:685: int /*unused*/) {

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

https://codereview.chromium.org/416683002/diff/280001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_openssl.cc:687:
SSLContext::GetInstance()->GetClientSocketFromSSL(ssl);

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

https://codereview.chromium.org/416683002/diff/280001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_openssl.cc:722: BIO_set_callback(ssl_bio,
&SSLClientSocketOpenSSL::BIOCallback);

Nit: Could you delete "SSLClientSocketOpenSSL::" ?

https://codereview.chromium.org/416683002/diff/280001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_openssl.cc:1589: // Determines if the session for
|ssl_| is in the cache, and calls the

"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.

https://codereview.chromium.org/416683002/diff/280001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_openssl.cc:1597: void
SSLClientSocketOpenSSL::CheckIfHandshakeFinished() {

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

https://codereview.chromium.org/416683002/diff/280001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_openssl.h (right):

https://codereview.chromium.org/416683002/diff/280001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_openssl.h:109: // Callback that is run by OpenSSL
to obtain information about the

obtain => "report" or "provide"

Alternatively, say:

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

https://codereview.chromium.org/416683002/diff/280001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_openssl.h:111: static void InfoCallback(const SSL*
ssl, int result, int unused);

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).

https://codereview.chromium.org/416683002/diff/280001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_openssl.h:170: static long BIOCallback(BIO *bio,

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.

https://codereview.chromium.org/416683002/diff/280001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_openssl.h:218: bool completed_handshake_;

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_;

https://codereview.chromium.org/416683002/diff/280001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_openssl.h:278: bool
ran_handshake_finished_callback_;

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"?

https://codereview.chromium.org/416683002/diff/280001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_openssl.h:280: // connection's SSL session.

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

https://codereview.chromium.org/416683002/diff/280001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_unittest.cc (left):

https://codereview.chromium.org/416683002/diff/280001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_unittest.cc:2725: LOG(ERROR) << "SSL Socket
prematurely connected";

Did you mean to delete this? (I agree it's not useful).

https://codereview.chromium.org/416683002/diff/280001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_unittest.cc (right):

https://codereview.chromium.org/416683002/diff/280001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_unittest.cc:2697: SpawnedTestServer::SSLOptions
ssl_options;

Delete this line. The |ssl_options| local variable is not used.

https://codereview.chromium.org/416683002/diff/280001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_unittest.cc:2732: // that do not cache their
session.

Nit: this should point out it is the server that doesn't cache sessions.
Something like:

  // Tests that the completion callback is run with a server that doesn't cache
sessions.

https://codereview.chromium.org/416683002/diff/280001/net/socket/ssl_session_...
File net/socket/ssl_session_cache_openssl.cc (right):

https://codereview.chromium.org/416683002/diff/280001/net/socket/ssl_session_...
net/socket/ssl_session_cache_openssl.cc:13: #include "base/callback.h"

Delete this.

https://codereview.chromium.org/416683002/diff/280001/net/socket/ssl_session_...
File net/socket/ssl_session_cache_openssl.h (right):

https://codereview.chromium.org/416683002/diff/280001/net/socket/ssl_session_...
net/socket/ssl_session_cache_openssl.h:11: #include "base/callback_forward.h"

Delete this.

https://codereview.chromium.org/416683002/diff/280001/net/test/spawned_test_s...
File net/test/spawned_test_server/base_test_server.h (right):

https://codereview.chromium.org/416683002/diff/280001/net/test/spawned_test_s...
net/test/spawned_test_server/base_test_server.h:208: // disabled, the server
will generate a new Session ID for every connection.

Hmm... "generate a new session ID" is not the behavior we want to test. What we
want to test is that the server returns an empty session ID in ServerHello.

I checked the tlslite/tls_connection.py code. It seems that if 'sessionCache' is
'None', it generates an empty session ID for ServerHello:

        # Create the ServerHello message
        if sessionCache:
            sessionID = getRandomBytes(32)
        else:
            sessionID = bytearray(0)

So I think this comment should be corrected.