net: Implement ChannelIDSourceChromium, which is based on Chromium's

net/quic/quic_crypto_client_stream.cc

Index: net/quic/quic_crypto_client_stream.cc
===================================================================
--- net/quic/quic_crypto_client_stream.cc	(revision 279242)
+++ net/quic/quic_crypto_client_stream.cc	(working copy)
@@ -4,17 +4,41 @@
 
 #include "net/quic/quic_crypto_client_stream.h"
 
-#include "net/quic/crypto/channel_id.h"
 #include "net/quic/crypto/crypto_protocol.h"
 #include "net/quic/crypto/crypto_utils.h"
 #include "net/quic/crypto/null_encrypter.h"
-#include "net/quic/crypto/proof_verifier.h"
 #include "net/quic/quic_client_session_base.h"
 #include "net/quic/quic_protocol.h"
 #include "net/quic/quic_session.h"
 
 namespace net {
 
+QuicCryptoClientStream::ChannelIDSourceCallbackImpl::
+ChannelIDSourceCallbackImpl(
+    QuicCryptoClientStream* stream)

[Ryan Hamilton, 2014/06/25 18:55:15]

can you move this to the previous line?

[wtc, 2014/06/28 16:03:29]

Done.

+    : stream_(stream) {}
+
+QuicCryptoClientStream::ChannelIDSourceCallbackImpl::
+~ChannelIDSourceCallbackImpl() {}
+
+void QuicCryptoClientStream::ChannelIDSourceCallbackImpl::Run(
+    scoped_ptr<ChannelIDKey>* channel_id_key) {
+  if (stream_ == NULL) {
+    return;
+  }
+
+  stream_->channel_id_key_.reset(channel_id_key->release());
+  stream_->channel_id_source_callback_ = NULL;
+  stream_->DoHandshakeLoop(NULL);
+
+  // The ChannelIDSource owns this object and will delete it when this method
+  // returns.
+}
+
+void QuicCryptoClientStream::ChannelIDSourceCallbackImpl::Cancel() {
+  stream_ = NULL;
+}
+
 QuicCryptoClientStream::ProofVerifierCallbackImpl::ProofVerifierCallbackImpl(
     QuicCryptoClientStream* stream)
     : stream_(stream) {}
@@ -55,11 +79,15 @@
       crypto_config_(crypto_config),
       server_id_(server_id),
       generation_counter_(0),
+      channel_id_source_callback_(NULL),
       proof_verify_callback_(NULL),
       verify_context_(verify_context) {
 }
 
 QuicCryptoClientStream::~QuicCryptoClientStream() {
+  if (channel_id_source_callback_) {
+    channel_id_source_callback_->Cancel();
+  }
   if (proof_verify_callback_) {
     proof_verify_callback_->Cancel();
   }
@@ -106,13 +134,13 @@
     next_state_ = STATE_IDLE;
     switch (state) {
       case STATE_INITIALIZE: {
-        if (!cached->IsEmpty() && !cached->signature().empty() &&
-            server_id_.is_https()) {
+        if (!cached->IsEmpty() && !cached->proof_valid() &&
+            !cached->signature().empty() && server_id_.is_https()) {
           DCHECK(crypto_config_->proof_verifier());
           // If the cached state needs to be verified, do it now.
           next_state_ = STATE_VERIFY_PROOF;
         } else {
-          next_state_ = STATE_SEND_CHLO;
+          next_state_ = STATE_GET_CHANNEL_ID;
         }
         break;
       }
@@ -152,35 +180,6 @@
           return;
         }
         session()->config()->ToHandshakeMessage(&out);
-
-        scoped_ptr<ChannelIDKey> channel_id_key;
-        bool do_channel_id = false;
-        if (crypto_config_->channel_id_source()) {
-          const CryptoHandshakeMessage* scfg = cached->GetServerConfig();
-          DCHECK(scfg);
-          const QuicTag* their_proof_demands;
-          size_t num_their_proof_demands;
-          if (scfg->GetTaglist(kPDMD, &their_proof_demands,
-                               &num_their_proof_demands) == QUIC_NO_ERROR) {
-            for (size_t i = 0; i < num_their_proof_demands; i++) {
-              if (their_proof_demands[i] == kCHID) {
-                do_channel_id = true;
-                break;
-              }
-            }
-          }
-        }
-        if (do_channel_id) {
-          QuicAsyncStatus status =
-              crypto_config_->channel_id_source()->GetChannelIDKey(
-                  server_id_.host(), &channel_id_key, NULL);
-          if (status != QUIC_SUCCESS) {
-            CloseConnectionWithDetails(QUIC_INVALID_CHANNEL_ID_SIGNATURE,
-                                       "Channel ID lookup failed");
-            return;
-          }
-        }
-
         error = crypto_config_->FillClientHello(
             server_id_,
             session()->connection()->connection_id(),
@@ -188,7 +187,7 @@
             cached,
             session()->connection()->clock()->WallNow(),
             session()->connection()->random_generator(),
-            channel_id_key.get(),
+            channel_id_key_.get(),
             &crypto_negotiated_params_,
             &out,
             &error_details);
@@ -254,7 +253,7 @@
             break;
           }
         }
-        next_state_ = STATE_SEND_CHLO;
+        next_state_ = STATE_GET_CHANNEL_ID;
         break;
       case STATE_VERIFY_PROOF: {
         ProofVerifier* verifier = crypto_config_->proof_verifier();
@@ -306,9 +305,55 @@
         } else {
           SetCachedProofValid(cached);
           cached->SetProofVerifyDetails(verify_details_.release());
-          next_state_ = STATE_SEND_CHLO;
+          next_state_ = STATE_GET_CHANNEL_ID;
         }
         break;
+      case STATE_GET_CHANNEL_ID: {
+        next_state_ = STATE_GET_CHANNEL_ID_COMPLETE;
+        channel_id_key_.reset();
+        bool do_channel_id = false;
+        if (server_id_.is_https() &&
+            server_id_.privacy_mode() == PRIVACY_MODE_DISABLED &&
+            crypto_config_->channel_id_source()) {
+          const CryptoHandshakeMessage* scfg = cached->GetServerConfig();
+          const QuicTag* their_proof_demands;
+          size_t num_their_proof_demands;
+          if (scfg &&  // scfg may be null when we send inchoate CHLO.
+              scfg->GetTaglist(kPDMD, &their_proof_demands,
+                               &num_their_proof_demands) == QUIC_NO_ERROR) {
+            for (size_t i = 0; i < num_their_proof_demands; i++) {
+              if (their_proof_demands[i] == kCHID) {
+                do_channel_id = true;
+                break;
+              }
+            }
+          }
+        }

[Ryan Hamilton, 2014/06/25 23:40:26]

This method is ... big. Can you factor this block into:
bool do_channel_id = RequiresChannelId(cached)
if (!do_channel_id) {
  break;
}
...

[wtc, 2014/06/28 16:03:29]

Done.

+
+        if (do_channel_id) {
+          ChannelIDSourceCallbackImpl* channel_id_source_callback =
+              new ChannelIDSourceCallbackImpl(this);
+          QuicAsyncStatus status =
+              crypto_config_->channel_id_source()->GetChannelIDKey(
+                  server_id_.host(), &channel_id_key_,
+                  channel_id_source_callback);
+          if (status == QUIC_PENDING) {

[Ryan Hamilton, 2014/06/25 23:40:26]

Consider using a switch here to be parallel with the code in STATE_VERIFY_PROOF.

[wtc, 2014/06/28 16:03:29]

Done. I wrote it this way to avoid duplicating the "delete
channel_id_source_callback" in two cases of the switch statement. But I
understand consistency is good.

[Ryan Hamilton, 2014/06/30 17:47:03]

Oh, fair enough! Feel free to switch both to the pattern you used in this CL if
you prefer...

+            channel_id_source_callback_ = channel_id_source_callback;
+            DVLOG(1) << "Looking up channel ID";
+            return;
+          }
+          delete channel_id_source_callback;
+          if (status == QUIC_FAILURE) {
+            CloseConnectionWithDetails(QUIC_INVALID_CHANNEL_ID_SIGNATURE,
+                                       "Channel ID lookup failed");
+            return;
+          }
+        }
+        break;
+      }
+      case STATE_GET_CHANNEL_ID_COMPLETE:
+        next_state_ = STATE_SEND_CHLO;

[Ryan Hamilton, 2014/06/25 23:40:26]

It seems like the channel id lookup might fail with an error code. In that case,
should we close the connection, or some such?

[wtc, 2014/06/28 16:03:30]

You are right -- we should handle synchronous and asynchronous channel id lookup
failures in the same way. I missed this point.

In my API design, an asynchronous channel id lookup failure is detected by a
null channel_id_key_. So if a channel id is not required, I am now transitioning
to the STATE_SEND_CHLO state directly.

+        break;
       case STATE_RECV_SHLO: {
         // We sent a CHLO that we expected to be accepted and now we're hoping
         // for a SHLO from the server to confirm that.