This CL adds unit tests for false start connections.

This CL adds unit tests for false start connections.

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

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

[Ryan Sleevi, 2014-08-06 01:23:28]

1) Bug #
2) Are there CLs this depends on? Is everything here dependent upon tip of tree?
Your Base URL suggests no.

[mshelley, 2014-08-06 01:40:50]

On 2014/08/06 01:23:28, Ryan Sleevi wrote:
> 1) Bug #
> 2) Are there CLs this depends on? Is everything here dependent upon tip of
tree?
> Your Base URL suggests no.

That's correct; this CL does not depend on any other CLs.

[Ryan Sleevi, 2014-08-06 01:44:10]

Redirecting this particular CL to davidben.

[davidben, 2014-08-06 19:12:47]

https://codereview.chromium.org/446623004/diff/20001/net/socket/ssl_client_so...
File net/socket/ssl_client_socket_unittest.cc (right):

https://codereview.chromium.org/446623004/diff/20001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_unittest.cc:2787: TEST_F(SSLClientSocketTest,
HandshakeCallbackIsRun_WithFalseStartFailure) {
Hrm. I'm not sure this test actually does anything different from
HandshakeCallbackIsRun_WithFailure. It's causing some[*] client write to fail,
but that's before false start kicks in[**].

The one case I can think of where False Start would affect your callback is if
the server's second leg failed to be read or was invalid somehow. I believe you
want to SetNextReadError right before
CreateAndConnectUntilServerFinishedReceived and then UnblockReadResult. Might
need to tweak CreateAndConnectUntilServerFinishedReceived to let you pass in a
StreamSocket parameter to wrap.

At the SSLClientSocket level, this would manifest itself as a Read() failure
rather than a Connect() failure. (Connect would actually succeed.)


[*] Transport writes and especially their errors in SSLClientSocket are
unfortunately really weird. So, the SetNextWriteError call causes the first
transport write to fail, which would be the ClientHello. So that all works out,
but we have a write buffer between TCPClientSocket and OpenSSL's state machine
for API reasons. And so OpenSSL will actually think the first write succeeded
and won't notice until the first write after we flush the write buffer. This
isn't actually a problem for HandshakeCallbackIsRun_WithFailure in so far as
you're guaranteed that'll happen before the client's second leg, so the
handshake will fail. Just a note that SetNextWriteError doesn't actually do the
obvious thing.

[**] False Start, if you're not familiar, makes the client, under some
circumstances, treat the handshake as completed after it has sent its second leg
(ClientKeyExchange, ChangeCipherSpec, Finished) and start sending application
data before waiting for the server's second leg (ChangeCipherSpec, Finished).

[mshelley, 2014-08-07 01:55:48]

Note: I confirmed in the debugger that int this test OnHandshakeCompletion is
called by the socket from DoReadCallback, as we would expect when a handshake
fails during false start.

[davidben, 2014-08-07 21:24:19]

Thanks! That's exactly the point I was thinking of for injecting the error.

https://codereview.chromium.org/446623004/diff/60001/net/socket/ssl_client_so...
File net/socket/ssl_client_socket_unittest.cc (right):

https://codereview.chromium.org/446623004/diff/60001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_unittest.cc:824: scoped_ptr<StreamSocket>
real_transport,
Nit: Switch the order of out_raw_transport and real_transport. (So that the
input parameters all come before the output ones.)

https://codereview.chromium.org/446623004/diff/60001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_unittest.cc:860: if (expect_false_start_error_) {
So this is a little odd in that, if |expect_false_start_error_| is true, it
assumes that |real_transport| is a SynchronousErrorStreamSocket rather than a
generic StreamSocket. I was actually thinking you'd call SetNextReadError
outside of CreateAndConnectUntilServerFinishedReceived (Aaah that function name.
I should have picked something shorter. :-) ).

But now that I look at UnblockWrite, that probably wouldn't work. Sorry, my
fault. I did not realize that, by calling UnblockWrite, we'd likely immediately
run the OpenSSL state machine (by virtue of running the callback on line 499),
causing SSLClientSocket to call Read no the transport earlier.

Option 1: I suspect if you change that line to

  // Defer running the callback by an event loop iteration so the
  // caller has a chance to affect the next read.
  base::MessageLoop::current()->PostTask(
      FROM_HERE, base::Bind(
          base::ResetAndReturn(&pending_write_callback_), rv));

it wouldn't break other tests and you could move the SetNextReadError call to
TestFalseStart...

Option 2: ...but I'm fine with just having
CreateAndConnectUntilServerFinishedReceived also handle the false start error
case. (I.e. disregard my suggestion of passing real_transport in as a parameter.
I didn't realize UnblockWrite worked that way.) In that case you can rename
expect_false_start_error_ to maybe fail_handshake_after_false_start_, always
wrap real_transport in a SynchronousErrorStreamSocket before wrapping in a
FakeBlockingStreamSocket, and then do all your SetNextReadError logic in there.
Then you wouldn't need to change the other tests.

[davidben, 2014-08-08 17:53:35]

lgtm, thanks! Sorry about churn.

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

https://codereview.chromium.org/446623004/diff/120001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_unittest.cc:828: scoped_ptr<StreamSocket>
real_transport = scoped_ptr<StreamSocket>(
Nit: You should just be able to write
   scoped_ptr<StreamSocket> real_transport(
as before.

https://codereview.chromium.org/446623004/diff/120001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_unittest.cc:2920: scoped_ptr<StreamSocket>
real_transport(
Nit: This line is no longer needed.

[mshelley, 2014-08-11 23:59:33]

The CQ bit was checked by mshelley@chromium.org

[commit-bot: I haz the power, 2014-08-12 00:06:48]

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

[commit-bot: I haz the power, 2014-08-12 14:30:40]

Change committed as 288955