Remove dependence on a message loop for net::PathBuilder.

net/cert/internal/path_builder.cc

Index: net/cert/internal/path_builder.cc
diff --git a/net/cert/internal/path_builder.cc b/net/cert/internal/path_builder.cc
index 36cd9d45f8d97bf2c5ae27fbec47cb9bda3ee4f4..46ff13d56a785985b71306e23161cbd81ceb90fb 100644
--- a/net/cert/internal/path_builder.cc
+++ b/net/cert/internal/path_builder.cc
@@ -7,7 +7,6 @@
 #include <set>
 #include <unordered_set>
 
-#include "base/callback_helpers.h"
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
 #include "net/base/net_errors.h"
@@ -71,24 +70,17 @@ class CertIssuersIter {
                   CertIssuerSources* cert_issuer_sources,
                   const TrustStore* trust_store);
 
-  // Gets the next candidate issuer. If an issuer is ready synchronously, SYNC
-  // is returned and the cert is stored in |*cert|.  If an issuer is not
-  // ready, ASYNC is returned and |callback| will be called once |*out_cert| has
-  // been set. If |callback| is null, always completes synchronously.
-  //
-  // In either case, if all issuers have been exhausted, |*out| is cleared.
-  CompletionStatus GetNextIssuer(CertificateOrTrustAnchor* out,
-                                 const base::Closure& callback);
+  // Gets the next candidate issuer, or clears |*out| when all issuers have been
+  // exhausted.
+  void GetNextIssuer(CertificateOrTrustAnchor* out);
 
   // Returns the |cert| for which issuers are being retrieved.
   const ParsedCertificate* cert() const { return cert_.get(); }
   scoped_refptr<ParsedCertificate> reference_cert() const { return cert_; }
 
  private:
+  void AddIssuers(ParsedCertificateList issuers);
   void DoAsyncIssuerQuery();
-  void GotAsyncAnchors(TrustAnchors anchors);
-  void GotAsyncCerts(CertIssuerSource::Request* request);
-  void NotifyIfNecessary();
 
   scoped_refptr<ParsedCertificate> cert_;
   CertIssuerSources* cert_issuer_sources_;
@@ -119,19 +111,12 @@ class CertIssuersIter {
   // Tracks which requests have been made yet.
   bool did_initial_query_ = false;
   bool did_async_issuer_query_ = false;
-  // If asynchronous requests were made, how many of them are still outstanding?
-  size_t pending_async_results_;
+  // If asynchronous requests were made, which ones are still outstanding?

[mattm, 2016/10/28 02:11:55]

comment is a bit confusing / indirect. maybe mention it is an index into
pending_async_requests_, and that it is next outstanding one to process, or
something?

[eroman, 2016/11/23 22:10:21]

Done.

+  size_t cur_async_request_ = 0;
   // Owns the Request objects for any asynchronous requests so that they will be
   // cancelled if CertIssuersIter is destroyed.
   std::vector<std::unique_ptr<CertIssuerSource::Request>>
       pending_async_requests_;
-  std::unique_ptr<TrustStore::Request> pending_anchor_request_;
-
-  // When GetNextIssuer was called and returned asynchronously, |*out_| is
-  // where the result will be stored, and |callback_| will be run when the
-  // result is ready.
-  CertificateOrTrustAnchor* out_;
-  base::Closure callback_;
 
   DISALLOW_COPY_AND_ASSIGN(CertIssuersIter);
 };
@@ -145,30 +130,15 @@ CertIssuersIter::CertIssuersIter(scoped_refptr<ParsedCertificate> in_cert,
   DVLOG(1) << "CertIssuersIter(" << CertDebugString(cert()) << ") created";
 }
 
-CompletionStatus CertIssuersIter::GetNextIssuer(CertificateOrTrustAnchor* out,
-                                                const base::Closure& callback) {
-  // Should not be called again while already waiting for an async result.
-  DCHECK(callback_.is_null());
-
+void CertIssuersIter::GetNextIssuer(CertificateOrTrustAnchor* out) {
   if (!did_initial_query_) {
     did_initial_query_ = true;
-    trust_store_->FindTrustAnchorsForCert(
-        cert_,
-        callback.is_null() ? TrustStore::TrustAnchorsCallback()
-                           : base::Bind(&CertIssuersIter::GotAsyncAnchors,
-                                        base::Unretained(this)),
-        &anchors_, &pending_anchor_request_);
+    trust_store_->FindTrustAnchorsForCert(cert_, &anchors_);
 
     for (auto* cert_issuer_source : *cert_issuer_sources_) {
       ParsedCertificateList new_issuers;
       cert_issuer_source->SyncGetIssuersOf(cert(), &new_issuers);
-      for (scoped_refptr<ParsedCertificate>& issuer : new_issuers) {
-        if (present_issuers_.find(issuer->der_cert().AsStringPiece()) !=
-            present_issuers_.end())
-          continue;
-        present_issuers_.insert(issuer->der_cert().AsStringPiece());
-        issuers_.push_back(std::move(issuer));
-      }
+      AddIssuers(std::move(new_issuers));
     }
     DVLOG(1) << anchors_.size() << " sync anchors, " << issuers_.size()
              << " sync issuers";
@@ -186,15 +156,31 @@ CompletionStatus CertIssuersIter::GetNextIssuer(CertificateOrTrustAnchor* out,
              << anchors_.size();
     // Still have anchors that haven't been returned yet, return one of them.
     *out = CertificateOrTrustAnchor(anchors_[cur_anchor_++]);
-    return CompletionStatus::SYNC;
+    return;
   }
 
-  if (pending_anchor_request_) {
-    DVLOG(1) << "CertIssuersIter(" << CertDebugString(cert())
-             << ") Still waiting for async trust anchor results.";
-    out_ = out;
-    callback_ = callback;
-    return CompletionStatus::ASYNC;
+  // If there aren't any issuers left, block until async results are ready.
+  if (cur_issuer_ >= issuers_.size()) {
+    if (!did_async_issuer_query_) {
+      // Now issue request(s) for async ones (AIA, etc).
+      DoAsyncIssuerQuery();
+    }
+
+    // TODO(eroman): Rather than blocking on the async requests in FIFO order,
+    // consume in the order they become ready.
+    while (cur_async_request_ < pending_async_requests_.size()) {
+      ParsedCertificateList new_issuers;
+      pending_async_requests_[cur_async_request_]->GetNext(&new_issuers);
+      if (new_issuers.empty()) {
+        // Request is exhausted, no more results pending from that
+        // CertIssuerSource.
+        pending_async_requests_[cur_async_request_++].reset();
+        continue;
+      }
+
+      AddIssuers(std::move(new_issuers));
+      break;
+    }
   }
 
   if (cur_issuer_ < issuers_.size()) {
@@ -205,148 +191,40 @@ CompletionStatus CertIssuersIter::GetNextIssuer(CertificateOrTrustAnchor* out,
     // A reference to the returned issuer is retained, since |present_issuers_|
     // points to data owned by it.
     *out = CertificateOrTrustAnchor(issuers_[cur_issuer_++]);
-    return CompletionStatus::SYNC;
-  }
-
-  if (did_async_issuer_query_) {
-    if (pending_async_results_ == 0) {
-      DVLOG(1) << "CertIssuersIter(" << CertDebugString(cert())
-               << ") Reached the end of all available issuers.";
-      // Reached the end of all available issuers.
-      *out = CertificateOrTrustAnchor();
-      return CompletionStatus::SYNC;
-    }
-
-    DVLOG(1) << "CertIssuersIter(" << CertDebugString(cert())
-             << ") Still waiting for async results from other "
-                "CertIssuerSources.";
-    // Still waiting for async results from other CertIssuerSources.
-    out_ = out;
-    callback_ = callback;
-    return CompletionStatus::ASYNC;
+    return;
   }
-  // Reached the end of synchronously gathered issuers.
 
-  if (callback.is_null()) {
-    // Synchronous-only mode, don't try to query async sources.
-    *out = CertificateOrTrustAnchor();
-    return CompletionStatus::SYNC;
-  }
-
-  // Now issue request(s) for async ones (AIA, etc).
-  DoAsyncIssuerQuery();
+  DVLOG(1) << "CertIssuersIter(" << CertDebugString(cert())
+           << ") Reached the end of all available issuers.";
+  // Reached the end of all available issuers.
+  *out = CertificateOrTrustAnchor();
+}
 
-  if (pending_async_results_ == 0) {
-    DVLOG(1) << "CertIssuersIter(" << CertDebugString(cert())
-             << ") No cert sources have async results.";
-    // No cert sources have async results.
-    *out = CertificateOrTrustAnchor();
-    return CompletionStatus::SYNC;
+void CertIssuersIter::AddIssuers(ParsedCertificateList new_issuers) {
+  for (scoped_refptr<ParsedCertificate>& issuer : new_issuers) {
+    if (present_issuers_.find(issuer->der_cert().AsStringPiece()) !=
+        present_issuers_.end())
+      continue;
+    present_issuers_.insert(issuer->der_cert().AsStringPiece());
+    issuers_.push_back(std::move(issuer));
   }
-
-  DVLOG(1) << "CertIssuersIter(" << CertDebugString(cert())
-           << ") issued AsyncGetIssuersOf call(s) (n=" << pending_async_results_
-           << ")";
-  out_ = out;
-  callback_ = callback;
-  return CompletionStatus::ASYNC;
 }
 
 void CertIssuersIter::DoAsyncIssuerQuery() {
   DCHECK(!did_async_issuer_query_);
   did_async_issuer_query_ = true;
-  pending_async_results_ = 0;
+  cur_async_request_ = 0;
   for (auto* cert_issuer_source : *cert_issuer_sources_) {
     std::unique_ptr<CertIssuerSource::Request> request;
-    cert_issuer_source->AsyncGetIssuersOf(
-        cert(),
-        base::Bind(&CertIssuersIter::GotAsyncCerts, base::Unretained(this)),
-        &request);
+    cert_issuer_source->AsyncGetIssuersOf(cert(), &request);
     if (request) {
       DVLOG(1) << "AsyncGetIssuersOf(" << CertDebugString(cert())
                << ") pending...";
-      pending_async_results_++;
       pending_async_requests_.push_back(std::move(request));
     }
   }
 }
 
-void CertIssuersIter::GotAsyncAnchors(TrustAnchors anchors) {
-  DVLOG(1) << "CertIssuersIter::GotAsyncAnchors(" << CertDebugString(cert())
-           << "): " << anchors.size() << " anchors";
-  for (scoped_refptr<TrustAnchor>& anchor : anchors)
-    anchors_.push_back(std::move(anchor));
-  pending_anchor_request_.reset();
-
-  NotifyIfNecessary();
-}
-
-void CertIssuersIter::GotAsyncCerts(CertIssuerSource::Request* request) {
-  DVLOG(1) << "CertIssuersIter::GotAsyncCerts(" << CertDebugString(cert())
-           << ")";
-  while (true) {
-    scoped_refptr<ParsedCertificate> cert;
-    CompletionStatus status = request->GetNext(&cert);
-    if (!cert) {
-      if (status == CompletionStatus::SYNC) {
-        // Request is exhausted, no more results pending from that
-        // CertIssuerSource.
-        DCHECK_GT(pending_async_results_, 0U);
-        pending_async_results_--;
-      }
-      break;
-    }
-    DCHECK_EQ(status, CompletionStatus::SYNC);
-    if (present_issuers_.find(cert->der_cert().AsStringPiece()) !=
-        present_issuers_.end())
-      continue;
-    present_issuers_.insert(cert->der_cert().AsStringPiece());
-    issuers_.push_back(std::move(cert));
-  }
-
-  // TODO(mattm): re-sort remaining elements of issuers_ (remaining elements may
-  // be more than the ones just inserted, depending on |cur_| value).
-
-  NotifyIfNecessary();
-}
-
-void CertIssuersIter::NotifyIfNecessary() {
-  // Notify that more results are available, if necessary.
-  if (!callback_.is_null()) {
-    if (cur_anchor_ < anchors_.size()) {
-      DVLOG(1) << "CertIssuersIter(" << CertDebugString(cert())
-               << "): async returning anchor " << cur_anchor_ << " of "
-               << anchors_.size();
-      *out_ = CertificateOrTrustAnchor(std::move(anchors_[cur_anchor_++]));
-      base::ResetAndReturn(&callback_).Run();
-      return;
-    }
-    if (cur_issuer_ < issuers_.size()) {
-      DCHECK(!pending_anchor_request_);
-      DVLOG(1) << "CertIssuersIter(" << CertDebugString(cert())
-               << "): async returning issuer " << cur_issuer_ << " of "
-               << issuers_.size();
-      *out_ = CertificateOrTrustAnchor(std::move(issuers_[cur_issuer_++]));
-      base::ResetAndReturn(&callback_).Run();
-      return;
-    }
-
-    if (!did_async_issuer_query_)
-      DoAsyncIssuerQuery();
-
-    if (pending_async_results_ == 0) {
-      DVLOG(1) << "CertIssuersIter(" << CertDebugString(cert())
-               << "): async returning empty result";
-      *out_ = CertificateOrTrustAnchor();
-      base::ResetAndReturn(&callback_).Run();
-      return;
-    }
-    DVLOG(1) << "CertIssuersIter(" << CertDebugString(cert())
-             << "): empty result, but other async results "
-                "pending, waiting..";
-  }
-}
-
 // CertIssuerIterPath tracks which certs are present in the path and prevents
 // paths from being built which repeat any certs (including different versions
 // of the same cert, based on Subject+SubjectAltName+SPKI).
@@ -446,11 +324,9 @@ class CertPathIter {
   // CertPathIter.
   void AddCertIssuerSource(CertIssuerSource* cert_issuer_source);
 
-  // Gets the next candidate path. If a path is ready synchronously, SYNC is
-  // returned and the path is stored in |*path|.  If a path is not ready,
-  // ASYNC is returned and |callback| will be called once |*path| has been set.
-  // In either case, if all paths have been exhausted, |*path| is cleared.
-  CompletionStatus GetNextPath(CertPath* path, const base::Closure& callback);
+  // Gets the next candidate path, or clears |*path| when all paths have been
+  // exhausted.
+  void GetNextPath(CertPath* path);
 
  private:
   enum State {
@@ -461,13 +337,9 @@ class CertPathIter {
     STATE_BACKTRACK,
   };
 
-  CompletionStatus DoLoop(bool allow_async);
-
-  CompletionStatus DoGetNextIssuer(bool allow_async);
-  CompletionStatus DoGetNextIssuerComplete();
-  CompletionStatus DoBackTrack();
-
-  void HandleGotNextIssuer(void);
+  void DoGetNextIssuer();
+  void DoGetNextIssuerComplete();
+  void DoBackTrack();
 
   // Stores the next candidate issuer, until it is used during the
   // STATE_GET_NEXT_ISSUER_COMPLETE step.
@@ -483,8 +355,6 @@ class CertPathIter {
   // The output variable for storing the next candidate path, which the client
   // passes in to GetNextPath. Only used for a single path output.
   CertPath* out_path_;
-  // The callback to be called if an async lookup generated a candidate path.
-  base::Closure callback_;
   // Current state of the state machine.
   State next_state_;
 
@@ -501,22 +371,10 @@ void CertPathIter::AddCertIssuerSource(CertIssuerSource* cert_issuer_source) {
   cert_issuer_sources_.push_back(cert_issuer_source);
 }
 
-CompletionStatus CertPathIter::GetNextPath(CertPath* path,
-                                           const base::Closure& callback) {
+// TODO(eroman): Simplify (doesn't need to use the "DoLoop" pattern).
+void CertPathIter::GetNextPath(CertPath* path) {
   out_path_ = path;
   out_path_->Clear();
-  CompletionStatus rv = DoLoop(!callback.is_null());
-  if (rv == CompletionStatus::ASYNC) {
-    callback_ = callback;
-  } else {
-    // Clear the reference to the output parameter as a precaution.
-    out_path_ = nullptr;
-  }
-  return rv;
-}
-
-CompletionStatus CertPathIter::DoLoop(bool allow_async) {
-  CompletionStatus result = CompletionStatus::SYNC;
   do {
     State state = next_state_;
     next_state_ = STATE_NONE;
@@ -525,39 +383,32 @@ CompletionStatus CertPathIter::DoLoop(bool allow_async) {
         NOTREACHED();
         break;
       case STATE_GET_NEXT_ISSUER:
-        result = DoGetNextIssuer(allow_async);
+        DoGetNextIssuer();
         break;
       case STATE_GET_NEXT_ISSUER_COMPLETE:
-        result = DoGetNextIssuerComplete();
+        DoGetNextIssuerComplete();
         break;
       case STATE_RETURN_A_PATH:
         // If the returned path did not verify, keep looking for other paths
         // (the trust root is not part of cur_path_, so don't need to
         // backtrack).
         next_state_ = STATE_GET_NEXT_ISSUER;
-        result = CompletionStatus::SYNC;
         break;
       case STATE_BACKTRACK:
-        result = DoBackTrack();
+        DoBackTrack();
         break;
     }
-  } while (result == CompletionStatus::SYNC && next_state_ != STATE_NONE &&
-           next_state_ != STATE_RETURN_A_PATH);
+  } while (next_state_ != STATE_NONE && next_state_ != STATE_RETURN_A_PATH);
 
-  return result;
+  out_path_ = nullptr;
 }
 
-CompletionStatus CertPathIter::DoGetNextIssuer(bool allow_async) {
+void CertPathIter::DoGetNextIssuer() {
   next_state_ = STATE_GET_NEXT_ISSUER_COMPLETE;
-  CompletionStatus rv = cur_path_.back()->GetNextIssuer(
-      &next_issuer_, allow_async
-                         ? base::Bind(&CertPathIter::HandleGotNextIssuer,
-                                      base::Unretained(this))
-                         : base::Closure());
-  return rv;
+  cur_path_.back()->GetNextIssuer(&next_issuer_);
 }
 
-CompletionStatus CertPathIter::DoGetNextIssuerComplete() {
+void CertPathIter::DoGetNextIssuerComplete() {
   // If the issuer is a trust anchor signal readiness.
   if (next_issuer_.IsTrustAnchor()) {
     DVLOG(1) << "CertPathIter got anchor("
@@ -566,14 +417,14 @@ CompletionStatus CertPathIter::DoGetNextIssuerComplete() {
     cur_path_.CopyPath(&out_path_->certs);
     out_path_->trust_anchor = std::move(next_issuer_.anchor);
     next_issuer_ = CertificateOrTrustAnchor();
-    return CompletionStatus::SYNC;
+    return;
   }
 
   if (next_issuer_.IsCertificate()) {
     // Skip this cert if it is already in the chain.
     if (cur_path_.IsPresent(next_issuer_.cert.get())) {
       next_state_ = STATE_GET_NEXT_ISSUER;
-      return CompletionStatus::SYNC;
+      return;
     }
 
     cur_path_.Append(base::MakeUnique<CertIssuersIter>(
@@ -590,10 +441,9 @@ CompletionStatus CertPathIter::DoGetNextIssuerComplete() {
     // more for the previous cert.
     next_state_ = STATE_BACKTRACK;
   }
-  return CompletionStatus::SYNC;
 }
 
-CompletionStatus CertPathIter::DoBackTrack() {
+void CertPathIter::DoBackTrack() {
   DVLOG(1) << "CertPathIter backtracking...";
   cur_path_.Pop();
   if (cur_path_.Empty()) {
@@ -603,17 +453,6 @@ CompletionStatus CertPathIter::DoBackTrack() {
     // Continue exploring issuers of the previous path.
     next_state_ = STATE_GET_NEXT_ISSUER;
   }
-  return CompletionStatus::SYNC;
-}
-
-void CertPathIter::HandleGotNextIssuer(void) {
-  DCHECK(!callback_.is_null());
-  CompletionStatus rv = DoLoop(true /* allow_async */);
-  if (rv == CompletionStatus::SYNC) {
-    // Clear the reference to the output parameter as a precaution.
-    out_path_ = nullptr;
-    base::ResetAndReturn(&callback_).Run();
-  }
 }
 
 CertPathBuilder::ResultPath::ResultPath() = default;
@@ -658,19 +497,10 @@ void CertPathBuilder::AddCertIssuerSource(
   cert_path_iter_->AddCertIssuerSource(cert_issuer_source);
 }
 
-CompletionStatus CertPathBuilder::Run(const base::Closure& callback) {
+// TODO(eroman): Simplify (doesn't need to use the "DoLoop" pattern).
+void CertPathBuilder::Run() {
   DCHECK_EQ(STATE_NONE, next_state_);
   next_state_ = STATE_GET_NEXT_PATH;
-  CompletionStatus rv = DoLoop(!callback.is_null());
-
-  if (rv == CompletionStatus::ASYNC)
-    callback_ = callback;
-
-  return rv;
-}
-
-CompletionStatus CertPathBuilder::DoLoop(bool allow_async) {
-  CompletionStatus result = CompletionStatus::SYNC;
 
   do {
     State state = next_state_;
@@ -680,38 +510,25 @@ CompletionStatus CertPathBuilder::DoLoop(bool allow_async) {
         NOTREACHED();
         break;
       case STATE_GET_NEXT_PATH:
-        result = DoGetNextPath(allow_async);
+        DoGetNextPath();
         break;
       case STATE_GET_NEXT_PATH_COMPLETE:
-        result = DoGetNextPathComplete();
+        DoGetNextPathComplete();
         break;
     }
-  } while (result == CompletionStatus::SYNC && next_state_ != STATE_NONE);
-
-  return result;
+  } while (next_state_ != STATE_NONE);
 }
 
-CompletionStatus CertPathBuilder::DoGetNextPath(bool allow_async) {
+void CertPathBuilder::DoGetNextPath() {
   next_state_ = STATE_GET_NEXT_PATH_COMPLETE;
-  CompletionStatus rv = cert_path_iter_->GetNextPath(
-      &next_path_, allow_async ? base::Bind(&CertPathBuilder::HandleGotNextPath,
-                                            base::Unretained(this))
-                               : base::Closure());
-  return rv;
-}
-
-void CertPathBuilder::HandleGotNextPath() {
-  DCHECK(!callback_.is_null());
-  CompletionStatus rv = DoLoop(true /* allow_async */);
-  if (rv == CompletionStatus::SYNC)
-    base::ResetAndReturn(&callback_).Run();
+  cert_path_iter_->GetNextPath(&next_path_);
 }
 
-CompletionStatus CertPathBuilder::DoGetNextPathComplete() {
+void CertPathBuilder::DoGetNextPathComplete() {
   if (next_path_.IsEmpty()) {
     // No more paths to check, signal completion.
     next_state_ = STATE_NONE;
-    return CompletionStatus::SYNC;
+    return;
   }
 
   // Verify the entire certificate chain.
@@ -729,14 +546,13 @@ CompletionStatus CertPathBuilder::DoGetNextPathComplete() {
     // Found a valid path, return immediately.
     // TODO(mattm): add debug/test mode that tries all possible paths.
     next_state_ = STATE_NONE;
-    return CompletionStatus::SYNC;
+    return;
   }
 
   // Path did not verify. Try more paths. If there are no more paths, the result
   // will be returned next time DoGetNextPathComplete is called with next_path_
   // empty.
   next_state_ = STATE_GET_NEXT_PATH;
-  return CompletionStatus::SYNC;
 }
 
 void CertPathBuilder::AddResultPath(std::unique_ptr<ResultPath> result_path) {