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

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 bfcab387872dba94b7d97eaaee0704076e90bf9c..ac347dec8de7277c1b4e254d21660e0dffc8c5d3 100644
--- a/net/cert/multi_threaded_cert_verifier.h
+++ b/net/cert/multi_threaded_cert_verifier.h
@@ -5,7 +5,7 @@
 #ifndef NET_CERT_MULTI_THREADED_CERT_VERIFIER_H_
 #define NET_CERT_MULTI_THREADED_CERT_VERIFIER_H_
 
-#include <map>
+#include <set>
 #include <string>
 #include <vector>
 
@@ -61,15 +61,13 @@ class NET_EXPORT_PRIVATE MultiThreadedCertVerifier
              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;
@@ -130,20 +128,19 @@ class NET_EXPORT_PRIVATE MultiThreadedCertVerifier
                     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(); }
@@ -154,12 +151,8 @@ class NET_EXPORT_PRIVATE MultiThreadedCertVerifier
   // 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_;