[ios] Removed CertVerifierBlockAdapter.

ios/web/net/crw_cert_verification_controller.mm

Index: ios/web/net/crw_cert_verification_controller.mm
diff --git a/ios/web/net/crw_cert_verification_controller.mm b/ios/web/net/crw_cert_verification_controller.mm
index ac202064be94a0a740abbf9ab3fc23cbcd1d2ae1..3e427e0dab5fc7cecbebc159d872fff9eec20477 100644
--- a/ios/web/net/crw_cert_verification_controller.mm
+++ b/ios/web/net/crw_cert_verification_controller.mm
@@ -9,194 +9,52 @@
 #include "base/ios/block_types.h"
 #include "base/logging.h"
 #include "base/mac/bind_objc_block.h"
-#include "base/mac/scoped_block.h"
 #import "base/memory/ref_counted.h"
-#include "base/metrics/histogram_macros.h"
-#include "base/rand_util.h"
 #include "base/strings/sys_string_conversions.h"
 #include "base/threading/worker_pool.h"
-#include "ios/web/net/cert_verifier_block_adapter.h"
 #include "ios/web/public/browser_state.h"
 #include "ios/web/public/certificate_policy_cache.h"
 #include "ios/web/public/web_thread.h"
 #import "ios/web/web_state/wk_web_view_security_util.h"
-#include "net/cert/cert_verify_result.h"
-#include "net/url_request/url_request_context.h"
-#include "net/url_request/url_request_context_getter.h"
-
-namespace {
-
-// Enum for Web.CertVerifyAgreement UMA metric to report certificate
-// verification mismatch between SecTrust API and CertVerifier. SecTrust API is
-// used for making load/no-load decision and CertVerifier is used for getting
-// the reason of verification failure. It is expected that mismatches will
-// happen for those 2 approaches (e.g. SecTrust API accepts the cert but
-// CertVerifier rejects it). This metric helps to understand how common
-// mismatches are.
-// Note: This enum is used to back an UMA histogram, and should be treated as
-// append-only.
-enum CertVerifyAgreement {
-  // There is no mismach. Both SecTrust API and CertVerifier accepted this cert.
-  CERT_VERIFY_AGREEMENT_ACCEPTED_BY_BOTH = 0,
-  // There is no mismach. Both SecTrust API and CertVerifier rejected this cert.
-  CERT_VERIFY_AGREEMENT_REJECTED_BY_BOTH,
-  // SecTrust API accepted the cert, but CertVerifier rejected it (e.g. MDM cert
-  // or CertVerifier was more strict during verification), this mismach is
-  // expected to be common because of MDM certs.
-  CERT_VERIFY_AGREEMENT_ACCEPTED_ONLY_BY_IOS,
-  // SecTrust API rejected the cert, but CertVerifier accepted it. This mismatch
-  // is expected to be uncommon.
-  CERT_VERIFY_AGREEMENT_ACCEPTED_ONLY_BY_NSS,
-  CERT_VERIFY_AGREEMENT_COUNT,
-};
-
-// This class takes ownership of block and releases it on UI thread, even if
-// |BlockHolder| is destructed on a background thread. On destruction calls its
-// block with default arguments, if block was not called before. This way
-// BlockHolder guarantees that block is always called to satisfy API contract
-// for CRWCertVerificationController (completion handlers are always called).
-template <class T>
-class BlockHolder : public base::RefCountedThreadSafe<BlockHolder<T>> {
- public:
-  // Takes ownership of |block|, which must not be null. On destruction calls
-  // this block with |DefaultArgs|, if block was not called before.
-  // |DefaultArgs| must be passed by value, not by ponter or reference.
-  template <typename... Arguments>
-  BlockHolder(T block, Arguments... DefaultArgs)
-      : block_([block copy]),
-        called_(false),
-        default_block_([^{
-          block(DefaultArgs...);
-        } copy]) {
-    DCHECK(block_);
-  }
-
-  // Calls underlying block with the given variadic arguments.
-  template <typename... Arguments>
-  void call(Arguments... Args) {
-    block_(Args...);
-    called_ = true;
-  }
-
- private:
-  BlockHolder() = delete;
-  friend class base::RefCountedThreadSafe<BlockHolder>;
-
-  // Finalizes object's destruction on UI thread by calling |default_block| (if
-  // |block| has not been called yet) and releasing all blocks. Must be called
-  // on UI thread.
-  static void Finalize(id block,
-                       ProceduralBlock default_block,
-                       bool block_was_called) {
-    DCHECK_CURRENTLY_ON(web::WebThread::UI);
-    // By calling default_block, BlockHolder guarantees that block is always
-    // called to satisfy API contract for CRWCertVerificationController
-    // (completion handlers are always called).
-    if (!block_was_called)
-      default_block();
-    [block release];
-    [default_block release];
-  }
-
-  // Releases underlying |block_| on UI thread, calls |default_block_| if
-  // |block_| has not been called yet.
-  ~BlockHolder() {
-    if (web::WebThread::CurrentlyOn(web::WebThread::UI)) {
-      Finalize(block_, default_block_, called_);
-    } else {
-      web::WebThread::PostTask(
-          web::WebThread::UI, FROM_HERE,
-          base::Bind(&BlockHolder::Finalize, block_, default_block_, called_));
-    }
-  }
-
-  T block_;
-  // true if this |block_| has already been called.
-  bool called_;
-  // Called on destruction if |block_| was not called.
-  ProceduralBlock default_block_;
-};
-
-typedef scoped_refptr<BlockHolder<web::PolicyDecisionHandler>>
-    PolicyDecisionHandlerHolder;
-
-}  // namespace
+#include "net/cert/cert_verify_proc_ios.h"
 
 @interface CRWCertVerificationController () {
-  // Cert verification object which wraps |net::CertVerifier|. Must be created,
-  // used and destroyed on IO Thread.
-  std::unique_ptr<web::CertVerifierBlockAdapter> _certVerifier;
-
-  // URLRequestContextGetter for obtaining net layer objects.
-  net::URLRequestContextGetter* _contextGetter;
-
   // Used to remember user exceptions to invalid certs.
   scoped_refptr<web::CertificatePolicyCache> _certPolicyCache;
 }
 
-// Cert verification flags. Must be used on IO Thread.
-@property(nonatomic, readonly) int certVerifyFlags;
-
-// Returns YES if CertVerifier should be run (even if SecTrust API considers
-// cert as valid) and Web.CertVerifyAgreement UMA metric should be reported.
-// The result of this method is random and nondeterministic.
-- (BOOL)shouldReportCertVerifyAgreement;
-
-// Reports Web.CertVerifyAgreement UMA metric.
-- (void)reportUMAForCertVerifyAgreement:(CertVerifyAgreement)agreement;
-
-// Creates _certVerifier object on IO thread.
-- (void)createCertVerifier;
+// Returns cert status for the given |trust|.
+- (net::CertStatus)certStatusFromTrustResult:(SecTrustResultType)trustResult
+                                 serverTrust:
+                                     (base::ScopedCFTypeRef<SecTrustRef>)trust;
 
 // Decides the policy for the given |trust| which was rejected by iOS and the
-// given |host| and calls |handler| on completion.
+// given |host| and calls |handler| on completion. Must be called on UI thread.
+// |handler| can not be null and will be called on UI thread.
 - (void)
 decideLoadPolicyForRejectedTrustResult:(SecTrustResultType)trustResult
                            serverTrust:(base::ScopedCFTypeRef<SecTrustRef>)trust
                                   host:(NSString*)host
-                     completionHandler:(PolicyDecisionHandlerHolder)handler;
-
-// Decides the policy for the given |trust| which was accepted by iOS and the
-// given |host| and calls |handler| on completion.
-- (void)
-decideLoadPolicyForAcceptedTrustResult:(SecTrustResultType)trustResult
-                           serverTrust:(base::ScopedCFTypeRef<SecTrustRef>)trust
-                                  host:(NSString*)host
-                     completionHandler:(PolicyDecisionHandlerHolder)handler;
-
-// Verifies the given |cert| for the given |host| using |net::CertVerifier| and
-// calls |completionHandler| on completion. This method can be called on any
-// thread. |completionHandler| cannot be null and will be called asynchronously
-// on IO thread or will never be called if IO task can't start or complete.
-- (void)verifyCert:(const scoped_refptr<net::X509Certificate>&)cert
-              forHost:(NSString*)host
-    completionHandler:(void (^)(net::CertVerifyResult))completionHandler;
+                     completionHandler:(web::PolicyDecisionHandler)handler;
 
 // Verifies the given |trust| using SecTrustRef API. |completionHandler| cannot
-// be null and will be either called asynchronously on Worker thread or will
-// never be called if the worker task can't start or complete.
+// be null and will be called on UI thread or never be called if the worker task
+// can't start or complete. Must be called on UI thread.
 - (void)verifyTrust:(base::ScopedCFTypeRef<SecTrustRef>)trust
     completionHandler:(void (^)(SecTrustResultType))completionHandler;
 
 // Returns cert accept policy for the given SecTrust result. |trustResult| must
 // not be for a valid cert. Must be called on IO thread.

[Ryan Sleevi, 2016/08/12 00:16:23]

Just trying to make sure I understand why the threading complexity remains:

1) CertPolicyCache is constructed on the UI thread, owned by the UI thread (line
70)
2) It's refcounted so it can be shared with both (bleh)
3) Its only use is in the RequestTrackerImpl (Is that code even still used?)

Because the CPC needs to only be touched on the IO thread, we have
loadPolicyForRejectedTrust (IO) and decideLoadPolicyForRejectedTrustResult (UI)
to marshal around?

A big part of my challenge here has been trying to understand the code in its
new form, and understand how much we need.

[Eugene But (OOO till 7-30), 2016/08/12 16:16:39]

Correct CertPolicyCache is constructed and owned by UI thread. Its methods can
only be called on IO thread though and that's why we still have threading
complexity.

RequestTrackerImpl was created for UIWebView and we plan to get rid of it but it
will not happen soon. Also CertPolicyCache is used in other places like TabModel
(downstream) and CRWSessionCertificatePolicyManager (upstream). 

 - (web::CertAcceptPolicy)
-    loadPolicyForRejectedTrustResult:(SecTrustResultType)trustResult
-                  certVerifierResult:(net::CertVerifyResult)certVerifierResult
-                         serverTrust:(SecTrustRef)trust
-                                host:(NSString*)host;
+loadPolicyForRejectedTrustResult:(SecTrustResultType)trustResult
+                      certStatus:(net::CertStatus)certStatus
+                     serverTrust:(SecTrustRef)trust
+                            host:(NSString*)host;
 
 @end
 
 @implementation CRWCertVerificationController
 
-#pragma mark - Superclass
-
-- (void)dealloc {
-  DCHECK(!_certVerifier);
-  [super dealloc];
-}
-
 #pragma mark - Public
 
 - (instancetype)init {
@@ -209,12 +67,8 @@ decideLoadPolicyForAcceptedTrustResult:(SecTrustResultType)trustResult
   DCHECK_CURRENTLY_ON(web::WebThread::UI);
   self = [super init];
   if (self) {
-    _contextGetter = browserState->GetRequestContext();
-    DCHECK(_contextGetter);
     _certPolicyCache =
         web::BrowserState::GetCertificatePolicyCache(browserState);
-
-    [self createCertVerifier];
   }
   return self;
 }
@@ -223,30 +77,20 @@ decideLoadPolicyForAcceptedTrustResult:(SecTrustResultType)trustResult
                             host:(NSString*)host
                completionHandler:(web::PolicyDecisionHandler)completionHandler {
   DCHECK_CURRENTLY_ON(web::WebThread::UI);
-  // completionHandler of |verifyCert:forHost:completionHandler:| is called on
-  // IO thread and then bounces back to UI thread. As a result all objects
-  // captured by completionHandler may be released on either UI or IO thread.
-  // Since |completionHandler| can potentially capture multiple thread unsafe
-  // objects (like Web Controller) |completionHandler| itself should never be
-  // released on background thread and |BlockHolder| ensures that.
-  __block PolicyDecisionHandlerHolder handlerHolder(
-      new BlockHolder<web::PolicyDecisionHandler>(
-          completionHandler, web::CERT_ACCEPT_POLICY_NON_RECOVERABLE_ERROR,
-          net::CertStatus()));
+  DCHECK(completionHandler);
+
   [self verifyTrust:trust
       completionHandler:^(SecTrustResultType trustResult) {
+        DCHECK_CURRENTLY_ON(web::WebThread::UI);
         if (web::GetSecurityStyleFromTrustResult(trustResult) ==
             web::SECURITY_STYLE_AUTHENTICATED) {

[Ryan Sleevi, 2016/08/12 00:16:23]

COMMENT: It's unclear to me why we indirect through SECURITY_STYLE here, as
opposed to using the kSecTrustResult directory.

[Eugene But (OOO till 7-30), 2016/08/12 16:16:39]

|web::GetSecurityStyleFromTrustResult(trustResult) ==
web::SECURITY_STYLE_AUTHENTICATED| is the indication that cert is good. Using
|trustResult == kSecTrustResultProceed || trustResult ==
kSecTrustResultUnspecified| everywhere is worse alternative (f.e. what if Apple
add third option which indicate good cert?).

[Ryan Sleevi, 2016/08/12 16:28:33]

Right, I understood that part perfectly :)
This is what I was trying to understand - the reasoning behind not using
trustResult directly. Apple won't change it because of the API committment,
because it's explicitly documented these are the only two good ones. Your
hypothetical third status would still require Chrome code change, but it seems
you're arguing that it would only require changing one place, rather than every
place dealing with it?

The reason this stands out to me is that, at least in Chrome, SecurityStyle is a
//content abstraction, but trust results are a //net abstraction, and //net sits
under //content. It feels "weird" to me, in the sense that it feels like a //net
layer (cert verification) is reaching up to the UI layer (SecurityStyle) to
figure out how to trust a certificate.

[Eugene But (OOO till 7-30), 2016/08/12 22:33:02]

Yes, that's my reasoning.
Thanks, now I understand your concern. I'm split on this and it's hard for me to
choose between maintainability and layering consistency. But I see you have a
stronger preference which makes my choice easier.

Done.

-          [self decideLoadPolicyForAcceptedTrustResult:trustResult
-                                           serverTrust:trust
-                                                  host:host
-                                     completionHandler:handlerHolder];
-        } else {
-          [self decideLoadPolicyForRejectedTrustResult:trustResult
-                                           serverTrust:trust
-                                                  host:host
-                                     completionHandler:handlerHolder];
+          completionHandler(web::CERT_ACCEPT_POLICY_ALLOW, net::CertStatus());
+          return;
         }
+        [self decideLoadPolicyForRejectedTrustResult:trustResult
+                                         serverTrust:trust
+                                                host:host
+                                   completionHandler:completionHandler];
       }];
 }
 
@@ -254,39 +98,16 @@ decideLoadPolicyForAcceptedTrustResult:(SecTrustResultType)trustResult
                           host:(NSString*)host
              completionHandler:(web::StatusQueryHandler)completionHandler {
   DCHECK_CURRENTLY_ON(web::WebThread::UI);
+  DCHECK(completionHandler);
 
-  // The completion handlers of |verifyCert:forHost:completionHandler:| and
-  // |verifyTrust:completionHandler:| will be deallocated on background thread.
-  // |completionHandler| itself should never be released on background thread
-  // and |BlockHolder| ensures that.
-  __block scoped_refptr<BlockHolder<web::StatusQueryHandler>> handlerHolder(
-      new BlockHolder<web::StatusQueryHandler>(
-          completionHandler, web::SECURITY_STYLE_UNKNOWN, net::CertStatus()));
   [self verifyTrust:trust
       completionHandler:^(SecTrustResultType trustResult) {
         web::SecurityStyle securityStyle =
             web::GetSecurityStyleFromTrustResult(trustResult);
-        if (securityStyle == web::SECURITY_STYLE_AUTHENTICATED) {
-          // SecTrust API considers this cert as valid.
-          dispatch_async(dispatch_get_main_queue(), ^{
-            handlerHolder->call(securityStyle, net::CertStatus());
-          });
-          return;
-        }
 
-        // Retrieve the net::CertStatus for invalid certificates to determine
-        // the rejection reason, it is possible that rejection reason could not
-        // be determined and |cert_status| will be empty.
-        scoped_refptr<net::X509Certificate> cert(
-            web::CreateCertFromTrust(trust));
-        [self verifyCert:cert
-                      forHost:host
-            completionHandler:^(net::CertVerifyResult certVerifierResult) {
-              dispatch_async(dispatch_get_main_queue(), ^{
-                handlerHolder->call(securityStyle,
-                                    certVerifierResult.cert_status);
-              });
-            }];
+        net::CertStatus certStatus =
+            [self certStatusFromTrustResult:trustResult serverTrust:trust];
+        completionHandler(securityStyle, certStatus);
       }];
 }
 
@@ -310,144 +131,57 @@ decideLoadPolicyForAcceptedTrustResult:(SecTrustResultType)trustResult
   }));
 }
 
-- (void)shutDown {
-  DCHECK_CURRENTLY_ON(web::WebThread::UI);
-  web::WebThread::PostTask(web::WebThread::IO, FROM_HERE, base::BindBlock(^{
-    // This block captures |self| delaying its deallocation and causing dealloc
-    // to happen on either IO or UI thread (which is fine for this class).
-    _certVerifier.reset();
-  }));
-}
-
 #pragma mark - Private
 
-- (int)certVerifyFlags {
-  DCHECK(web::WebThread::CurrentlyOn(web::WebThread::IO));
-  DCHECK(_contextGetter);
-  // |net::URLRequestContextGetter| lifetime is expected to be at least the same
-  // or longer than |BrowserState| lifetime.
-  net::URLRequestContext* context = _contextGetter->GetURLRequestContext();
-  DCHECK(context);
-  net::SSLConfigService* SSLConfigService = context->ssl_config_service();
-  DCHECK(SSLConfigService);
-  net::SSLConfig config;
-  SSLConfigService->GetSSLConfig(&config);
-  return config.GetCertVerifyFlags();
-}
-
-- (BOOL)shouldReportCertVerifyAgreement {
-  // Cert verification is an expensive operation. Since extra verification will
-  // be used only for UMA reporting purposes, do that only for 1% of cases.
-  return base::RandGenerator(100) == 0;
-}
-
-- (void)reportUMAForCertVerifyAgreement:(CertVerifyAgreement)agreement {
-  UMA_HISTOGRAM_ENUMERATION("Web.CertVerifyAgreement", agreement,
-                            CERT_VERIFY_AGREEMENT_COUNT);
-}
-
-- (void)createCertVerifier {
-  web::WebThread::PostTask(web::WebThread::IO, FROM_HERE, base::BindBlock(^{
-    net::URLRequestContext* context = _contextGetter->GetURLRequestContext();
-    _certVerifier.reset(new web::CertVerifierBlockAdapter(
-        context->cert_verifier(), context->net_log()));
-  }));
+- (net::CertStatus)certStatusFromTrustResult:(SecTrustResultType)trustResult
+                                 serverTrust:
+                                     (base::ScopedCFTypeRef<SecTrustRef>)trust {
+  web::SecurityStyle securityStyle =
+      web::GetSecurityStyleFromTrustResult(trustResult);
+  if (securityStyle == web::SECURITY_STYLE_AUTHENTICATED)

[Ryan Sleevi, 2016/08/12 00:16:22]

It's a little weird that you check securityStyle here (which is a mapped value
from trustResult) but then use trustResult directly (line 144)

For me at least, using trustResult directly seemed to make more sense, but am I
missing something?

[Eugene But (OOO till 7-30), 2016/08/12 16:16:39]

I didn't want to use |trustResult| here because if would require code like this:
|trustResult == kSecTrustResultProceed || trustResult ==
kSecTrustResultUnspecified|

which is less maintainable than using |GetSecurityStyleFromTrustResult|.

[Ryan Sleevi, 2016/08/12 16:28:33]

I guess I disagree (see the other comment :P) Ultimately the layering feels
weird here, and that's tripping me up when I try to map this code onto how we've
structured the Chromium code, but it's not something I'd push back on this CL
for, just as something to consider (again, with the previous comment)

[Eugene But (OOO till 7-30), 2016/08/12 22:33:02]

Done.

+    return net::CertStatus();
+
+  if (trustResult == kSecTrustResultDeny)
+    return net::CERT_STATUS_AUTHORITY_INVALID;

[Ryan Sleevi, 2016/08/12 00:16:23]

Having this short-circuit *seems* like it means we'd show less information if
the user affirmatively rejected a cert. Doesn't CertVerifyProciOS already handle
this?

[Eugene But (OOO till 7-30), 2016/08/12 16:16:39]

Yes, this is from CertVerifyProciOS
https://cs.chromium.org/chromium/src/net/cert/cert_verify_proc_ios.cc?q=case+...

Do you think I should extract lines 266-282 from cert_verify_proc_ios.cc to a
separate method and use it here?

[Ryan Sleevi, 2016/08/12 16:28:33]

In general, I would strongly discourage extracting when you just have two
consumers. In general, I'm a subscriber of the Rule of Three (
https://en.wikipedia.org/wiki/Rule_of_three_(computer_programming) ) -
repetition can lead to significantly more maintainable code, as
counter-intuitive as that is, because it reduces your dependency graph and
allows you to reason about the impact of a change.

And it's not the same as what's in CVPIOS. CVPIOS ors the result - so it
guarantees you'll *at least* set AUTHORITY_INVALID, but may set additional
stuff.

To match what CVPIOS does, you should do

net::CertStatus certStatus;
if (trustResult == kSecTrustResultDeny)
  certStatus |= CERT_STATUS_AUTHORITY_INVALID;
certStatus |= net::CertVerifyProcIOS::GetCertFailureStatusFromTrust(trust);
if (!net::IsCertStatusError(certStatus)) {
  certStatus |= net::CERT_STATUS_INVALID;
}
return certStatus;

[Eugene But (OOO till 7-30), 2016/08/12 22:33:02]

CVPIOS has a switch-case so I matched that.

+
+  net::CertStatus certStatus =
+      net::CertVerifyProcIOS::GetCertFailureStatusFromTrust(trust);
+  if (!net::IsCertStatusError(certStatus)) {
+    // |cert_status| must not be no-error if SecTrust API considers the
+    // cert as invalid. Otherwise there will be issues with errors
+    // reporting and recovery.
+    certStatus = net::CERT_STATUS_INVALID;
+  }
+  return certStatus;
 }
 
 - (void)
 decideLoadPolicyForRejectedTrustResult:(SecTrustResultType)trustResult
                            serverTrust:(base::ScopedCFTypeRef<SecTrustRef>)trust
                                   host:(NSString*)host
-                     completionHandler:(PolicyDecisionHandlerHolder)handler {
-  // SecTrust API considers this cert as invalid. Check the reason and
-  // whether or not user has decided to proceed with this bad cert.
-  scoped_refptr<net::X509Certificate> cert(web::CreateCertFromTrust(trust));
-  [self verifyCert:cert
-                forHost:host
-      completionHandler:^(net::CertVerifyResult certVerifierResult) {
-        if (!net::IsCertStatusError(certVerifierResult.cert_status)) {
-          // |cert_status| must not be no-error if SecTrust API considers the
-          // cert as invalid. Otherwise there will be issues with errors
-          // reporting and recovery.
-          certVerifierResult.cert_status = net::CERT_STATUS_INVALID;
-        }
+                     completionHandler:(web::PolicyDecisionHandler)handler {
+  DCHECK_CURRENTLY_ON(web::WebThread::UI);
+  DCHECK(handler);
+  web::WebThread::PostTask(
+      web::WebThread::IO, FROM_HERE, base::BindBlock(^{

[Ryan Sleevi, 2016/08/12 00:16:22]

Making sure I've grokked the reasoning for this:

The whole reason to thread-hop to the IO thread is because you need to check the
cert_policy_cache (in loadPolicyForRejectedTrustResult), right?

[Eugene But (OOO till 7-30), 2016/08/12 16:16:39]

Correct. I added comments to the code.

+        net::CertStatus certStatus =
+            [self certStatusFromTrustResult:trustResult serverTrust:trust];
 
         web::CertAcceptPolicy policy =
             [self loadPolicyForRejectedTrustResult:trustResult
-                                certVerifierResult:certVerifierResult
+                                        certStatus:certStatus
                                        serverTrust:trust.get()
                                               host:host];
 
         dispatch_async(dispatch_get_main_queue(), ^{
-          if ([self shouldReportCertVerifyAgreement]) {
-            CertVerifyAgreement agreement =
-                net::IsCertStatusError(certVerifierResult.cert_status)
-                    ? CERT_VERIFY_AGREEMENT_REJECTED_BY_BOTH
-                    : CERT_VERIFY_AGREEMENT_ACCEPTED_ONLY_BY_NSS;
-            [self reportUMAForCertVerifyAgreement:agreement];
-          }
-          handler->call(policy, certVerifierResult.cert_status);
+          handler(policy, certStatus);
         });
-      }];
-}
-
-- (void)
-decideLoadPolicyForAcceptedTrustResult:(SecTrustResultType)trustResult
-                           serverTrust:(base::ScopedCFTypeRef<SecTrustRef>)trust
-                                  host:(NSString*)host
-                     completionHandler:(PolicyDecisionHandlerHolder)handler {
-  // SecTrust API considers this cert as valid.
-  dispatch_async(dispatch_get_main_queue(), ^{
-    handler->call(web::CERT_ACCEPT_POLICY_ALLOW, net::CertStatus());
-  });
-
-  if ([self shouldReportCertVerifyAgreement]) {
-    // Execute CertVerifier::Verify to collect CertVerifyAgreement UMA.
-    scoped_refptr<net::X509Certificate> cert(web::CreateCertFromTrust(trust));
-    [self verifyCert:cert
-                  forHost:host
-        completionHandler:^(net::CertVerifyResult certVerifierResult) {
-          // SecTrust API accepted this cert and |PolicyDecisionHandler| has
-          // been called already. |CertVerifier| verification is executed only
-          // to collect CertVerifyAgreement UMA.
-          dispatch_async(dispatch_get_main_queue(), ^{
-            CertVerifyAgreement agreement =
-                net::IsCertStatusError(certVerifierResult.cert_status)
-                    ? CERT_VERIFY_AGREEMENT_ACCEPTED_ONLY_BY_IOS
-                    : CERT_VERIFY_AGREEMENT_ACCEPTED_BY_BOTH;
-            [self reportUMAForCertVerifyAgreement:agreement];
-          });
-        }];
-  }
-}
-
-- (void)verifyCert:(const scoped_refptr<net::X509Certificate>&)cert
-              forHost:(NSString*)host
-    completionHandler:(void (^)(net::CertVerifyResult))completionHandler {
-  DCHECK(completionHandler);
-  __block scoped_refptr<net::X509Certificate> blockCert = cert;
-  web::WebThread::PostTask(web::WebThread::IO, FROM_HERE, base::BindBlock(^{
-    // WeakNSObject does not work across different threads, hence this block
-    // retains self.
-    if (!_certVerifier) {
-      completionHandler(net::CertVerifyResult());
-      return;
-    }
-
-    web::CertVerifierBlockAdapter::Params params(std::move(blockCert),
-                                                 base::SysNSStringToUTF8(host));
-    params.flags = self.certVerifyFlags;
-    // OCSP response is not provided by iOS API.
-    // CRLSets are not used, as the OS is used to make load/no-load
-    // decisions, not the CertVerifier.
-    _certVerifier->Verify(params, ^(net::CertVerifyResult result, int) {
-      completionHandler(result);
-    });
-  }));
+      }));
 }
 
 - (void)verifyTrust:(base::ScopedCFTypeRef<SecTrustRef>)trust
     completionHandler:(void (^)(SecTrustResultType))completionHandler {
+  DCHECK_CURRENTLY_ON(web::WebThread::UI);
   DCHECK(completionHandler);
   // SecTrustEvaluate performs trust evaluation synchronously, possibly making
   // network requests. The UI thread should not be blocked by that operation.
@@ -456,15 +190,17 @@ decideLoadPolicyForAcceptedTrustResult:(SecTrustResultType)trustResult
     if (SecTrustEvaluate(trust.get(), &trustResult) != errSecSuccess) {

[Ryan Sleevi, 2016/08/12 00:16:23]

Curious: Is there any reason we don't/didn't use SecTrustEvaluateAsync here?
Seems like it could avoid the base::WorkerPool requirement

[Eugene But (OOO till 7-30), 2016/08/12 16:16:39]

SecTrustEvaluateAsync takes GCD queue as param and we want to use Chromium
threading primitives instead of GCD.

[Ryan Sleevi, 2016/08/12 16:28:33]

That feels like a very strange response to me, considering lines 193-195. I
suppose there's some nuance here between that code (and all the blocks being
taken here throughout this file) and GCD, but as a non-main iOS developer, it
seems very inconsistent.

[Eugene But (OOO till 7-30), 2016/08/12 22:33:02]

|dispatch_async(dispatch_get_main_queue()| is equivalent to
|web::WebThread::PostTask(web::WebThread::UI, FROM_HERE, base::BindBlock(^{|,
both run block on UI thread. GCD queues are different from threads (different
queues may share the same thread). But consistency is a fair point, so I won't
use |dispatch_async(dispatch_get_main_queue()| in this file.

       trustResult = kSecTrustResultInvalid;
     }
-    completionHandler(trustResult);
+    dispatch_async(dispatch_get_main_queue(), ^{
+      completionHandler(trustResult);
+    });
   }), false /* task_is_slow */);
 }
 
 - (web::CertAcceptPolicy)
-    loadPolicyForRejectedTrustResult:(SecTrustResultType)trustResult
-                  certVerifierResult:(net::CertVerifyResult)certVerifierResult
-                         serverTrust:(SecTrustRef)trust
-                                host:(NSString*)host {
+loadPolicyForRejectedTrustResult:(SecTrustResultType)trustResult
+                      certStatus:(net::CertStatus)certStatus
+                     serverTrust:(SecTrustRef)trust
+                            host:(NSString*)host {
   DCHECK_CURRENTLY_ON(web::WebThread::IO);
   DCHECK_NE(web::SECURITY_STYLE_AUTHENTICATED,
             web::GetSecurityStyleFromTrustResult(trustResult));
@@ -482,8 +218,7 @@ decideLoadPolicyForAcceptedTrustResult:(SecTrustResultType)trustResult
           net::X509Certificate::OSCertHandles());
 
   web::CertPolicy::Judgment judgment = _certPolicyCache->QueryPolicy(
-      leafCert.get(), base::SysNSStringToUTF8(host),
-      certVerifierResult.cert_status);
+      leafCert.get(), base::SysNSStringToUTF8(host), certStatus);
 
   return (judgment == web::CertPolicy::ALLOWED)
              ? web::CERT_ACCEPT_POLICY_RECOVERABLE_ERROR_ACCEPTED_BY_USER