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..5f5304a6cce9ddd5880536fa30dc80e66ce3d6b4 100644
--- a/net/quic/crypto/quic_crypto_client_config.cc
+++ b/net/quic/crypto/quic_crypto_client_config.cc
@@ -38,11 +38,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()) {}
 
@@ -150,6 +152,7 @@ void QuicCryptoClientConfig::CachedState::SetProof(const vector<string>& certs,
   SetProofInvalid();
   certs_ = certs;
   server_config_sig_ = signature.as_string();
+  need_to_persist_ = true;

[wtc, 2014/02/11 01:01:45]

This doesn't seem like the right place to set need_to_persist to true because
the proof for a server config is optional.

[ramant (doing other things), 2014/02/11 07:57:55]

Done.

 }
 
 void QuicCryptoClientConfig::CachedState::ClearProof() {
@@ -197,6 +200,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 +225,55 @@ void QuicCryptoClientConfig::CachedState::InitializeFrom(
   server_config_valid_ = other.server_config_valid_;
 }
 
+// TODO(rtenneti): LoadQuicServerInfo and SaveQuicServerInfo have duplication of
+// data in CachedState and QuicServerInfo. We should eliminate the duplication
+// of data.
+// An issue to be solved: while we are loading the data from disk cache, it is
+// possible for another request for the same hostname update the CachedState
+// because that request has sent FillInchoateClientHello and got REJ message.
+// Loading of data from disk cache shouldn't blindly overwrite what is in
+// CachedState.
+void QuicCryptoClientConfig::CachedState::LoadQuicServerInfo() {
+  if (server_config_valid_ || !server_config_sig_.empty()) {

[wtc, 2014/02/11 01:01:45]

It seems that we should not test server_config_valid_.

It should be enough to just test !server_config_.empty(). Testing
!server_config_sig_.empty() is not ideal because server config signature is
optional.

[ramant (doing other things), 2014/02/11 07:57:55]

Done.

+    return;
+  }
+  DCHECK(quic_server_info_.get());
+
+  const QuicServerInfo::State& state(quic_server_info_->state());
+  if (state.certs_.empty()) {

[wtc, 2014/02/11 01:01:45]

It is better to test state.server_config_.empty() instead, because state.certs_
may be empty in a valid State (if we didn't request a proof).

[ramant (doing other things), 2014/02/11 07:57:55]

Done.

+    return;
+  }
+
+  server_config_ = state.server_config_;
+  source_address_token_ = state.source_address_token_;
+  server_config_sig_ = state.server_config_sig_;
+  certs_ = state.certs_;
+  need_to_persist_ = false;
+}
+
+void QuicCryptoClientConfig::CachedState::SaveQuicServerInfo() {
+  if (!quic_server_info_.get() || !need_to_persist_) {
+    return;
+  }
+  DCHECK(server_config_valid_);
+
+  // If the QuicServerInfo hasn't managed to load from disk yet then we can't
+  // save anything.
+  if (!quic_server_info_->IsDataReady()) {
+    return;
+  }
+
+  QuicServerInfo::State* state = quic_server_info_->mutable_state();
+
+  state->Clear();

[wtc, 2014/02/11 01:01:45]

This state->Clear() call is not necessary because we are going to set all
members of |state| next.

[ramant (doing other things), 2014/02/11 07:57:55]

Done.

+  state->server_config_ = server_config_;
+  state->source_address_token_ = source_address_token_;
+  state->server_config_sig_ = server_config_sig_;
+  state->certs_ = certs_;
+
+  quic_server_info_->Persist();

[wtc, 2014/02/11 01:01:45]

We should set need_to_persist_ to false after the Persist() call.

[ramant (doing other things), 2014/02/11 07:57:55]

Done.

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