Cert - protobufs to serialize and deserialize CertVerifierCache.

net/cert/multi_threaded_cert_verifier.h

Index: net/cert/multi_threaded_cert_verifier.h
diff --git a/net/cert/multi_threaded_cert_verifier.h b/net/cert/multi_threaded_cert_verifier.h
index 62d68075483e675bca1a6b6e6776fe3f54b8bc04..b9bb8b28e0035a14b8a0486b40bd9b19da5705e0 100644
--- a/net/cert/multi_threaded_cert_verifier.h
+++ b/net/cert/multi_threaded_cert_verifier.h
@@ -70,8 +70,21 @@ class NET_EXPORT_PRIVATE MultiThreadedCertVerifier
 
   bool SupportsOCSPStapling() override;
 
+  // Adds explicitly-specified data to CertVerifierCache. Returns true if
+  // |cache_| is updated.

[Ryan Sleevi, 2016/04/29 23:33:21]

This is not a helpful comment, because it provides no details about what these
parameters are, how they should be set, or even why this method exists. It goes
directly to talking about implementation details, but that shouldn't be done for
a public header and public method.

// Caches |result| as the result for |hostname|, with the error code
// of |error|, which was previously obtained by calling |Verify()|
// with |flags| at |start_time|, for the certificate whose ordered
// chain was |hash_values|, which was completed at
// |verification_time|, and should expire by |expiration_time|.
// If it returns true, subsequent calls to |Verify()| will return this
// result, if it is before |expiration_time| and matches the
// |hostname| and |flags|.


(If you read the above, you'll see how it's unclear the difference
between |start_time| and |verification_time|. I don't consider the
above comment complete, but mostly wanted to explore how to be clearer
in the comment).

It's also unclear, from this, why you return a bool result, but I'm less picky
about that. Just that it's not clear *why* someone should care about that (just
for metrics?). That one I'm not picky about documenting, other than 

[ramant (doing other things), 2016/04/30 22:58:11]

Many many thanks for the above comments.

Done. 

+  bool AddCertResult(std::string& hostname,
+                     int flags,
+                     std::vector<SHA1HashValue>& hash_values,
+                     base::Time start_time,
+                     int error,
+                     const CertVerifyResult& result,
+                     base::Time verification_time,
+                     base::Time expiration_time);
+
  private:
   struct JobToRequestParamsComparator;
+  friend class CertVerifierCacheIterator;
+  friend class CertVerifierCachePersisterTest;
   friend class CertVerifierRequest;
   friend class CertVerifierJob;
   friend class MultiThreadedCertVerifierTest;
@@ -84,6 +97,9 @@ class NET_EXPORT_PRIVATE MultiThreadedCertVerifier
                            RequestParamsComparators);
   FRIEND_TEST_ALL_PREFIXES(MultiThreadedCertVerifierTest,
                            CertTrustAnchorProvider);
+  FRIEND_TEST_ALL_PREFIXES(CertVerifierCachePersisterTest, PersistCache);
+  FRIEND_TEST_ALL_PREFIXES(CertVerifierCachePersisterTest,
+                           PersistCacheExpiredEntry);

[Ryan Sleevi, 2016/04/29 23:33:21]

This (and line 87) improperly creates the circular dependency, except via tests,
because now this code depends on knowing how it's used. You should be testing
the interface, not the implementation.

[ramant (doing other things), 2016/04/30 22:58:11]

Done.

 
   // Input parameters of a certificate verification request.
   struct NET_EXPORT_PRIVATE RequestParams {
@@ -94,6 +110,10 @@ class NET_EXPORT_PRIVATE MultiThreadedCertVerifier
                   int flags_arg,
                   const CertificateList& additional_trust_anchors);
     RequestParams(const RequestParams& other);
+    RequestParams(std::string& hostname_arg,
+                  int flags_arg,
+                  std::vector<SHA1HashValue>& hash_values_arg,

[Ryan Sleevi, 2016/04/29 23:33:21]

Don't pass by non-const reference. I realize you did this to swap, but you need
to take it explicitly, and std::move.

At your call site, if you're trying to optimize, you should std::move() into
that.

[ramant (doing other things), 2016/04/30 22:58:11]

Done.

+                  base::Time start_time_arg);
     ~RequestParams();
 
     bool operator<(const RequestParams& other) const;
@@ -110,6 +130,7 @@ class NET_EXPORT_PRIVATE MultiThreadedCertVerifier
   // CachedResult contains the result of a certificate verification.
   struct NET_EXPORT_PRIVATE CachedResult {
     CachedResult();
+    CachedResult(int error_arg, CertVerifyResult result_arg);
     ~CachedResult();
 
     int error;  // The return value of CertVerifier::Verify.
@@ -124,7 +145,7 @@ class NET_EXPORT_PRIVATE MultiThreadedCertVerifier
   // ensure that expiration is measured both by the 'general' case (now + cache
   // TTL) and by whether or not significant enough clock skew was introduced
   // since the last verification.
-  struct CacheValidityPeriod {
+  struct NET_EXPORT_PRIVATE CacheValidityPeriod {

[Ryan Sleevi, 2016/04/29 23:33:21]

Why?

[ramant (doing other things), 2016/04/30 22:58:11]

Deleted them. Changed the tests to test the interface and not the
implementation.

     explicit CacheValidityPeriod(const base::Time& now);
     CacheValidityPeriod(const base::Time& now, const base::Time& expiration);
 
@@ -132,7 +153,7 @@ class NET_EXPORT_PRIVATE MultiThreadedCertVerifier
     base::Time expiration_time;
   };
 
-  struct CacheExpirationFunctor {
+  struct NET_EXPORT_PRIVATE CacheExpirationFunctor {

[Ryan Sleevi, 2016/04/29 23:33:21]

Why?

[ramant (doing other things), 2016/04/30 22:58:11]

Deleted them. Changed the tests to test the interface and not the
implementation.

     // Returns true iff |now| is within the validity period of |expiration|.
     bool operator()(const CacheValidityPeriod& now,
                     const CacheValidityPeriod& expiration) const;
@@ -145,8 +166,11 @@ class NET_EXPORT_PRIVATE MultiThreadedCertVerifier
 
   using JobSet = std::set<CertVerifierJob*, JobComparator>;
 
-  typedef ExpiringCache<RequestParams, CachedResult, CacheValidityPeriod,
-                        CacheExpirationFunctor> CertVerifierCache;
+  typedef ExpiringCache<RequestParams,
+                        CachedResult,
+                        CacheValidityPeriod,
+                        CacheExpirationFunctor>
+      CertVerifierCache;
 
   // Saves |result| into the cache, keyed by |key|.
   void SaveResultToCache(const RequestParams& key, const CachedResult& result);
@@ -186,6 +210,35 @@ class NET_EXPORT_PRIVATE MultiThreadedCertVerifier
   DISALLOW_COPY_AND_ASSIGN(MultiThreadedCertVerifier);
 };
 
+class NET_EXPORT_PRIVATE CertVerifierCacheIterator {

[Ryan Sleevi, 2016/04/29 23:33:21]

I suggested several times that this be a member.

MultiThreadedCertVerifier::Iterator

To be more explicit: CertVerifierCacheIterator is extremely confusing with
CertVerifierCache::Iterator, which this is not, and has no fundamental relation
to.

[ramant (doing other things), 2016/04/30 22:58:11]

Sincere apologies. Undid this change.

Was trying to avoid forward declaration of struct's required by
CertVerifierCache typedef. 

Done.

+ public:
+  explicit CertVerifierCacheIterator(const MultiThreadedCertVerifier& verifier);
+  ~CertVerifierCacheIterator();
+
+  bool HasNext() const { return iterator_.HasNext(); }
+  void Advance() { iterator_.Advance(); }
+
+  const std::string& hostname() const { return iterator_.key().hostname; }
+  int flags() const { return iterator_.key().flags; }
+  const std::vector<SHA1HashValue>& hash_values() const {
+    return iterator_.key().hash_values;
+  }
+  const base::Time& start_time() const { return iterator_.key().start_time; }
+  int error() const { return iterator_.value().error; }
+  const CertVerifyResult& result() const { return iterator_.value().result; }
+  const base::Time& verification_time() const {
+    return iterator_.expiration().verification_time;
+  }
+  const base::Time& expiration_time() const {
+    return iterator_.expiration().expiration_time;
+  }
+
+ private:
+  MultiThreadedCertVerifier::CertVerifierCache::Iterator iterator_;
+
+  DISALLOW_COPY_AND_ASSIGN(CertVerifierCacheIterator);
+};
+
 }  // namespace net
 
 #endif  // NET_CERT_MULTI_THREADED_CERT_VERIFIER_H_