Always call didReceiveAuthenticationChallenge: completion handler.

iOS throws an exception (which leads to a crash) if didReceiveAuthenticationChallenge:
completion handler is deallocated but never called. Crash was happening
because CertVerifierBlockAdapter did not call its completion handler
if CertVerifierAdapter was destroyed during cert verification (may
happen when Tab is closed during the load).

Changed CertVerifierBlockAdapter to always call its Verify completion
handler by making net::CertVerifier::Request non-cancelable, so each
verification request will complete.

This change does not extend a lifetime of WKWebView (retaining its completion handler
does not retain WKWebView itself).

BUG=541484

Review URL: https://codereview.chromium.org/1392143003

Cr-Commit-Position: refs/heads/master@{#357014}

3 files changed, 43 insertions, 20 deletions

diff --git a/ios/web/net/cert_verifier_block_adapter.cc b/ios/web/net/cert_verifier_block_adapter.cc
index b56b414..a43429a 100644
--- a/ios/web/net/cert_verifier_block_adapter.cc
+++ b/ios/web/net/cert_verifier_block_adapter.cc
@@ -14,9 +14,10 @@ namespace web {
 
 namespace {
 
-// Resource manager which keeps CertVerifyResult, X509Certificate and
-// BoundNetLog alive until verification is completed. Also holds unowned pointer
-// to |net::CertVerifier::Request|.
+// Resource manager which keeps CertVerifyResult, X509Certificate,
+// CertVerifier::Request and BoundNetLog alive until verification is completed.
+// This class is refcounted so it can be captured by a block, keeping its
+// members alive.
 struct VerificationContext
     : public base::RefCountedThreadSafe<VerificationContext> {
   VerificationContext(scoped_refptr<net::X509Certificate> cert,
@@ -26,8 +27,13 @@ struct VerificationContext
         net_log(net::BoundNetLog::Make(
             net_log,
             net::NetLog::SOURCE_IOS_WEB_VIEW_CERT_VERIFIER)) {}
-  // Unowned verification request.
-  net::CertVerifier::Request* request;
+
+  // Stores the current verification request. The request must outlive the
+  // VerificationContext and the CertVerifierBlockAdapter, so that the
+  // verification request is not cancelled. CertVerifierBlockAdapter::Verify
+  // guarantees its completion handler to be called, which will not happen if
+  // verification request is cancelled.
+  scoped_ptr<net::CertVerifier::Request> request;
   // The result of certificate verification.
   net::CertVerifyResult result;
   // Certificate being verified.
@@ -74,12 +80,6 @@ void CertVerifierBlockAdapter::Verify(
   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;
@@ -88,10 +88,13 @@ void CertVerifierBlockAdapter::Verify(
                                      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();
+    // 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()|.
+    context->request = request.Pass();
     // Completion handler will be called from |callback| when verification
     // request is completed.
     return;
diff --git a/ios/web/net/cert_verifier_block_adapter.h b/ios/web/net/cert_verifier_block_adapter.h
index 2646f91..e2d4732 100644
--- a/ios/web/net/cert_verifier_block_adapter.h
+++ b/ios/web/net/cert_verifier_block_adapter.h
@@ -30,8 +30,8 @@ class CertVerifierBlockAdapter {
   CertVerifierBlockAdapter(net::CertVerifier* cert_verifier,
                            net::NetLog* net_log);
 
-  // When the verifier is destroyed, all certificate verification requests are
-  // canceled, and their completion handlers will not be called.
+  // When the verifier is destroyed, certificate verification requests are not
+  // canceled, and their completion handlers are guaranteed to be called.
   ~CertVerifierBlockAdapter();
 
   // Encapsulates verification params. |cert| and |hostname| are mandatory, the
@@ -69,12 +69,11 @@ class CertVerifierBlockAdapter {
   // Verifies certificate with given |params|. |completion_handler| must not be
   // null and can be called either synchronously (in the same runloop) or
   // asynchronously.
+  // Note: |completion_handler| is guaranteed to be called, even if the instance
+  // |Verify()| was called on is destroyed.
   void Verify(const Params& params, CompletionHandler completion_handler);
 
  private:
-  // Pending verification requests. Request must be alive until verification is
-  // completed, otherwise verification operation will be cancelled.
-  ScopedVector<net::CertVerifier::Request> pending_requests_;
   // Underlying unowned CertVerifier.
   net::CertVerifier* cert_verifier_;
   // Unowned NetLog required by CertVerifier.
diff --git a/ios/web/net/cert_verifier_block_adapter_unittest.cc b/ios/web/net/cert_verifier_block_adapter_unittest.cc
index f05acf6..9480571 100644
--- a/ios/web/net/cert_verifier_block_adapter_unittest.cc
+++ b/ios/web/net/cert_verifier_block_adapter_unittest.cc
@@ -148,4 +148,25 @@ TEST_F(CertVerifierBlockAdapterTest, DefaultParamsAndSyncError) {
   EXPECT_EQ(kExpectedError, actual_error);
 }
 
+// Tests that the completion handler passed to |Verify()| is called, even if the
+// adapter is destroyed.
+TEST_F(CertVerifierBlockAdapterTest, CompletionHandlerIsAlwaysCalled) {
+  scoped_ptr<net::CertVerifier> verifier(net::CertVerifier::CreateDefault());
+  scoped_ptr<CertVerifierBlockAdapter> test_adapter(
+      new CertVerifierBlockAdapter(verifier.get(), &net_log_));
+
+  // Call |Verify| and destroy the adapter.
+  CertVerifierBlockAdapter::Params params(cert_.get(), kHostName);
+  __block bool verification_completed = false;
+  test_adapter->Verify(params, ^(net::CertVerifyResult, int) {
+    verification_completed = true;
+  });
+  test_adapter.reset();
+
+  // Make sure that the completion handler is called.
+  base::test::ios::WaitUntilCondition(^{
+    return verification_completed;
+  }, base::MessageLoop::current(), base::TimeDelta());
+}
+
 }  // namespace web