Persist server's crypto config data to disk cache for 0-RTT

net/quic/quic_crypto_client_stream.cc

Index: net/quic/quic_crypto_client_stream.cc
diff --git a/net/quic/quic_crypto_client_stream.cc b/net/quic/quic_crypto_client_stream.cc
index 3e5f84059fc0ec4567c39d1da66a18f7c3f5299c..960e72d5a73765d7d467ab68aab3a0e666de7d93 100644
--- a/net/quic/quic_crypto_client_stream.cc
+++ b/net/quic/quic_crypto_client_stream.cc
@@ -11,6 +11,7 @@
 #include "net/quic/crypto/null_encrypter.h"
 #include "net/quic/crypto/proof_verifier.h"
 #include "net/quic/crypto/proof_verifier_chromium.h"
+#include "net/quic/crypto/quic_server_info.h"
 #include "net/quic/quic_protocol.h"
 #include "net/quic/quic_session.h"
 #include "net/ssl/ssl_connection_status_flags.h"
@@ -74,7 +75,8 @@ QuicCryptoClientStream::QuicCryptoClientStream(
       crypto_config_(crypto_config),
       server_hostname_(server_hostname),
       generation_counter_(0),
-      proof_verify_callback_(NULL) {
+      proof_verify_callback_(NULL),
+      disk_cache_load_result_(ERR_UNEXPECTED) {
 }
 
 QuicCryptoClientStream::~QuicCryptoClientStream() {
@@ -91,7 +93,7 @@ void QuicCryptoClientStream::OnHandshakeMessage(
 }
 
 bool QuicCryptoClientStream::CryptoConnect() {
-  next_state_ = STATE_SEND_CHLO;
+  next_state_ = STATE_LOAD_QUIC_SERVER_INFO;
   DoHandshakeLoop(NULL);
   return true;
 }
@@ -159,6 +161,16 @@ void QuicCryptoClientStream::DoHandshakeLoop(
     const State state = next_state_;
     next_state_ = STATE_IDLE;
     switch (state) {
+      case STATE_LOAD_QUIC_SERVER_INFO: {
+        if (DoLoadQuicServerInfo(cached) == ERR_IO_PENDING) {
+          return;
+        }
+        break;
+      }
+      case STATE_LOAD_QUIC_SERVER_INFO_COMPLETE: {
+        DoLoadQuicServerInfoComplete(cached);
+        break;
+      }
       case STATE_SEND_CHLO: {
         // Send the client hello in plaintext.
         session()->connection()->SetDefaultEncryptionLevel(ENCRYPTION_NONE);
@@ -394,4 +406,73 @@ void QuicCryptoClientStream::DoHandshakeLoop(
   }
 }
 
+void QuicCryptoClientStream::OnIOComplete(int result) {
+  DCHECK_EQ(STATE_LOAD_QUIC_SERVER_INFO_COMPLETE, next_state_);
+  DCHECK_NE(ERR_IO_PENDING, result);
+  disk_cache_load_result_ = result;
+  DoHandshakeLoop(NULL);
+}
+
+int QuicCryptoClientStream::DoLoadQuicServerInfo(
+    QuicCryptoClientConfig::CachedState* cached) {
+  next_state_ = STATE_SEND_CHLO;
+  QuicServerInfo* quic_server_info = cached->quic_server_info();
+  if (!quic_server_info) {
+    return OK;
+  }
+
+  disk_cache_load_result_ = OK;

[wtc, 2014/02/19 22:53:28]

This probably should be initialized to ERR_UNEXPECTED. We should never use the
value we assign here.

Alternatively, just remove this line.

[ramant (doing other things), 2014/02/20 02:34:06]

Done.

+  generation_counter_ = cached->generation_counter();
+  next_state_ = STATE_LOAD_QUIC_SERVER_INFO_COMPLETE;
+
+  // TODO(rtenneti): If multiple tabs load the same URL, all requests except for
+  // the first request send InchoateClientHello. Fix the code to handle multiple
+  // requests. A possible solution is to wait for the first request to finish
+  // and use the data from the disk cache for all requests.
+  // We always call WaitForDataReady, so that we can save server config to disk
+  // cache, though we might have already loaded server config from a canonical
+  // config.

[wtc, 2014/02/19 22:53:28]

This comment ("We always call WaitForDataReady, ...") is hard to understand.

You should point out that quic_server_info->Persist requires quic_server_info to
be ready.

In "we might have already loaded server config from a canonical config", change
"loaded server config" to "initialized |cached|". Change "a canonical config" to
"the cached state for a canonical hostname".

[ramant (doing other things), 2014/02/20 02:34:06]

Done.

+  int rv = quic_server_info->WaitForDataReady(
+      base::Bind(&QuicCryptoClientStream::OnIOComplete,
+                 base::Unretained(this)));
+
+  if (rv != ERR_IO_PENDING) {
+    disk_cache_load_result_ = rv;
+  }
+  return rv;
+}
+
+void QuicCryptoClientStream::DoLoadQuicServerInfoComplete(
+    QuicCryptoClientConfig::CachedState* cached) {
+  next_state_ = STATE_SEND_CHLO;
+
+  // Check if generation_counter has changed between STATE_LOAD_QUIC_SERVER_INFO
+  // and STATE_LOAD_QUIC_SERVER_INFO_COMPLETE state changes.
+  // TODO(rtenneti): Do we need generation_counter_? Could we just call
+  // cached->IsEmpty() (sever_config might have been filled with server config
+  // information from canonical server config)?

[wtc, 2014/02/19 22:53:28]

If |cached| becomes non-empty but the generation counter is the same, there is a
bug. The code that fills |cached| needs to bump the generation counter. I guess
the bug is in CachedState::InitializeFrom?

Once the bug in CachedState::InitializeFrom is fixed, I think it should be
sufficient to just check generation_counter_ != cached->generation_counter().

On the other hand, it seems fine to just check !cached->IsEmpty() because that's
really what we want to check: if someone else already saved a server config, we
don't want to overwrite it. Also, it someone else saved a server config and then
cleared it (so cached->IsEmpty() is true, but the generation counter changed),
we still want to load from QuicServerInfo.

In summary, I think we can just check !cached->IsEmpty() and remove the code
related to generation_counter_ from this method and the DoLoadQuicServerInfo
method.

[ramant (doing other things), 2014/02/20 02:34:06]

Done.

+  if (generation_counter_ != cached->generation_counter() ||
+      !cached->IsEmpty()) {
+    // Someone else has already saved a server config received from the network.

[wtc, 2014/02/19 22:53:28]

This comment should say "from the network or the canonical server config".

[ramant (doing other things), 2014/02/20 02:34:06]

Done.

+    return;
+  }
+
+  if (disk_cache_load_result_ != OK || !cached->LoadQuicServerInfo(
+          session()->connection()->clock()->WallNow())) {
+    // It is ok to proceed to STATE_SEND_CHLO when we cannot load QuicServerInfo
+    // from the disk cache.
+    DCHECK(cached->IsEmpty());
+    DVLOG(1) << "Empty server_config";
+    return;
+  }
+
+  ProofVerifier* verifier = crypto_config_->proof_verifier();
+  if (!verifier) {
+    // If no verifier is set then we don't check the certificates.
+    cached->SetProofValid();
+  } else if (!cached->signature().empty()) {
+    next_state_ = STATE_VERIFY_PROOF;
+  }
+}
+
 }  // namespace net