Send the ClientHello handshake message. Fix a bug in

Send the ClientHello handshake message.

Add most of the fields to ClientHello. Add the
QuicCryptoClientStream::CryptoConnect method and have
QuicClientSession::CryptoConnect call it. Add a unit test for
QuicCryptoClientStream to check a ClientHello message by parsing
it and checking the fields.

Define the value in CryptoTagValueMap as string instead of
StringPiece. This causes CryptoTagValueMap to copy the values,
which is less efficient but much safer.

Merge internal CLs: 39503243, 39692710

R=rch@chromium.org
BUG=none
TEST=net_unittests

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

[wtc, 2012-12-20 00:33:08]

rch: if my questions are unclear, I'll be happy to explain them
in person. Thanks.

https://codereview.chromium.org/11633030/diff/1/net/quic/quic_client_session.cc
File net/quic/quic_client_session.cc (right):

https://codereview.chromium.org/11633030/diff/1/net/quic/quic_client_session....
net/quic/quic_client_session.cc:58: }

Do you think |callback| should be passed along to crypto_stream_?

https://codereview.chromium.org/11633030/diff/1/net/quic/quic_client_session_...
File net/quic/quic_client_session_test.cc (right):

https://codereview.chromium.org/11633030/diff/1/net/quic/quic_client_session_...
net/quic/quic_client_session_test.cc:178: ASSERT_EQ(ERR_IO_PENDING,
session_.CryptoConnect(callback_.callback()));

Do you want me to copy
  ASSERT_EQ(1u, connection_->packets_.size());
  CheckClientHelloPacket(connection_->packets_[0]);
to this test?

Or do you think it is enough for the CryptoConnectSendsCorrectData
test to do that?

https://codereview.chromium.org/11633030/diff/1/net/quic/quic_client_session_...
net/quic/quic_client_session_test.cc:190:
CheckClientHelloPacket(connection_->packets_[0]);

Note that I copied these two lines to this test. Not sure if
this is appropriate.

https://codereview.chromium.org/11633030/diff/1/net/quic/quic_stream_factory_...
File net/quic/quic_stream_factory_test.cc (right):

https://codereview.chromium.org/11633030/diff/1/net/quic/quic_stream_factory_...
net/quic/quic_stream_factory_test.cc:134: MockWrite(SYNCHRONOUS,
ERR_CONNECTION_RESET),

I can't use |chlo|, which is an empty CHLO message, because
we are now generating a real CHLO message.

How do you suggest I fix this? I can change
ConstructHandshakePacket to build a real CHLO message, but
that is a lot of work and requires more refactoring.

Also note that the test still passes even though I use
the mock result ERR_CONNECTION_RESET. This seems to suggest
we are ignoring the failure of socket write.

[Ryan Hamilton, 2012-12-20 18:51:06]

https://codereview.chromium.org/11633030/diff/1/net/quic/quic_client_session.cc
File net/quic/quic_client_session.cc (right):

https://codereview.chromium.org/11633030/diff/1/net/quic/quic_client_session....
net/quic/quic_client_session.cc:58: }
On 2012/12/20 00:33:08, wtc wrote:
> 
> Do you think |callback| should be passed along to crypto_stream_?

Can we avoid doing so?  The internal code does not use CompletionCallbacks.

https://codereview.chromium.org/11633030/diff/1/net/quic/quic_client_session_...
File net/quic/quic_client_session_test.cc (right):

https://codereview.chromium.org/11633030/diff/1/net/quic/quic_client_session_...
net/quic/quic_client_session_test.cc:178: ASSERT_EQ(ERR_IO_PENDING,
session_.CryptoConnect(callback_.callback()));
On 2012/12/20 00:33:08, wtc wrote:
> 
> Do you want me to copy
>   ASSERT_EQ(1u, connection_->packets_.size());
>   CheckClientHelloPacket(connection_->packets_[0]);
> to this test?
> 
> Or do you think it is enough for the CryptoConnectSendsCorrectData
> test to do that?

I think it's fine to rely on the earlier test.

https://codereview.chromium.org/11633030/diff/1/net/quic/quic_client_session_...
net/quic/quic_client_session_test.cc:190:
CheckClientHelloPacket(connection_->packets_[0]);
On 2012/12/20 00:33:08, wtc wrote:
> 
> Note that I copied these two lines to this test. Not sure if
> this is appropriate.

My vote goes for not duplicating the earlier test, but I don't feel strongly.

https://codereview.chromium.org/11633030/diff/1/net/quic/quic_stream_factory_...
File net/quic/quic_stream_factory_test.cc (right):

https://codereview.chromium.org/11633030/diff/1/net/quic/quic_stream_factory_...
net/quic/quic_stream_factory_test.cc:134: MockWrite(SYNCHRONOUS,
ERR_CONNECTION_RESET),
On 2012/12/20 00:33:08, wtc wrote:
> 
> I can't use |chlo|, which is an empty CHLO message, because
> we are now generating a real CHLO message.
> 
> How do you suggest I fix this? I can change
> ConstructHandshakePacket to build a real CHLO message, but
> that is a lot of work and requires more refactoring.
> 
> Also note that the test still passes even though I use
> the mock result ERR_CONNECTION_RESET. This seems to suggest
> we are ignoring the failure of socket write.

Let's chat offline.

[wtc, 2012-12-22 04:21:51]

Please review patch set 2. I hope this is closer to what you
have in mind. I have two more questions for you. Thanks.

https://codereview.chromium.org/11633030/diff/7001/net/quic/crypto/crypto_pro...
File net/quic/crypto/crypto_protocol.h (right):

https://codereview.chromium.org/11633030/diff/7001/net/quic/crypto/crypto_pro...
net/quic/crypto/crypto_protocol.h:73: struct QuicClientHelloConfig {

Should I define this struct in this header or in its own
header?

quic_protocol.h defines several structs, which is why I
just define this struct in this header.

https://codereview.chromium.org/11633030/diff/7001/net/quic/quic_utils.h
File net/quic/quic_utils.h (right):

https://codereview.chromium.org/11633030/diff/7001/net/quic/quic_utils.h#newc...
net/quic/quic_utils.h:45: CryptoHandshakeMessage* message);

Is QuicUtils the right home for these two utility functions?
Or should I add a HandshakeUtils class for them?

[Ryan Hamilton, 2012-12-22 22:31:32]

https://codereview.chromium.org/11633030/diff/7001/net/quic/crypto/crypto_fra...
File net/quic/crypto/crypto_framer_test.cc (right):

https://codereview.chromium.org/11633030/diff/7001/net/quic/crypto/crypto_fra...
net/quic/crypto/crypto_framer_test.cc:216:
EXPECT_EQ("abcdef",visitor.messages_[0].tag_value_map[0x12345678]);
nit: missing space.

https://codereview.chromium.org/11633030/diff/7001/net/quic/crypto/crypto_fra...
net/quic/crypto/crypto_framer_test.cc:261:
EXPECT_EQ("abcdef",visitor.messages_[0].tag_value_map[0x12345678]);
nit: missing space.

https://codereview.chromium.org/11633030/diff/7001/net/quic/crypto/crypto_fra...
net/quic/crypto/crypto_framer_test.cc:299:
EXPECT_EQ("abcdef",visitor.messages_[0].tag_value_map[0x12345678]);
nit: missing space.

https://codereview.chromium.org/11633030/diff/7001/net/quic/crypto/crypto_pro...
File net/quic/crypto/crypto_protocol.cc (right):

https://codereview.chromium.org/11633030/diff/7001/net/quic/crypto/crypto_pro...
net/quic/crypto/crypto_protocol.cc:12:
QuicClientHelloConfig::QuicClientHelloConfig() {
These seem like sensible values, but I confess, I was surprised to see the
default constructor initialize them to these values.  I might be tempted to have
the default constructor initialize them to 0 or "empty" values, and have a
distinct method to populate them with defaults.

https://codereview.chromium.org/11633030/diff/7001/net/quic/crypto/crypto_pro...
File net/quic/crypto/crypto_protocol.h (right):

https://codereview.chromium.org/11633030/diff/7001/net/quic/crypto/crypto_pro...
net/quic/crypto/crypto_protocol.h:73: struct QuicClientHelloConfig {
On 2012/12/22 04:21:51, wtc wrote:
> 
> Should I define this struct in this header or in its own
> header?
> 
> quic_protocol.h defines several structs, which is why I
> just define this struct in this header.

Defining it here seems fine to me.  However, I think I'd call it
QuicClientHelloMessage (instead of Config).  This is analogous to the various
Quic*Frame structs we have which define an in-memory representation of the
frames.

https://codereview.chromium.org/11633030/diff/7001/net/quic/crypto/crypto_pro...
net/quic/crypto/crypto_protocol.h:85: uint32 idle_connection_state_lifetime;
Can we make this a QuicTime::Delta?  (In-memory, times should be QuicTime and
deltas should be QuicTime::Delta.  We only switch to integers when we go to/from
the wire).

https://codereview.chromium.org/11633030/diff/7001/net/quic/crypto/crypto_pro...
net/quic/crypto/crypto_protocol.h:87: uint32 keepalive_timeout;
Ditto.

https://codereview.chromium.org/11633030/diff/7001/net/quic/quic_client_sessi...
File net/quic/quic_client_session.cc (right):

https://codereview.chromium.org/11633030/diff/7001/net/quic/quic_client_sessi...
net/quic/quic_client_session.cc:57: // TODO(wtc): map to a net error code.
Not sure about this comment.  Isn't ERR_CONNECTION_FAILED a net error code?

https://codereview.chromium.org/11633030/diff/7001/net/quic/quic_client_sessi...
File net/quic/quic_client_session_test.cc (right):

https://codereview.chromium.org/11633030/diff/7001/net/quic/quic_client_sessi...
net/quic/quic_client_session_test.cc:100: void
QuicClientSessionTest::CheckClientHelloPacket(QuicPacket* packet) {
Instead of parsing all the fields of the the CHLO message in
QuicClientSessionTest, I think I would move this logic to a new test in
quic_crypto_client_stream_test.cc

Then in this test, I would probably simply call ConstructClientHelloPacket() to
create a CHLO packet and strcmp the results with packets[0].  

Make sense?  basically, the test that inspects the innards of the CHLO packet
belongs in a test of the code the generates it.  Everywhere else, we can simply
generate a CHLO with the same inputs and be confident that it is valid.

https://codereview.chromium.org/11633030/diff/7001/net/quic/quic_crypto_clien...
File net/quic/quic_crypto_client_stream.h (right):

https://codereview.chromium.org/11633030/diff/7001/net/quic/quic_crypto_clien...
net/quic/quic_crypto_client_stream.h:32: std::string nonce_;
nit: #include <string>

https://codereview.chromium.org/11633030/diff/7001/net/quic/quic_crypto_strea...
File net/quic/quic_crypto_stream_test.cc (right):

https://codereview.chromium.org/11633030/diff/7001/net/quic/quic_crypto_strea...
net/quic/quic_crypto_stream_test.cc:30: messages_.push_back(message);
Nice!  This is much cleaner.

https://codereview.chromium.org/11633030/diff/7001/net/quic/quic_utils.h
File net/quic/quic_utils.h (right):

https://codereview.chromium.org/11633030/diff/7001/net/quic/quic_utils.h#newc...
net/quic/quic_utils.h:43: static void FillClientHelloMessage(const
QuicClientHelloConfig& config,
I think you'll end up wanting the inverse of this method for ease of testing and
for handling server side.  Doesn't need to be added in this CL, though it would
make your CheckClientHelloMessage method easier to implement.

https://codereview.chromium.org/11633030/diff/7001/net/quic/quic_utils.h#newc...
net/quic/quic_utils.h:44: const std::string& nonce,
nit: add #include <string>

https://codereview.chromium.org/11633030/diff/7001/net/quic/quic_utils.h#newc...
net/quic/quic_utils.h:45: CryptoHandshakeMessage* message);
On 2012/12/22 04:21:51, wtc wrote:
> 
> Is QuicUtils the right home for these two utility functions?
> Or should I add a HandshakeUtils class for them?

This is fine.  I think I'd probably create a CryptoUtils class.  Either way,
though.

[wtc, 2013-01-15 03:57:59]

Please review patch set 3.

Thank you for your thoughtful review comments. I made all
the changes you suggested except two.

1. I didn't rename QuicClientHelloConfig to QuicClientHelloMessage
because I intend to use the struct to represent the client-side
crypto configuration settings. It just so happens that it
is very similar to (but not the same as) the contents of
ClientHello.

I did rename the struct to QuicClientCryptoConfig to make
this clearer. However, I am still open to naming this
struct QuicClientHelloMessage, in which case it will need
to match the contents of ClientHello exactly.

2. I didn't add a new function to decode a ClientHello
message and use it in a QuicClientCryptoStream unit test.
One reason is that I don't know how to extract the ClientHello
message generated by QuicClientCryptoStream. (The other
reason is that it would be better to do that as part of
the server-side code to handle ClientHello.)

https://codereview.chromium.org/11633030/diff/7001/net/quic/quic_client_sessi...
File net/quic/quic_client_session_test.cc (right):

https://codereview.chromium.org/11633030/diff/7001/net/quic/quic_client_sessi...
net/quic/quic_client_session_test.cc:100: void
QuicClientSessionTest::CheckClientHelloPacket(QuicPacket* packet) {

On 2012/12/22 22:31:32, Ryan Hamilton wrote:
> Instead of parsing all the fields of the the CHLO message in
> QuicClientSessionTest, I think I would move this logic to a new test in
> quic_crypto_client_stream_test.cc
> 
> Then in this test, I would probably simply call ConstructClientHelloPacket()
to
> create a CHLO packet and strcmp the results with packets[0].

Thank you for the suggestion. I did the latter, but not
the former.

I will think about the former and ask you if I have questions,
then implement it (possibly in a separate CL).

https://codereview.chromium.org/11633030/diff/19001/net/quic/crypto/crypto_pr...
File net/quic/crypto/crypto_protocol.cc (right):

https://codereview.chromium.org/11633030/diff/19001/net/quic/crypto/crypto_pr...
net/quic/crypto/crypto_protocol.cc:34: idle_connection_state_lifetime =
QuicTime::Delta::FromMilliseconds(300000);

Would you like me to add a QuicTime::Delta::FromSeconds()
method to make this code more readable?

[Ryan Hamilton, 2013-01-15 04:27:41]

lgtm

https://codereview.chromium.org/11633030/diff/19001/net/quic/crypto/crypto_pr...
File net/quic/crypto/crypto_protocol.cc (right):

https://codereview.chromium.org/11633030/diff/19001/net/quic/crypto/crypto_pr...
net/quic/crypto/crypto_protocol.cc:34: idle_connection_state_lifetime =
QuicTime::Delta::FromMilliseconds(300000);
On 2013/01/15 03:57:59, wtc wrote:
> 
> Would you like me to add a QuicTime::Delta::FromSeconds()
> method to make this code more readable?

The current code is fine with me.

[wtc, 2013-01-15 23:11:03]

Please review patch set 4, which has the new
QuicCryptoClientStreamTest that you suggested. Thanks.

You can just review the diffs between patch sets 3 and 4.

https://codereview.chromium.org/11633030/diff/19001/net/quic/test_tools/quic_...
File net/quic/test_tools/quic_test_utils.h (right):

https://codereview.chromium.org/11633030/diff/19001/net/quic/test_tools/quic_...
net/quic/test_tools/quic_test_utils.h:206: void(QuicStreamId id,
base::StringPiece data, bool fin));

I removed this mock WriteData method because QuicSession
doesn't have a three-argument form of WriteData.

https://codereview.chromium.org/11633030/diff/31001/net/quic/quic_crypto_clie...
File net/quic/quic_crypto_client_stream_test.cc (right):

https://codereview.chromium.org/11633030/diff/31001/net/quic/quic_crypto_clie...
net/quic/quic_crypto_client_stream_test.cc:161: }

I had to use the real WriteData() method of session().

I am worried that the real WriteData() method may malfunction
if all the other methods of session() are mocked. Do you
know if this is OK? The test does pass.

https://codereview.chromium.org/11633030/diff/31001/net/quic/test_tools/quic_...
File net/quic/test_tools/quic_test_utils.h (right):

https://codereview.chromium.org/11633030/diff/31001/net/quic/test_tools/quic_...
net/quic/test_tools/quic_test_utils.h:157: QuicConnectionHelperInterface*
helper);

This formatting is allowed by the current Style Guide. If
you prefer one parameter per line, please let me know.

[Ryan Hamilton, 2013-01-16 00:29:36]

LGTM, modulo the style question.  Can you ping jar (since he sits next to you). 
If he's cool with it, I'm cool with it.

https://codereview.chromium.org/11633030/diff/19001/net/quic/test_tools/quic_...
File net/quic/test_tools/quic_test_utils.h (right):

https://codereview.chromium.org/11633030/diff/19001/net/quic/test_tools/quic_...
net/quic/test_tools/quic_test_utils.h:206: void(QuicStreamId id,
base::StringPiece data, bool fin));
On 2013/01/15 23:11:03, wtc wrote:
> 
> I removed this mock WriteData method because QuicSession
> doesn't have a three-argument form of WriteData.

Oh nice.

https://codereview.chromium.org/11633030/diff/31001/net/quic/quic_crypto_clie...
File net/quic/quic_crypto_client_stream_test.cc (right):

https://codereview.chromium.org/11633030/diff/31001/net/quic/quic_crypto_clie...
net/quic/quic_crypto_client_stream_test.cc:161: }
On 2013/01/15 23:11:03, wtc wrote:
> 
> I had to use the real WriteData() method of session().
> 
> I am worried that the real WriteData() method may malfunction
> if all the other methods of session() are mocked. Do you
> know if this is OK? The test does pass.

I think this should be fine.  If it ever breaks, the test will fail so whoever
breaks it can fix it :>

https://codereview.chromium.org/11633030/diff/31001/net/quic/test_tools/quic_...
File net/quic/test_tools/quic_test_utils.h (right):

https://codereview.chromium.org/11633030/diff/31001/net/quic/test_tools/quic_...
net/quic/test_tools/quic_test_utils.h:157: QuicConnectionHelperInterface*
helper);
On 2013/01/15 23:11:03, wtc wrote:
> 
> This formatting is allowed by the current Style Guide. If
> you prefer one parameter per line, please let me know.

Are you sure?  jar complains about this every time.

[wtc, 2013-01-16 00:43:49]

https://codereview.chromium.org/11633030/diff/31001/net/quic/test_tools/quic_...
File net/quic/test_tools/quic_test_utils.h (right):

https://codereview.chromium.org/11633030/diff/31001/net/quic/test_tools/quic_...
net/quic/test_tools/quic_test_utils.h:157: QuicConnectionHelperInterface*
helper);

On 2013/01/16 00:29:36, Ryan Hamilton wrote:
>
> Are you sure?  jar complains about this every time. 

Yes, I am sure. See the second code example at
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Decla...

  If you have too much text to fit on one line:

    ReturnType ClassName::ReallyLongFunctionName(Type par_name1, Type par_name2,
                                                 Type par_name3) {
      DoSomething();
      ...
    }

I changed the formatting to preserve the old style.

[commit-bot: I haz the power, 2013-01-16 03:42:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wtc@chromium.org/11633030/57005

[commit-bot: I haz the power, 2013-01-16 08:51:19]

Change committed as 177102