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

net/quic/crypto/quic_server_info.cc

Index: net/quic/crypto/quic_server_info.cc
diff --git a/net/quic/crypto/quic_server_info.cc b/net/quic/crypto/quic_server_info.cc
index 25cb9192b7ec423f67d6d7a5378ecf17c979826b..fe76df0e5e8e9861a2e614e181e8fee5bc431de1 100644
--- a/net/quic/crypto/quic_server_info.cc
+++ b/net/quic/crypto/quic_server_info.cc
@@ -6,7 +6,12 @@
 
 #include "base/bind.h"

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

I think this header can be removed.

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

Done.

 #include "base/pickle.h"
-#include "base/strings/string_piece.h"
+
+using std::string;

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

I think we should remove this "using std::string" statement because this file is
only used in Chromium.

If you think we should add "using std::string", then we should remove all the
"std::" from "std::string" in this file.

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

Done.

+
+namespace {
+const size_t kQuicCryptoConfigVersion = 1;
+}  // namespace

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

1. Nit: kQuicCryptoConfigVersion can be of a smaller type, such as int or
unsigned int. It will be pickled as an int, so I think int is appropriate.

2. Nit: add blank lines, like this:

namespace {

const size_t kQuicCryptoConfigVersion = 1;

} // namespace

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

Done.

 
 namespace net {
 
@@ -15,13 +20,15 @@ QuicServerInfo::State::State() {}
 QuicServerInfo::State::~State() {}
 
 void QuicServerInfo::State::Clear() {
-  data.clear();
+  server_config_.clear();
+  source_address_token_.clear();
+  server_config_sig_.clear();
+  certs_.clear();
 }
 
-// TODO(rtenneti): Flesh out the details.
-QuicServerInfo::QuicServerInfo(
-    const std::string& hostname)
+QuicServerInfo::QuicServerInfo(const std::string& hostname)
     : hostname_(hostname),
+      data_loaded_(false),
       weak_factory_(this) {
 }
 
@@ -40,6 +47,7 @@ bool QuicServerInfo::Parse(const std::string& data) {
   State* state = mutable_state();
 
   state->Clear();
+  data_loaded_ = false;

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

I recommend moving this to the beginning of ParseInner. This way, by looking at
only the ParseInner function, we know when we set data_loaded_ to false and when
we set it to true.

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

Done.

 
   bool r = ParseInner(data);
   if (!r)
@@ -48,11 +56,51 @@ bool QuicServerInfo::Parse(const std::string& data) {
 }
 
 bool QuicServerInfo::ParseInner(const std::string& data) {
-  // TODO(rtenneti): restore QuicCryptoClientConfig::CachedState.
-  // State* state = mutable_state();
+  State* state = mutable_state();
 
-  // Pickle p(data.data(), data.size());
-  // PickleIterator iter(p);
+  // No data was read from the disk cache.
+  if (data.empty()) {
+    return false;
+  }
+
+  Pickle p(data.data(), data.size());
+  PickleIterator iter(p);
+
+  int version = -1;
+  if (!p.ReadInt(&iter, &version) || version < 0) {

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

You should check version != kQuicCryptoConfigVersion instead of (or in addition
to) version < 0. We only support kQuicCryptoConfigVersion.

version != kQuicCryptoConfigVersion should get a different log message
"Unsupported version".

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

Done.

+    DVLOG(1) << "Missing version";
+    return false;
+  }
+
+  if (!p.ReadString(&iter, &state->server_config_)) {
+    DVLOG(1) << "Malformed server_config";
+    return false;
+  }
+  if (!p.ReadString(&iter, &state->source_address_token_)) {
+    DVLOG(1) << "Malformed source_address_token";
+    return false;
+  }
+  if (!p.ReadString(&iter, &state->server_config_sig_)) {
+    DVLOG(1) << "Malformed server_config_sig";
+    return false;
+  }
+
+  // Read certs.
+  int num_certs;
+  if (!p.ReadInt(&iter, &num_certs) || num_certs < 0) {
+    DVLOG(1) << "Malformed num_certs";
+    return false;
+  }
+
+  for (int i = 0; i < num_certs; i++) {
+    string cert;
+    if (!p.ReadString(&iter, &cert)) {
+      DVLOG(1) << "Malformed cert";
+      return false;
+    }
+    state->certs_.push_back(cert);
+  }
+  data_loaded_ = true;
 
   return true;
 }
@@ -60,8 +108,21 @@ bool QuicServerInfo::ParseInner(const std::string& data) {
 std::string QuicServerInfo::Serialize() const {
   Pickle p(sizeof(Pickle::Header));
 
-  // TODO(rtenneti): serialize QuicCryptoClientConfig::CachedState.
-  return std::string(reinterpret_cast<const char *>(p.data()), p.size());
+  if (!p.WriteInt(kQuicCryptoConfigVersion) ||
+      !p.WriteString(state_.server_config_) ||
+      !p.WriteString(state_.source_address_token_) ||
+      !p.WriteString(state_.server_config_sig_) ||
+      !p.WriteInt(state_.certs_.size())) {

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

This probably should be p.WriteUInt32 since state_.certs_.size() is unsigned. It
would also be nice to check for overflow:

    ...
    state_.certs_.size() > std::numeric_limits<uint32>::max() ||
    !p.WriteUint32(state_.certs_.size())) {

If we use WriteInt, then we should check:

    ...
    state_.certs_.size() > std::numeric_limits<int>::max() ||
    !p.WriteInt(state_.certs_.size())) {
	
Need to include <limits> for std::numeric_limits.

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

Done.

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

Done.

+    return "";

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

Nit: it may be better to return std::string().

+  }
+
+  for (size_t i = 0; i < state_.certs_.size(); i++) {
+    if (!p.WriteString(state_.certs_[i])) {
+      return "";
+    }
+  }
+
+  return string(reinterpret_cast<const char *>(p.data()), p.size());
 }
 
 QuicServerInfoFactory::~QuicServerInfoFactory() {}