Unwind the SSL connection holdback experiment and remove related code

Unwind the SSL connection holdback experiment and remove related code

BUG=398967, 464147, 461882, 451081
TBR=jyasskin

Committed: https://crrev.com/f020edc83428cc51863ecde069803c78f0cf307e
Cr-Commit-Position: refs/heads/master@{#320774}

[Ryan Sleevi, 2015-03-05 22:28:32]

rsleevi@chromium.org changed reviewers:
+ davidben@chromium.org

[Ryan Sleevi, 2015-03-05 22:28:33]

David: Please pay extra close attention to this CL when reviewing. This is the
initial bit, sans git cl format (to make reviewing hopefully easier)

I want to make sure I didn't revert anything extra that shouldn't have been

[davidben, 2015-03-09 18:02:37]

There's a few spurious formatting-only changes. I didn't bother commenting on
them all under the assumption that clang-format will normalize them.

https://codereview.chromium.org/981723008/diff/1/net/socket/socket_test_util.cc
File net/socket/socket_test_util.cc (left):

https://codereview.chromium.org/981723008/diff/1/net/socket/socket_test_util....
net/socket/socket_test_util.cc:1335: transport_socket->socket()->NetLog()),
I think the RHS is correct, right? (Eh, git cl format will have an opinion.)

https://codereview.chromium.org/981723008/diff/1/net/socket/socket_test_util.cc
File net/socket/socket_test_util.cc (right):

https://codereview.chromium.org/981723008/diff/1/net/socket/socket_test_util....
net/socket/socket_test_util.cc:1353: base::Bind(&ConnectCallback,
base::Unretained(this), callback));
I'm slightly confused about what happened here. The LHS never calls Connect on
the transport, as far as I can tell. Oversight when adding the other features?

The WeakPtr is probably worth retaining (hah). I'm guessing some test crashed if
the MockSSLClientSocket was destroyed before Connect completed.

https://codereview.chromium.org/981723008/diff/1/net/socket/socket_test_util.h
File net/socket/socket_test_util.h (left):

https://codereview.chromium.org/981723008/diff/1/net/socket/socket_test_util....
net/socket/socket_test_util.h:738: private:
Probably want to keep that line.

https://codereview.chromium.org/981723008/diff/1/net/socket/ssl_client_socket.h
File net/socket/ssl_client_socket.h (right):

https://codereview.chromium.org/981723008/diff/1/net/socket/ssl_client_socket...
net/socket/ssl_client_socket.h:21: class CTVerifier;
Should stay after ChannelIDService.

https://codereview.chromium.org/981723008/diff/1/net/socket/ssl_client_socket...
File net/socket/ssl_client_socket_openssl.cc (left):

https://codereview.chromium.org/981723008/diff/1/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_openssl.cc:407: // fallback connections to use a
separate session cache.
GetSocketSessionCacheKey got lost but, when you bring it back (or perhaps leave
it as a private method?), this needs to stay there.

https://codereview.chromium.org/981723008/diff/1/net/socket/ssl_client_socket...
File net/socket/ssl_client_socket_openssl.cc (right):

https://codereview.chromium.org/981723008/diff/1/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_openssl.cc:222: return
GetSocketSessionCacheKey(*socket);
I think you forgot to restore this function.

https://codereview.chromium.org/981723008/diff/1/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_openssl.cc:1529: 
Spurious? (It was probably a spurious change before too. *shrug*)

https://codereview.chromium.org/981723008/diff/1/net/socket/ssl_client_socket...
File net/socket/ssl_client_socket_openssl.h (left):

https://codereview.chromium.org/981723008/diff/1/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_openssl.h:303: bool marked_session_as_good_;
Fine to remove this now, but my session cache CL will probably bring some form
of it back. I think the verify + false start resolving should happen in
SSLClientSocketOpenSSL rather than in the session cache; the session cache will
just have an Insert method and it'll be the caller's responsibility to not
insert a session until both handshake and verification are done. Interface is
simpler that way.

(Also, I doubt we can actually change it in BoringSSL, but, in the False Start
case, I don't like SSL_get_session being called before the handshake is
completely finished. It gets into weirdnesses of, if there's a pending renego,
should SSL_get_session return the session we'd currently using or the session
for the current connection. With session tickets, you're even accessing the
SSL_SESSION before the ticket's filled in.)

https://codereview.chromium.org/981723008/diff/1/net/socket/ssl_client_socket...
File net/socket/ssl_client_socket_openssl.h (right):

https://codereview.chromium.org/981723008/diff/1/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_openssl.h:224: bool completed_handshake_;
I think the LHS naming is actually more accurate in the face of False Start.
(Maybe even be worth an explanatory comment. Or we can leave that for another
CL.)

https://codereview.chromium.org/981723008/diff/1/net/socket/ssl_client_socket...
File net/socket/ssl_client_socket_pool.h (right):

https://codereview.chromium.org/981723008/diff/1/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_pool.h:166: CompletionCallback callback_;
FYI, this part of the revert is just a rename. (I'm fine with LHS's or RHS's
name, but wanted to point it out in case you didn't notice and have an opinion.)

https://codereview.chromium.org/981723008/diff/1/net/socket/ssl_session_cache...
File net/socket/ssl_session_cache_openssl.cc (right):

https://codereview.chromium.org/981723008/diff/1/net/socket/ssl_session_cache...
net/socket/ssl_session_cache_openssl.cc:264: return;
(I think the CHECK is actually correct too, but I'm going to be sending you a CL
to change all this later anyway.)

[Ryan Sleevi, 2015-03-09 21:23:56]

With updates, but still sans-formating to make it easier

https://codereview.chromium.org/981723008/diff/1/net/socket/socket_test_util.cc
File net/socket/socket_test_util.cc (right):

https://codereview.chromium.org/981723008/diff/1/net/socket/socket_test_util....
net/socket/socket_test_util.cc:1353: base::Bind(&ConnectCallback,
base::Unretained(this), callback));
On 2015/03/09 18:02:36, David Benjamin wrote:
> I'm slightly confused about what happened here. The LHS never calls Connect on
> the transport, as far as I can tell. Oversight when adding the other features?
> 
> The WeakPtr is probably worth retaining (hah). I'm guessing some test crashed
if
> the MockSSLClientSocket was destroyed before Connect completed.

If it does, then the underlying transport isn't adhering to the mock contract
(it's legal to delete while pending)

[davidben, 2015-03-11 01:05:13]

lgtm modulo git cl format and comment about completed_handshake_ vs
completed_connect_

https://codereview.chromium.org/981723008/diff/1/net/socket/socket_test_util.cc
File net/socket/socket_test_util.cc (right):

https://codereview.chromium.org/981723008/diff/1/net/socket/socket_test_util....
net/socket/socket_test_util.cc:1353: base::Bind(&ConnectCallback,
base::Unretained(this), callback));
On 2015/03/09 21:23:56, Ryan Sleevi wrote:
> On 2015/03/09 18:02:36, David Benjamin wrote:
> > I'm slightly confused about what happened here. The LHS never calls Connect
on
> > the transport, as far as I can tell. Oversight when adding the other
features?
> > 
> > The WeakPtr is probably worth retaining (hah). I'm guessing some test
crashed
> if
> > the MockSSLClientSocket was destroyed before Connect completed.
> 
> If it does, then the underlying transport isn't adhering to the mock contract
> (it's legal to delete while pending)

Acknowledged.

https://codereview.chromium.org/981723008/diff/40001/net/socket/ssl_client_so...
File net/socket/ssl_client_socket_openssl.h (left):

https://codereview.chromium.org/981723008/diff/40001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_openssl.h:231: // Set when Connect finishes.
Did you prefer to keep the name change reverted? (The trouble with
completed_handshake_ is that it's not accurate. This currently gets set when
Connect() finishes. If we False Start, the handshake doesn't complete until
later.)

[Ryan Sleevi, 2015-03-16 18:35:05]

rsleevi@chromium.org changed reviewers:
+ jyasskin@chromium.org

[Ryan Sleevi, 2015-03-16 18:35:25]

TBR jyasskin (since it's just removing some mock methods that appear in the base
class)

[Ryan Sleevi, 2015-03-16 18:36:53]

The CQ bit was checked by rsleevi@chromium.org

[Ryan Sleevi, 2015-03-16 18:36:54]

The patchset sent to the CQ was uploaded after l-g-t-m from
davidben@chromium.org
Link to the patchset: https://codereview.chromium.org/981723008/#ps80001 (title:
"Rename & reformat")

[commit-bot: I haz the power, 2015-03-16 18:37:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/981723008/80001

[commit-bot: I haz the power, 2015-03-16 19:31:49]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-03-16 19:32:22]

Patchset 5 (id:??) landed as
https://crrev.com/f020edc83428cc51863ecde069803c78f0cf307e
Cr-Commit-Position: refs/heads/master@{#320774}