Make CertVerifier provide stronger lifetime guarantees.

CertVerifier today guarantees that if the associated
CertVerifier::Request is deleted, the caller-supplied
callback will not be invoked. However, it does not provide
any guarantees as to when the callback itself will be
freed.

This strengthens the guarantee, such that when a ::Request
is deleted, it will also Reset() the callback. Similarly,
it provides a strong guarantee that if the CertVerifier is
deleted, the associated Callbacks will be Reset. Put
differently, if either (CertVerifier is deleted ||
Request is deleted), then the Callback will be Reset. This
makes it easier for callers to be confident of an
"eventually consistent" approach.

Prior to this change, when the Callback was destroyed was
a bit indeterminate. In some cases, when the underlying
CertVerifier went away, the Callback would be destroyed. In
other cases, it was not until the ::Request was deleted by
the caller. And in one, it was not until both the ::Request
was deleted and the underlying CertVerifier had been
deleted/the asynchronous job completed before the callback
would be destroyed.

Bug: none
Change-Id: I527fa9f3700016269d4236bae03c13510ab69b7b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1862116Reviewed-by: Matthew Denton <mpdenton@chromium.org>
Reviewed-by: Matt Mueller <mattm@chromium.org>
Commit-Queue: Ryan Sleevi <rsleevi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#707472}

net/cert/cert_verifier.h

@@ -151,18 +151,23 @@ class NET_EXPORT CertVerifier {
   // Returns OK if successful or an error code upon failure.
   //
   // The |*verify_result| structure, including the |verify_result->cert_status|
-  // bitmask, is always filled out regardless of the return value.  If the
+  // bitmask, is always filled out regardless of the return value. If the
   // certificate has multiple errors, the corresponding status flags are set in
   // |verify_result->cert_status|, and the error code for the most serious
   // error is returned.
   //
-  // |callback| must not be null.  ERR_IO_PENDING is returned if the operation
+  // |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.
   //
-  // On asynchronous completion (when Verify returns ERR_IO_PENDING) |out_req|
-  // will be reset with a pointer to the request. Freeing this pointer before
-  // the request has completed will cancel it.
+  // |*out_req| is used to store a request handle in the event of asynchronous
+  // completion (when Verify returns ERR_IO_PENDING). Provided that neither
+  // the CertVerifier nor the Request have been deleted, |callback| will be
+  // invoked once the underlying verification finishes. If either the
+  // CertVerifier or the Request are deleted, then |callback| will be Reset()
+  // and will not be invoked. It is fine for |out_req| to outlive the
+  // CertVerifier, and it is fine to reset |out_req| or delete the
+  // CertVerifier during the processing of |callback|.
   //
   // If Verify() completes synchronously then |out_req| *may* be reset to
   // nullptr. However it is not guaranteed that all implementations will reset

net/cert/coalescing_cert_verifier.cc

@@ -354,6 +354,8 @@ CoalescingCertVerifier::Request::~Request() {
 }
 
 void CoalescingCertVerifier::Request::Complete(int result) {
+  DCHECK(job_);  // There must be a pending/non-aborted job to complete.
+
   *verify_result_ = job_->verify_result();
 
   // On successful completion, the Job removes the Request from its set;
@@ -368,12 +370,16 @@ void CoalescingCertVerifier::Request::Complete(int result) {
 }
 
 void CoalescingCertVerifier::Request::OnJobAbort() {
+  DCHECK(job_);  // There must be a pending job to abort.
+
   // If the Job is deleted before the Request, just clean up. The Request will
   // eventually be deleted by the caller.
   net_log_.AddEvent(NetLogEventType::CANCELLED);
   net_log_.EndEvent(NetLogEventType::CERT_VERIFIER_REQUEST);
 
   job_ = nullptr;
+  // Note: May delete |this|, if the caller made |callback_| own the Request.
+  callback_.Reset();
 }
 
 CoalescingCertVerifier::CoalescingCertVerifier(

net/cert/trial_comparison_cert_verifier.cc

@@ -242,11 +242,12 @@ class TrialComparisonCertVerifier::Job::Request : public CertVerifier::Request {
 
   // Called when the Job has completed, and used to invoke the client
   // callback.
-  // Note: |this| may be deleted after calling this.
+  // Note: |this| may be deleted after calling this method.
   void OnJobComplete(int result, const CertVerifyResult& verify_result);
 
   // Called when the Job is aborted (e.g. the underlying
   // TrialComparisonCertVerifier is being deleted).
+  // Note: |this| may be deleted after calling this method.
   void OnJobAborted();
 
  private:
@@ -275,7 +276,9 @@ TrialComparisonCertVerifier::Job::Job(const CertVerifier::Config& config,
 
 TrialComparisonCertVerifier::Job::~Job() {
   if (request_) {
+    // Note: May delete |request_|.
     request_->OnJobAborted();
+    request_ = nullptr;
   }
 
   if (parent_) {
@@ -628,6 +631,9 @@ void TrialComparisonCertVerifier::Job::Request::OnJobComplete(
 void TrialComparisonCertVerifier::Job::Request::OnJobAborted() {
   DCHECK(parent_);
   parent_ = nullptr;
+
+  // DANGER: |this| may be deleted when this callback is destroyed.
+  client_callback_.Reset();
 }
 
 TrialComparisonCertVerifier::TrialComparisonCertVerifier(