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

net/quic/crypto/quic_crypto_client_config.cc

Index: net/quic/crypto/quic_crypto_client_config.cc
diff --git a/net/quic/crypto/quic_crypto_client_config.cc b/net/quic/crypto/quic_crypto_client_config.cc
index 876391065330cd78b1259d34c45a63a7994ee870..d18944bf3fe0e3ad1aa290714e1a24195628d6dd 100644
--- a/net/quic/crypto/quic_crypto_client_config.cc
+++ b/net/quic/crypto/quic_crypto_client_config.cc
@@ -5,6 +5,8 @@
 #include "net/quic/crypto/quic_crypto_client_config.h"
 
 #include "base/stl_util.h"
+#include "net/base/completion_callback.h"
+#include "net/base/net_errors.h"
 #include "net/quic/crypto/cert_compressor.h"
 #include "net/quic/crypto/channel_id.h"
 #include "net/quic/crypto/common_cert_set.h"
@@ -38,11 +40,13 @@ QuicCryptoClientConfig::~QuicCryptoClientConfig() {
 
 QuicCryptoClientConfig::CachedState::CachedState()
     : server_config_valid_(false),
+      need_to_persist_(false),
       generation_counter_(0) {}
 
 QuicCryptoClientConfig::CachedState::CachedState(
     scoped_ptr<QuicServerInfo> quic_server_info)
     : server_config_valid_(false),
+      need_to_persist_(false),
       generation_counter_(0),
       quic_server_info_(quic_server_info.Pass()) {}
 
@@ -164,6 +168,7 @@ void QuicCryptoClientConfig::CachedState::SetProofValid() {
 
 void QuicCryptoClientConfig::CachedState::SetProofInvalid() {
   server_config_valid_ = false;
+  need_to_persist_ = true;

[wtc, 2014/02/07 00:54:11]

IMPORTANT: is this a bug? It seems that we should only persist if the proof is
valid, but not if the proof is invalid.

[ramant (doing other things), 2014/02/07 20:30:51]

Added need_to_persist_ = true in SetProof and only when certs have changed.
WDYT?

   ++generation_counter_;
 }
 
@@ -197,6 +202,10 @@ QuicCryptoClientConfig::CachedState::proof_verify_details() const {
   return proof_verify_details_.get();
 }
 
+QuicServerInfo* QuicCryptoClientConfig::CachedState::quic_server_info() const {
+  return quic_server_info_.get();
+}
+
 void QuicCryptoClientConfig::CachedState::set_source_address_token(
     StringPiece token) {
   source_address_token_ = token.as_string();
@@ -218,6 +227,51 @@ void QuicCryptoClientConfig::CachedState::InitializeFrom(
   server_config_valid_ = other.server_config_valid_;
 }
 
+void QuicCryptoClientConfig::CachedState::LoadQuicServerInfo() {
+  if (proof_valid() || !signature().empty()) {

[wtc, 2014/02/07 00:54:11]

1. Why do you test !signature().empty()?

2. Since we are in the same class, we should use the members proof_valid_,
signature_, quic_server_info_ directly, rather than their getter methods.

Please check all the new code you wrote for this issue.

[ramant (doing other things), 2014/02/07 20:30:51]

Made the changes to access members directly.
Was worried about the race condition, that multiple requests for the same URL
were going on at the same time.

For the first request, we are waiting for the data to be read from disk cache.
The second request might have sent CHLO (without certs) and server has sent back
REJ message with certs and we are in the process of verifying proof.

Meanwhile first request has finished reading the data from the disk and trying
to update the data with what it has read.

This change says, we will use the data from disk cache only if the cache object
doesn't have data. What do you think?

+    return;
+  }
+  DCHECK(quic_server_info());
+
+  const QuicServerInfo::State& state(quic_server_info_->state());
+  if (state.certs_.empty()) {
+    return;
+  }
+
+  server_config_ = state.server_config_;
+  source_address_token_ = state.source_address_token_;
+  server_config_sig_ = state.server_config_sig_;
+  for (size_t i = 0; i < state.certs_.size(); i++) {
+    certs_.push_back(state.certs_[i]);
+  }

[wtc, 2014/02/07 00:54:11]

I think we should be able to assign a std::vector directly:

  certs_ = state.certs_;

[ramant (doing other things), 2014/02/07 20:30:51]

Done.

+  need_to_persist_ = false;
+}
+
+void QuicCryptoClientConfig::CachedState::SaveQuicServerInfo() {

[wtc, 2014/02/07 00:54:11]

IMPORTANT: these two methods show a duplication of data in CachedState and
QuicServerInfo. We should eliminate the duplication of data.

This may mean combining CachedState and QuicServerInfo into one. For example,
CachedState can implement the QuicServerInfo interface. Other solutions are
possible.

[ramant (doing other things), 2014/02/07 20:30:51]

Added a TODO for the above. Possible race condition between reading from disk
cache and another request updating the cached state.

+  if (!quic_server_info() || !proof_valid() || signature().empty() ||
+      !need_to_persist_) {

[wtc, 2014/02/07 00:54:11]

It seems that if we set need_to_persist_ appropriately, we just need to test
!need_to_persist_ here.

[ramant (doing other things), 2014/02/07 20:30:51]

Done.

+    return;
+  }
+
+  // If the QuicServerInfo hasn't managed to load from disk yet then we can't
+  // save anything.
+  if (quic_server_info_->WaitForDataReady(CompletionCallback()) != OK) {
+    return;
+  }
+
+  QuicServerInfo::State* state = quic_server_info_->mutable_state();
+
+  state->Clear();
+  state->server_config_ = server_config_;
+  state->source_address_token_ = source_address_token_;
+  state->server_config_sig_ = server_config_sig_;
+  for (size_t i = 0; i < certs_.size(); i++) {
+    state->certs_.push_back(certs_[i]);
+  }

[wtc, 2014/02/07 00:54:11]

An assignment should work:

  state->certs_ = certs_;

[ramant (doing other things), 2014/02/07 20:30:51]

Done.

+
+  quic_server_info_->Persist();
+}
+
 void QuicCryptoClientConfig::SetDefaults() {
   // Key exchange methods.
   kexs.resize(2);