Require handshake confirmation until a QUIC connection is created succesfully when using a new netw…

Require handshake confirmation until a QUIC connection is created succesfully when using a new network.

BUG=

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

[ramant (doing other things), 2013-08-19 18:39:05]

lgtm

https://chromiumcodereview.appspot.com/23279011/diff/3001/net/quic/quic_netwo...
File net/quic/quic_network_transaction_unittest.cc (right):

https://chromiumcodereview.appspot.com/23279011/diff/3001/net/quic/quic_netwo...
net/quic/quic_network_transaction_unittest.cc:726: // connecto the the server,
in this test we require confirmation
nit: connecto the -> connection to

https://chromiumcodereview.appspot.com/23279011/diff/3001/net/quic/test_tools...
File net/quic/test_tools/mock_crypto_client_stream.cc (right):

https://chromiumcodereview.appspot.com/23279011/diff/3001/net/quic/test_tools...
net/quic/test_tools/mock_crypto_client_stream.cc:55:
QuicSession::CryptoHandshakeEvent event) {
overly nit: consider adding a "DCHECK_NE(event, COLD_START);" to avoid wrong
usage of this method (if it makes sense). What do you think?

[Ryan Hamilton, 2013-08-19 18:51:17]

Thanks!

https://chromiumcodereview.appspot.com/23279011/diff/3001/net/quic/quic_netwo...
File net/quic/quic_network_transaction_unittest.cc (right):

https://chromiumcodereview.appspot.com/23279011/diff/3001/net/quic/quic_netwo...
net/quic/quic_network_transaction_unittest.cc:726: // connecto the the server,
in this test we require confirmation
On 2013/08/19 18:39:05, ramant wrote:
> nit: connecto the -> connection to

Done.

https://chromiumcodereview.appspot.com/23279011/diff/3001/net/quic/test_tools...
File net/quic/test_tools/mock_crypto_client_stream.cc (right):

https://chromiumcodereview.appspot.com/23279011/diff/3001/net/quic/test_tools...
net/quic/test_tools/mock_crypto_client_stream.cc:55:
QuicSession::CryptoHandshakeEvent event) {
On 2013/08/19 18:39:05, ramant wrote:
> overly nit: consider adding a "DCHECK_NE(event, COLD_START);" to avoid wrong
> usage of this method (if it makes sense). What do you think?

I like the idea, but COLD_START is one of the hanshake modes defined in this
mock class.  |event| is a QuicSession::CryptoHandshakeEvent, which only includes
initially established, re-established, handshake confirmed.  So I think we're ok
here.

[commit-bot: I haz the power, 2013-08-19 18:53:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rch@chromium.org/23279011/9001

[commit-bot: I haz the power, 2013-08-19 18:53:41]

Failed to apply patch for net/quic/quic_client_session.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file net/quic/quic_client_session.cc
  Hunk #1 FAILED at 89.
  1 out of 4 hunks FAILED -- saving rejects to file
net/quic/quic_client_session.cc.rej

Patch:       net/quic/quic_client_session.cc
Index: net/quic/quic_client_session.cc
diff --git a/net/quic/quic_client_session.cc b/net/quic/quic_client_session.cc
index
d7fb0d2675ed5be5be7e630232387730df092f3a..53c3a5cb03d31575ac160f7c6bb76990339be92a
100644
--- a/net/quic/quic_client_session.cc
+++ b/net/quic/quic_client_session.cc
@@ -89,14 +89,15 @@ QuicClientSession::QuicClientSession(
     QuicCryptoClientConfig* crypto_config,
     NetLog* net_log)
     : QuicSession(connection, config, false),
-      weak_factory_(this),
+      require_confirmation_(false),
       stream_factory_(stream_factory),
       socket_(socket),
       read_buffer_(new IOBufferWithSize(kMaxPacketSize)),
       read_pending_(false),
       num_total_streams_(0),
       net_log_(BoundNetLog::Make(net_log, NetLog::SOURCE_QUIC_SESSION)),
-      logger_(net_log_) {
+      logger_(net_log_),
+      weak_factory_(this) {
   crypto_stream_.reset(
       crypto_client_stream_factory ?
           crypto_client_stream_factory->CreateQuicCryptoClientStream(
@@ -209,7 +210,9 @@ bool QuicClientSession::GetSSLInfo(SSLInfo* ssl_info) {
   return crypto_stream_->GetSSLInfo(ssl_info);
 }
 
-int QuicClientSession::CryptoConnect(const CompletionCallback& callback) {
+int QuicClientSession::CryptoConnect(bool require_confirmation,
+                                     const CompletionCallback& callback) {
+  require_confirmation_ = require_confirmation;
   RecordHandshakeState(STATE_STARTED);
   if (!crypto_stream_->CryptoConnect()) {
     // TODO(wtc): change crypto_stream_.CryptoConnect() to return a
@@ -217,7 +220,9 @@ int QuicClientSession::CryptoConnect(const
CompletionCallback& callback) {
     return ERR_CONNECTION_FAILED;
   }
 
-  if (IsEncryptionEstablished()) {
+  bool can_notify = require_confirmation_ ?
+      IsCryptoHandshakeConfirmed() : IsEncryptionEstablished();
+  if (can_notify) {
     return OK;
   }
 
@@ -250,7 +255,8 @@ void QuicClientSession::CloseStream(QuicStreamId stream_id)
{
 }
 
 void QuicClientSession::OnCryptoHandshakeEvent(CryptoHandshakeEvent event) {
-  if (!callback_.is_null()) {
+  if (!callback_.is_null() &&
+      (!require_confirmation_ || event == HANDSHAKE_CONFIRMED)) {
     // TODO(rtenneti): Currently for all CryptoHandshakeEvent events, callback_
     // could be called because there are no error events in
CryptoHandshakeEvent
     // enum. If error events are added to CryptoHandshakeEvent, then the

[commit-bot: I haz the power, 2013-08-19 19:45:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rch@chromium.org/23279011/13001

[commit-bot: I haz the power, 2013-08-19 20:04:23]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-08-19 22:16:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rch@chromium.org/23279011/45001

[commit-bot: I haz the power, 2013-08-20 02:04:16]

Change committed as 218412

[wtc, 2013-08-21 01:09:32]

Patch set 5 LGTM. I have some questions.

https://codereview.chromium.org/23279011/diff/45001/net/quic/quic_client_sess...
File net/quic/quic_client_session.cc (right):

https://codereview.chromium.org/23279011/diff/45001/net/quic/quic_client_sess...
net/quic/quic_client_session.cc:266: QuicSession::OnCryptoHandshakeEvent(event);

An unrelated question: have you thought about the order of running the
callback and calling QuicSession::OnCryptoHandshakeEvent()? Does the
order matter?

The reason I asked is that usually base::ResetAndReturn(&callback_).Run()
is the last thing we do in an OnXXXEvent method.

https://codereview.chromium.org/23279011/diff/45001/net/quic/quic_stream_fact...
File net/quic/quic_stream_factory.cc (right):

https://codereview.chromium.org/23279011/diff/45001/net/quic/quic_stream_fact...
net/quic/quic_stream_factory.cc:253: : require_confirmation_(true),

Can you explain why require_confirmation_ should be initialized to true?

I guess it's because we require confirmation for the very first connect and
the first connect after each network change?

It would be nice to document this somewhere, which will make it easier to
understand where we should set require_confirmation_ to true or false.

https://codereview.chromium.org/23279011/diff/45001/net/quic/quic_stream_fact...
net/quic/quic_stream_factory.cc:300: if (rv == OK) {

Should we also set require_confirmation_ to false in this case, to match
the asynchronous completion case on line 309?

https://codereview.chromium.org/23279011/diff/45001/net/quic/test_tools/mock_...
File net/quic/test_tools/mock_crypto_client_stream.cc (right):

https://codereview.chromium.org/23279011/diff/45001/net/quic/test_tools/mock_...
net/quic/test_tools/mock_crypto_client_stream.cc:56: encryption_established_ =
true;

Nit: it seems that we only need to set encryption_established_ to true
on the ENCRYPTION_ESTABLISHED event, right?

https://codereview.chromium.org/23279011/diff/45001/net/quic/test_tools/mock_...
File net/quic/test_tools/mock_crypto_client_stream.h (right):

https://codereview.chromium.org/23279011/diff/45001/net/quic/test_tools/mock_...
net/quic/test_tools/mock_crypto_client_stream.h:50: // Invokes the sessions's
CryptoHandshakeEvent method with the specified

Typo: CryptoHandshakeEvent => OnCryptoHandshakeEvent

https://codereview.chromium.org/23279011/diff/45001/net/quic/test_tools/mock_...
net/quic/test_tools/mock_crypto_client_stream.h:52: void
SendOnCryptoHandshakeEvent(QuicSession::CryptoHandshakeEvent event);

What does the "Send" in the method's name mean?