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

net/cert/cert_verifier.h

Index: net/cert/cert_verifier.h
diff --git a/net/cert/cert_verifier.h b/net/cert/cert_verifier.h
index 71a347854272cf537f2e6bc4104d4fdf83556cce..aaab36c83c2daaaf416904bf91a6e2f5e5782be3 100644
--- a/net/cert/cert_verifier.h
+++ b/net/cert/cert_verifier.h
@@ -20,14 +20,14 @@ 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
@@ -99,8 +99,8 @@ class NET_EXPORT CertVerifier {
   // 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,
@@ -110,13 +110,9 @@ class NET_EXPORT CertVerifier {
                      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();