Add False Start tests

Add False Start tests

Tests both that False Start occurs when it should and doesn't
when it shouldn't.

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

[davidben, 2014-03-31 15:53:53]

Context for domino chain: https://crbug.com/354132#c1

Sending this out for review ahead of time, but note that it applies on top of
all the tlslite changes. Until the prerequisite CLs get in, there won't be try
coverage. So feel free to wait until then for the final review.

[Ryan Sleevi, 2014-03-31 20:47:55]

Could you please describe/annotate a bit more about how you came to this design?
It's not at all obvious from your description.

For example, why add WaitFor*, when TestCompletionCallback has
callback.GetResult(), which already accomplishes this? Why get rid of the
BlockingState shared logic - how does the read/write differ substantially now?

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

https://codereview.chromium.org/208293002/diff/60001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_unittest.cc:344: void WaitForRead();
Red flag: "WaitFor"* anything, with a RunLoop, is usually a read-flag. It seems
like you dropped the BlockingState purely to accomodate this? If so, why?

[davidben, 2014-03-31 22:15:00]

On 2014/03/31 20:47:55, Ryan Sleevi wrote:
> Could you please describe/annotate a bit more about how you came to this
design?
> It's not at all obvious from your description.
> 
> For example, why add WaitFor*, when TestCompletionCallback has
> callback.GetResult(), which already accomplishes this? Why get rid of the
> BlockingState shared logic - how does the read/write differ substantially now?
> 
>
https://codereview.chromium.org/208293002/diff/60001/net/socket/ssl_client_so...
> File net/socket/ssl_client_socket_unittest.cc (right):
> 
>
https://codereview.chromium.org/208293002/diff/60001/net/socket/ssl_client_so...
> net/socket/ssl_client_socket_unittest.cc:344: void WaitForRead();
> Red flag: "WaitFor"* anything, with a RunLoop, is usually a read-flag. It
seems
> like you dropped the BlockingState purely to accomodate this? If so, why?

GetResult() on the TestCompletionCallback would wait for the entire connect to
succeed. The goal of that mess is to get to the point we can block reads of the
second leg of the server's handshake. Then it finally does a GetResult to wait
for the entire handshake to succeed. Doing that reliably was a little screwy. It
can't just SetNextReadWouldBlock at the beginning since that would block the
server's first leg. I need to SetNextReadWouldBlock sometime between the first
two legs.

I also didn't want to assume any of the legs occurred in a single read or write.
So, instead, it relies on the server's first leg is not sent until the client's
first leg is done, etc. So it steps through each leg by blocking the next. The
point when SetNextReadWouldBlock will reliably catch the server's second leg is
right before it receives the client's second leg (the client second leg getting
sent over the channel means the server's first leg is done, and stopping before
the client's second leg reach's the server means the server can't have sent its
second leg yet).

The reason I had to get rid of BlockingState is because this makes the blocking
mechanism for Read and Write different. Read performs the actual read and blocks
returning the data to the application (equivalent to how BlockingState works).
Write blocks actually sending the write over to the network. In some sense, this
makes the two halves more similar. Both reads and writes are blocked at a point
where you know the data has been sent but not yet received.

Blocking only acknowledgement of the Write (as BlockingState did) means the data
has already reached the test server, so it may have already produced a reply.
And since we have this buffer between the SSL implementation and the transport,
DoTransportIO will have read the reply and OpenSSL/NSS will consume it, so we
haven't meaningfully blocked anything relating to the write in question. We've
blocked some future write when the buffer finally gets full. (My first iteration
added waits to BlockingState and it didn't work because of this.)

[wtc, 2014-04-02 19:26:55]

Patch set 4 LGTM. Please wait for Ryan's approval.

I didn't review net/socket/ssl_client_socket_unittest.cc. I am satisfied with
the changes to the other files.

https://codereview.chromium.org/208293002/diff/60001/net/test/spawned_test_se...
File net/test/spawned_test_server/base_test_server.h (right):

https://codereview.chromium.org/208293002/diff/60001/net/test/spawned_test_se...
net/test/spawned_test_server/base_test_server.h:79: // is acceptable. Preferred
over explicitly OR-ing all ciphers.

Typo: ciphers => key exchange algorithms

https://codereview.chromium.org/208293002/diff/60001/net/test/spawned_test_se...
net/test/spawned_test_server/base_test_server.h:83: KEY_EXCHANGE_DHE_RSA = (1 <<
1),

The test server actually supports more key exchange algorithms than just these
two. Should the other supported key exchange algorithms also be exposed here?

https://codereview.chromium.org/208293002/diff/60001/net/test/spawned_test_se...
net/test/spawned_test_server/base_test_server.h:186: bool support_npn;

Nit: support_npn => enable_npn?

https://codereview.chromium.org/208293002/diff/60001/net/tools/testserver/tes...
File net/tools/testserver/testserver.py (right):

https://codereview.chromium.org/208293002/diff/60001/net/tools/testserver/tes...
net/tools/testserver/testserver.py:166: self.ssl_client_cas = []

Nit: you may want to set self.next_protos = next_protos here, to match the order
of the constructor arguments.

https://codereview.chromium.org/208293002/diff/60001/net/tools/testserver/tes...
net/tools/testserver/testserver.py:2187: help='Specify the next proto value(s)
to '

Question: how are multiple values separated. By spaces or commas?

Nit: it would be good to give an example of a value for this option. Also, if
the test server doesn't support SPDY, then the only supported value is
"http/1.1", right?

https://codereview.chromium.org/208293002/diff/60001/net/tools/testserver/tes...
net/tools/testserver/testserver.py:2189: 'NPN will not be used used. This option
'

Nit: used used => used

https://codereview.chromium.org/208293002/diff/60001/net/tools/testserver/tes...
net/tools/testserver/testserver.py:2191: 'the list of preferred protos.')

Nit: I don't understand why you said "preferred". Are you implying the order of
multiple instances of this option match the order of preference?

[Ryan Sleevi, 2014-04-02 22:09:13]

LGTM. I focused primarily on the unit test, which is quite excellent.

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

https://codereview.chromium.org/208293002/diff/60001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_unittest.cc:477: if (!pending_write_buf_)
comment nit: Can you add a comment about what this condition means

// Do nothing if UnblockWrite() was called after SetNextWriteShouldBlock(),
without a Write() in between.

https://codereview.chromium.org/208293002/diff/60001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_unittest.cc:663: // up in a write buffer or if it
Incomplete comment?

https://codereview.chromium.org/208293002/diff/60001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_unittest.cc:668: // Because of False Start, the
handshake should complete.
comment nit: Expand this comment

// When False Starting, the handshake should complete before receiving the
Change Cipher Spec and Finished messages.

https://codereview.chromium.org/208293002/diff/60001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_unittest.cc:685: // The read will hang.
comment nit: Explain why the read will hang (eg: it's waiting for the peer to
complete the handshake, which is still blocked)

https://codereview.chromium.org/208293002/diff/60001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_unittest.cc:2272: // False Start requires NPN and a
PFS cipher suite.
comment nit: s/PFS/forward-secret/ (match your comment on 2291)

https://codereview.chromium.org/208293002/diff/60001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_unittest.cc:2287: SSLConfig client_config;
client_config.next_protos.clear() , to ensure that even if the SSLConfig
defaults change, you're "safe" ?

https://codereview.chromium.org/208293002/diff/60001/net/test/spawned_test_se...
File net/test/spawned_test_server/base_test_server.cc (right):

https://codereview.chromium.org/208293002/diff/60001/net/test/spawned_test_se...
net/test/spawned_test_server/base_test_server.cc:430:
arguments->Set("next-proto", base::Value::CreateStringValue("http/1.1"));
Should this be a string option, rather than a bool?

https://codereview.chromium.org/208293002/diff/60001/net/test/spawned_test_se...
File net/test/spawned_test_server/base_test_server.h (right):

https://codereview.chromium.org/208293002/diff/60001/net/test/spawned_test_se...
net/test/spawned_test_server/base_test_server.h:83: KEY_EXCHANGE_DHE_RSA = (1 <<
1),
On 2014/04/02 19:26:55, wtc wrote:
> 
> The test server actually supports more key exchange algorithms than just these
> two. Should the other supported key exchange algorithms also be exposed here?

Not until we need em :)

https://codereview.chromium.org/208293002/diff/60001/net/test/spawned_test_se...
net/test/spawned_test_server/base_test_server.h:186: bool support_npn;
On 2014/04/02 19:26:55, wtc wrote:
> 
> Nit: support_npn => enable_npn?

+1

[davidben, 2014-04-03 19:38:36]

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

https://codereview.chromium.org/208293002/diff/60001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_unittest.cc:477: if (!pending_write_buf_)
On 2014/04/02 22:09:13, Ryan Sleevi wrote:
> comment nit: Can you add a comment about what this condition means
> 
> // Do nothing if UnblockWrite() was called after SetNextWriteShouldBlock(),
> without a Write() in between.

Done.

https://codereview.chromium.org/208293002/diff/60001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_unittest.cc:663: // up in a write buffer or if it
On 2014/04/02 22:09:13, Ryan Sleevi wrote:
> Incomplete comment?

Done. (For additional context: callback.have_result() is false in NSS and true
in OpenSSL. I think because we give OpenSSL a BIO pair while the memio for NSS
actually does PR_WOULD_BLOCK_ERROR until PutWriteResult.)

https://codereview.chromium.org/208293002/diff/60001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_unittest.cc:668: // Because of False Start, the
handshake should complete.
On 2014/04/02 22:09:13, Ryan Sleevi wrote:
> comment nit: Expand this comment
> 
> // When False Starting, the handshake should complete before receiving the
> Change Cipher Spec and Finished messages.

Done.

https://codereview.chromium.org/208293002/diff/60001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_unittest.cc:685: // The read will hang.
On 2014/04/02 22:09:13, Ryan Sleevi wrote:
> comment nit: Explain why the read will hang (eg: it's waiting for the peer to
> complete the handshake, which is still blocked)

Done.

https://codereview.chromium.org/208293002/diff/60001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_unittest.cc:2272: // False Start requires NPN and a
PFS cipher suite.
On 2014/04/02 22:09:13, Ryan Sleevi wrote:
> comment nit: s/PFS/forward-secret/ (match your comment on 2291)

Done.

https://codereview.chromium.org/208293002/diff/60001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_unittest.cc:2287: SSLConfig client_config;
On 2014/04/02 22:09:13, Ryan Sleevi wrote:
> client_config.next_protos.clear() , to ensure that even if the SSLConfig
> defaults change, you're "safe" ?

Done.

https://codereview.chromium.org/208293002/diff/60001/net/test/spawned_test_se...
File net/test/spawned_test_server/base_test_server.cc (right):

https://codereview.chromium.org/208293002/diff/60001/net/test/spawned_test_se...
net/test/spawned_test_server/base_test_server.cc:430:
arguments->Set("next-proto", base::Value::CreateStringValue("http/1.1"));
On 2014/04/02 22:09:13, Ryan Sleevi wrote:
> Should this be a string option, rather than a bool?

Well, I suppose if we want to go really crazy, it could be a string vector or a
bitmask. But a bool is probably sufficient for now since the test server can
only speak HTTP/1.1 anyway.

https://codereview.chromium.org/208293002/diff/60001/net/test/spawned_test_se...
File net/test/spawned_test_server/base_test_server.h (right):

https://codereview.chromium.org/208293002/diff/60001/net/test/spawned_test_se...
net/test/spawned_test_server/base_test_server.h:79: // is acceptable. Preferred
over explicitly OR-ing all ciphers.
On 2014/04/02 19:26:55, wtc wrote:
> 
> Typo: ciphers => key exchange algorithms

Done.

https://codereview.chromium.org/208293002/diff/60001/net/test/spawned_test_se...
net/test/spawned_test_server/base_test_server.h:83: KEY_EXCHANGE_DHE_RSA = (1 <<
1),
On 2014/04/02 22:09:13, Ryan Sleevi wrote:
> On 2014/04/02 19:26:55, wtc wrote:
> > 
> > The test server actually supports more key exchange algorithms than just
these
> > two. Should the other supported key exchange algorithms also be exposed
here?
> 
> Not until we need em :)

tlslite does, but the test server will need to pass along other parameters to
tlslite before they're supported anyway.

https://codereview.chromium.org/208293002/diff/60001/net/test/spawned_test_se...
net/test/spawned_test_server/base_test_server.h:186: bool support_npn;
On 2014/04/02 19:26:55, wtc wrote:
> 
> Nit: support_npn => enable_npn?

Done.

https://codereview.chromium.org/208293002/diff/60001/net/tools/testserver/tes...
File net/tools/testserver/testserver.py (right):

https://codereview.chromium.org/208293002/diff/60001/net/tools/testserver/tes...
net/tools/testserver/testserver.py:166: self.ssl_client_cas = []
On 2014/04/02 19:26:55, wtc wrote:
> 
> Nit: you may want to set self.next_protos = next_protos here, to match the
order
> of the constructor arguments.

Done.

https://codereview.chromium.org/208293002/diff/60001/net/tools/testserver/tes...
net/tools/testserver/testserver.py:2187: help='Specify the next proto value(s)
to '
On 2014/04/02 19:26:55, wtc wrote:
> 
> Question: how are multiple values separated. By spaces or commas?

It should be the same as --ssl-bulk-cipher and friends:

--next-proto=http/1.1 --next-proto=spdy/3

> Nit: it would be good to give an example of a value for this option. Also, if
> the test server doesn't support SPDY, then the only supported value is
> "http/1.1", right?

Though, yeah, it is true that HTTP/1.1 is the only supported value, which makes
the multivalued thing a little silly. I've switched it to a boolean to match the
API. We can make it more interesting if this server ever grows other protocols.

https://codereview.chromium.org/208293002/diff/60001/net/tools/testserver/tes...
net/tools/testserver/testserver.py:2191: 'the list of preferred protos.')
On 2014/04/02 19:26:55, wtc wrote:
> 
> Nit: I don't understand why you said "preferred". Are you implying the order
of
> multiple instances of this option match the order of preference?

Hrm. I think that's what I meant? That's a somewhat bizarre wording... it's the
list of protos that the server presents to the client. I believe they're
supposed to be order. The client might also be allowed to pick one off the list
for some reason if it really really wants to? I dunno.

[davidben, 2014-04-11 14:41:18]

Rebased to trunk. Now to see what the try bots say since DHE_RSA landed.

Almost there...

[Ryan Sleevi, 2014-04-11 19:08:49]

lgtm

[davidben, 2014-04-11 19:11:46]

The CQ bit was checked by davidben@chromium.org

[commit-bot: I haz the power, 2014-04-11 19:14:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/davidben@chromium.org/208293002/100001

[commit-bot: I haz the power, 2014-04-11 20:49:34]

Change committed as 263348