Refactor the API for CertVerifier::Verify() and the implementation of MultiThreadedCertVerifier::Ver

net/cert/multi_threaded_cert_verifier.h

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef NET_CERT_MULTI_THREADED_CERT_VERIFIER_H_
 #define NET_CERT_MULTI_THREADED_CERT_VERIFIER_H_
 
-#include <map>
+#include <set>
 #include <string>
 #include <vector>
 
 #include "base/basictypes.h"
 #include "base/gtest_prod_util.h"
 #include "base/memory/ref_counted.h"
 #include "base/threading/non_thread_safe.h"
 #include "net/base/completion_callback.h"
 #include "net/base/expiring_cache.h"
 #include "net/base/hash_value.h"
@@ skipping 35 matching lines @@
       CertTrustAnchorProvider* trust_anchor_provider);
 
   // CertVerifier implementation
   int Verify(X509Certificate* cert,
              const std::string& hostname,
              const std::string& ocsp_response,
              int flags,
              CRLSet* crl_set,
              CertVerifyResult* verify_result,
              const CompletionCallback& callback,
-             CertVerifier::RequestHandle* out_req,
+             scoped_ptr<Request>* out_req,
              const BoundNetLog& net_log) override;
 
-  void CancelRequest(CertVerifier::RequestHandle req) override;
-
   bool SupportsOCSPStapling() override;
 
  private:
-  friend class CertVerifierWorker;  // Calls HandleResult.
+  struct JobToRequestParamsComparator;
   friend class CertVerifierRequest;
   friend class CertVerifierJob;
   friend class MultiThreadedCertVerifierTest;
   FRIEND_TEST_ALL_PREFIXES(MultiThreadedCertVerifierTest, CacheHit);
   FRIEND_TEST_ALL_PREFIXES(MultiThreadedCertVerifierTest, DifferentCACerts);
   FRIEND_TEST_ALL_PREFIXES(MultiThreadedCertVerifierTest, InflightJoin);
   FRIEND_TEST_ALL_PREFIXES(MultiThreadedCertVerifierTest, CancelRequest);
   FRIEND_TEST_ALL_PREFIXES(MultiThreadedCertVerifierTest,
                            RequestParamsComparators);
   FRIEND_TEST_ALL_PREFIXES(MultiThreadedCertVerifierTest,
@@ skipping 40 matching lines @@
     base::Time verification_time;
     base::Time expiration_time;
   };
 
   struct CacheExpirationFunctor {
     // Returns true iff |now| is within the validity period of |expiration|.
     bool operator()(const CacheValidityPeriod& now,
                     const CacheValidityPeriod& expiration) const;
   };
 
+  using JobSet = std::set<CertVerifierJob*>;

[Ryan Sleevi, 2015/05/06 22:31:48]

I'm not terribly keen on this change. The original implementation stored the
Jobs ordered by their RequestParams, so it was a quick lookup. Your new
implementation sorts the set by the raw pointer values, which then forces a full
scan of the set when trying to find the matching object. This seems
algorithmically suboptimal.

[eroman, 2015/05/08 23:44:16]

Wow, how embarassing :(
Thanks for spotting this!

There is supposed to be a custom comparator for this set (otherwise the other
assumptions with using lower_bound() really don't make sense).

Fixed, and added a unit test that covers this.

+
   typedef ExpiringCache<RequestParams, CachedResult, CacheValidityPeriod,
                         CacheExpirationFunctor> CertVerifierCache;
 
-  void HandleResult(X509Certificate* cert,
-                    const std::string& hostname,
-                    const std::string& ocsp_response,
-                    int flags,
-                    const CertificateList& additional_trust_anchors,
-                    int error,
-                    const CertVerifyResult& verify_result);
+  void SaveResultToCache(const RequestParams& key, const CachedResult& result);

[Ryan Sleevi, 2015/05/06 22:31:48]

Document

[eroman, 2015/05/08 23:44:16]

Done.

 
   // CertDatabase::Observer methods:
   void OnCACertChanged(const X509Certificate* cert) override;
 
+  CertVerifierJob* FindJob(const RequestParams& key);
+  scoped_ptr<CertVerifierJob> RemoveJob(CertVerifierJob* job);

[Ryan Sleevi, 2015/05/06 22:31:48]

Document

[eroman, 2015/05/08 23:44:16]

Done.

+
   // For unit testing.
   void ClearCache() { cache_.Clear(); }
   size_t GetCacheSize() const { return cache_.size(); }
   uint64 cache_hits() const { return cache_hits_; }
   uint64 requests() const { return requests_; }
   uint64 inflight_joins() const { return inflight_joins_; }
 
   // cache_ maps from a request to a cached result.
   CertVerifierCache cache_;
 
-  // inflight_ maps from a request to an active verification which is taking
-  // place.
-  std::map<RequestParams, CertVerifierJob*> inflight_;
-
-  // A non-owning pointer to the first job for histogramming.
-  CertVerifierJob* first_job_;
+  // inflight_ holds the jobs for which an active verification is taking place.
+  JobSet inflight_;
 
   uint64 requests_;
   uint64 cache_hits_;
   uint64 inflight_joins_;
 
   scoped_refptr<CertVerifyProc> verify_proc_;
 
   CertTrustAnchorProvider* trust_anchor_provider_;
 
   DISALLOW_COPY_AND_ASSIGN(MultiThreadedCertVerifier);
 };
 
 }  // namespace net
 
 #endif  // NET_CERT_MULTI_THREADED_CERT_VERIFIER_H_