Allways call didReceiveAuthenticationChallenge: completion handler.

ios/web/net/cert_verifier_block_adapter.cc

 // Copyright 2015 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.
 
 #include "ios/web/net/cert_verifier_block_adapter.h"
 
 #include "base/mac/bind_objc_block.h"
 #include "net/base/net_errors.h"
 #include "net/cert/crl_set.h"
 #include "net/cert/x509_certificate.h"
 #include "net/log/net_log.h"
 
 namespace web {
 
 namespace {
 
 // Resource manager which keeps CertVerifyResult, X509Certificate and
 // BoundNetLog alive until verification is completed. Also holds unowned pointer
 // to |net::CertVerifier::Request|.

[Ryan Sleevi, 2015/10/28 18:32:27]

This comment needs updating.

Also, an explainer on why refcounted.

[Eugene But (OOO till 7-30), 2015/10/29 15:43:38]

Done.

 struct VerificationContext
     : public base::RefCountedThreadSafe<VerificationContext> {
   VerificationContext(scoped_refptr<net::X509Certificate> cert,
                       net::NetLog* net_log)
       : request(nullptr),
         cert(cert.Pass()),
         net_log(net::BoundNetLog::Make(
             net_log,
             net::NetLog::SOURCE_IOS_WEB_VIEW_CERT_VERIFIER)) {}
-  // Unowned verification request.
-  net::CertVerifier::Request* request;
+  // Verification request must be alive until verification is completed.

[Ryan Sleevi, 2015/10/28 18:32:27]

I find this comment a bit confusing. You're making a statement about a
requirement, but it's unclear where this requirement is coming from or why.

Put differently, you're not describing what this member is, but why it is, and
that's a bit... odd.

// Stores the current verification request. The request must outlive the
VerificationContext, so that the verification
// request is not cancelled, [because why]

[Eugene But (OOO till 7-30), 2015/10/29 15:43:38]

Done.

+  scoped_ptr<net::CertVerifier::Request> request;
   // The result of certificate verification.
   net::CertVerifyResult result;
   // Certificate being verified.
   scoped_refptr<net::X509Certificate> cert;
   // BoundNetLog required by CertVerifier.
   net::BoundNetLog net_log;
 
  private:
   friend class base::RefCountedThreadSafe<VerificationContext>;
   VerificationContext() = delete;
@@ skipping 26 matching lines @@
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(completion_handler);
   if (!params.cert || params.hostname.empty()) {
     completion_handler(net::CertVerifyResult(), net::ERR_INVALID_ARGUMENT);
     return;
   }
 
   scoped_refptr<VerificationContext> context(
       new VerificationContext(params.cert, net_log_));
   net::CompletionCallback callback = base::BindBlock(^(int error) {
-    // Remove pending request.
-    auto request_iterator = std::find(
-        pending_requests_.begin(), pending_requests_.end(), context->request);
-    DCHECK(pending_requests_.end() != request_iterator);
-    pending_requests_.erase(request_iterator);
-
     completion_handler(context->result, error);
   });
   scoped_ptr<net::CertVerifier::Request> request;
   int error = cert_verifier_->Verify(params.cert.get(), params.hostname,
                                      params.ocsp_response, params.flags,
                                      params.crl_set.get(), &(context->result),
                                      callback, &request, context->net_log);
   if (error == net::ERR_IO_PENDING) {
-    // Make sure that |net::CertVerifier::Request| is alive until either
-    // verification is completed or CertVerifierBlockAdapter is destroyed.
-    pending_requests_.push_back(request.Pass());
-    context->request = pending_requests_.back();
+    // Make sure that |net::CertVerifier::Request| is alive until verification
+    // is completed. It means that cert verification request will not be
+    // cancellable but it will guarantee that |completion_handler| callback is
+    // always called by this method, which is a part of API contract.

[Ryan Sleevi, 2015/10/28 18:32:27]

This still reads a bit... weird.

// Keep the |net::CertVerifier::Request| alive until verification completes.
Because |context| is kept alive by
// |callback| (through base::BindBlock), this means that the cert verification
request cannot be cancelled.
// However, it guarantees that |callback| - and thus |completion_handler| - will
always be called, which is
// a necessary part of the API contract of |CertVerifierBlockAdapter::Verify()|.

[Eugene But (OOO till 7-30), 2015/10/29 15:43:38]

Done.

+    context->request = request.Pass();
     // Completion handler will be called from |callback| when verification
     // request is completed.
     return;
   }
 
   // Verification has either failed or result was retrieved from the cache.
   completion_handler(context->result, error);
 }
 
 }  // namespace web