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

net/cert/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_CERT_VERIFIER_H_
 #define NET_CERT_CERT_VERIFIER_H_
 
 #include <string>
 
 #include "base/basictypes.h"
 #include "net/base/completion_callback.h"
 #include "net/base/net_export.h"
 
 namespace net {
 
 class BoundNetLog;
 class CertVerifyResult;
 class CRLSet;
 class X509Certificate;
 
 // CertVerifier represents a service for verifying certificates.
 //
-// CertVerifiers can handle multiple requests at a time. A simpler alternative
-// for consumers that only have 1 outstanding request at a time is to create a
-// SingleRequestCertVerifier wrapper around CertVerifier (which will
-// automatically cancel the single request when it goes out of scope).
+// CertVerifiers can handle multiple requests at a time.
 class NET_EXPORT CertVerifier {
  public:
-  // Opaque pointer type used to cancel outstanding requests.
-  typedef void* RequestHandle;
+  class Request {

[pneubeck (no reviews), 2015/05/13 10:13:07]

another thing that I stumbled over:

it's not documented nor disallowed to copy this request. I'd suggest doing the
latter by using the DISALLOW_COPY macro.

[pneubeck (no reviews), 2015/05/13 12:01:20]

and, since the copy is not allowed anymore, I find one use case particularly
painful:

If I maintain a list of pending requests, I want to remove any request from that
list once it finished.
However, since the Request is not available before the Verify() call, it cannot
be bound to the callback.
Before, I could resolve that with a trick, namely store the RequestHandle (ie.
void*) in at a heap allocated memory address and pass that to the callback.

So, i think i have two possible solutions:
- I continue heap allocating a Request* and fill it with the Request's address
after the Verify call

- I maintain my own Handle per request, e.g. a counter and a map<Handle,
Request>. This is very cumbersome...

- You could change the Verify callback to obtain a Request*, which would be the
cleanest solution, IMO.

+   public:
+    // Destruction of the Request cancels it.
+    virtual ~Request() {}
+  };
 
   enum VerifyFlags {
     // If set, enables online revocation checking via CRLs and OCSP for the
     // certificate chain.
     VERIFY_REV_CHECKING_ENABLED = 1 << 0,
 
     // If set, and the certificate being verified may be an EV certificate,
     // attempt to verify the certificate according to the EV processing
     // guidelines. In order to successfully verify a certificate as EV,
     // either an online or offline revocation check must be successfully
@@ skipping 51 matching lines @@
   // VERIFY_REV_CHECKING_ENABLED is not set), EV certificate verification will
   // not be performed.
   //
   // |crl_set| points to an optional CRLSet structure which can be used to
   // avoid revocation checks over the network.
   //
   // |callback| must not be null.  ERR_IO_PENDING is returned if the operation
   // could not be completed synchronously, in which case the result code will
   // be passed to the callback when available.
   //
-  // |*out_req| will be filled with a handle to the async request.
-  // This handle is not valid after the request has completed.
+  // |*out_req| will be filled with a pointer to the asynchronous request.

[pneubeck (no reviews), 2015/05/12 09:20:16]

It seems that it is not guaranteed that out_req will be not null when Verify()
returns.
This descriptions sounds like it however.
Maybe you should state that out_req will only be filled if IO_PENDING is
returned, otherwise it's reset to nullptr.

[eroman, 2015/05/12 14:22:11]

Thanks Phillip! I will address that in a follow-up change as this has already
landed.

+  // Freeing this pointer before the request has completed will cancel it.
   //
   // TODO(rsleevi): Move CRLSet* out of the CertVerifier signature.
   virtual int Verify(X509Certificate* cert,
                      const std::string& hostname,
                      const std::string& ocsp_response,
                      int flags,
                      CRLSet* crl_set,
                      CertVerifyResult* verify_result,
                      const CompletionCallback& callback,
-                     RequestHandle* out_req,
+                     scoped_ptr<Request>* out_req,
                      const BoundNetLog& net_log) = 0;
 
-  // Cancels the specified request. |req| is the handle returned by Verify().
-  // After a request is canceled, its completion callback will not be called.
-  virtual void CancelRequest(RequestHandle req) = 0;
-
   // Returns true if this CertVerifier supports stapled OCSP responses.
   virtual bool SupportsOCSPStapling();
 
   // Creates a CertVerifier implementation that verifies certificates using
   // the preferred underlying cryptographic libraries.
   static CertVerifier* CreateDefault();
 };
 
 }  // namespace net
 
 #endif  // NET_CERT_CERT_VERIFIER_H_