Allow CertNetFetcher to be shutdown from the network thread

net/tools/cert_verify_tool/verify_using_path_builder.cc

Index: net/tools/cert_verify_tool/verify_using_path_builder.cc
diff --git a/net/tools/cert_verify_tool/verify_using_path_builder.cc b/net/tools/cert_verify_tool/verify_using_path_builder.cc
index adaf5fe5d81aab62aa3d1b15f25d57e62acc3d14..a8ddfe73b22bf801fc2a749ae0202faff68be1f3 100644
--- a/net/tools/cert_verify_tool/verify_using_path_builder.cc
+++ b/net/tools/cert_verify_tool/verify_using_path_builder.cc
@@ -7,6 +7,7 @@
 #include <iostream>
 
 #include "base/memory/ptr_util.h"
+#include "base/run_loop.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/threading/thread.h"
@@ -221,15 +222,29 @@ class URLRequestContextGetterForAia : public net::URLRequestContextGetter {
   std::unique_ptr<net::URLRequestContext> context_;
 };
 
-}  // namespace
+void ShutDownCertNetFetcher(
+    const scoped_refptr<net::CertNetFetcher>& cert_net_fetcher,
+    const scoped_refptr<base::SingleThreadTaskRunner>& network_task_runner,
+    const base::Closure& callback) {
+  network_task_runner->PostTaskAndReply(
+      FROM_HERE, base::Bind(&net::CertNetFetcher::Shutdown, cert_net_fetcher),
+      callback);
+}
 
-// Verifies |target_der_cert| using CertPathBuilder.
-bool VerifyUsingPathBuilder(
+// Performs the actual certificate verification using CertPathBuilder. When
+// completed, it sets |result| to true if verification was successful and false
+// otherwise. Then it shuts down |cert_net_fetcher| on the network thread and
+// calls |callback| when all this is complete.
+void DoVerifyOnWorkerThread(
+    scoped_refptr<net::CertNetFetcher> cert_net_fetcher,
+    scoped_refptr<base::SingleThreadTaskRunner> network_task_runner,
     const CertInput& target_der_cert,
     const std::vector<CertInput>& intermediate_der_certs,
     const std::vector<CertInput>& root_der_certs,
     const base::Time at_time,
-    const base::FilePath& dump_prefix_path) {
+    const base::FilePath& dump_prefix_path,
+    const base::Closure& callback,
+    bool* result) {
   base::Time::Exploded exploded_time;
   at_time.UTCExplode(&exploded_time);
   net::der::GeneralizedTime time = ConvertExplodedTime(exploded_time);
@@ -265,58 +280,121 @@ bool VerifyUsingPathBuilder(
 
   scoped_refptr<net::ParsedCertificate> target_cert =
       ParseCertificate(target_der_cert);
-  if (!target_cert)
-    return false;
+  if (!target_cert) {
+    *result = false;
+    ShutDownCertNetFetcher(cert_net_fetcher, network_task_runner, callback);
+    return;
+  }
 
   // Verify the chain.
   net::SimpleSignaturePolicy signature_policy(2048);
-  net::CertPathBuilder::Result result;
+  net::CertPathBuilder::Result verify_result;
   net::CertPathBuilder path_builder(target_cert, &trust_store,
-                                    &signature_policy, time, &result);
+                                    &signature_policy, time, &verify_result);
   path_builder.AddCertIssuerSource(&intermediate_cert_issuer_source);
 #if defined(USE_NSS_CERTS)
   net::CertIssuerSourceNSS cert_issuer_source_nss;
   path_builder.AddCertIssuerSource(&cert_issuer_source_nss);
 #endif
 
-  // Initialize an AIA fetcher, that uses a separate thread for running the
-  // networking message loop.
-  base::Thread::Options options(base::MessageLoop::TYPE_IO, 0);
-  base::Thread thread("network_thread");
-  CHECK(thread.StartWithOptions(options));
-  scoped_refptr<URLRequestContextGetterForAia> url_request_context_getter(
-      new URLRequestContextGetterForAia(thread.task_runner()));
-  auto cert_net_fetcher =
-      CreateCertNetFetcher(url_request_context_getter.get());
-  net::CertIssuerSourceAia aia_cert_issuer_source(cert_net_fetcher.get());
+  net::CertIssuerSourceAia aia_cert_issuer_source(cert_net_fetcher);
   path_builder.AddCertIssuerSource(&aia_cert_issuer_source);
 
   // Run the path builder.
   path_builder.Run();
 
-  // Stop the temporary network thread..
-  url_request_context_getter->ShutDown();
-  thread.Stop();
-
   // TODO(crbug.com/634443): Display any errors/warnings associated with path
   //                         building that were not part of a particular
   //                         PathResult.
   std::cout << "CertPathBuilder result: "
-            << (result.HasValidPath() ? "SUCCESS" : "FAILURE") << "\n";
+            << (verify_result.HasValidPath() ? "SUCCESS" : "FAILURE") << "\n";
 
-  for (size_t i = 0; i < result.paths.size(); ++i) {
-    PrintResultPath(result.paths[i].get(), i, i == result.best_result_index);
+  for (size_t i = 0; i < verify_result.paths.size(); ++i) {
+    PrintResultPath(verify_result.paths[i].get(), i,
+                    i == verify_result.best_result_index);
   }
 
   // TODO(mattm): add flag to dump all paths, not just the final one?
-  if (!dump_prefix_path.empty() && result.paths.size()) {
+  if (!dump_prefix_path.empty() && verify_result.paths.size()) {
     if (!DumpParsedCertificateChain(
             dump_prefix_path.AddExtension(
                 FILE_PATH_LITERAL(".CertPathBuilder.pem")),
-            result.paths[result.best_result_index]->path)) {
-      return false;
+            verify_result.paths[verify_result.best_result_index]->path)) {
+      *result = false;
+      ShutDownCertNetFetcher(cert_net_fetcher, network_task_runner, callback);
+      return;
     }
   }
 
-  return result.HasValidPath();
+  *result = verify_result.HasValidPath();
+  ShutDownCertNetFetcher(cert_net_fetcher, network_task_runner, callback);
+}
+
+// Creates a CertNetFetcher and passes a reference to the worker thread to
+// perform the verification using that CertNetFetcher.
+void StartVerifyOnNetworkThread(
+    scoped_refptr<base::SingleThreadTaskRunner> worker_task_runner,
+    scoped_refptr<URLRequestContextGetterForAia> context_getter,
+    const CertInput& target_der_cert,
+    const std::vector<CertInput>& intermediate_der_certs,
+    const std::vector<CertInput>& root_der_certs,
+    const base::Time at_time,
+    const base::FilePath& dump_prefix_path,
+    const base::Closure& callback,
+    bool* result) {
+  scoped_refptr<net::CertNetFetcher> cert_net_fetcher =
+      net::CreateCertNetFetcher(context_getter->GetURLRequestContext());
+  worker_task_runner->PostTask(
+      FROM_HERE,
+      base::Bind(&DoVerifyOnWorkerThread, cert_net_fetcher,
+                 base::ThreadTaskRunnerHandle::Get(), target_der_cert,
+                 intermediate_der_certs, root_der_certs, at_time,
+                 dump_prefix_path, callback, result));
+}
+
+}  // namespace
+
+// Verifies |target_der_cert| using CertPathBuilder.
+bool VerifyUsingPathBuilder(
+    const CertInput& target_der_cert,
+    const std::vector<CertInput>& intermediate_der_certs,
+    const std::vector<CertInput>& root_der_certs,
+    const base::Time at_time,
+    const base::FilePath& dump_prefix_path) {
+  // Create a network thread for AIA fetching operations. A CertNetFetcher will
+  // be created and shutdown on this thread.
+  base::Thread::Options options(base::MessageLoop::TYPE_IO, 0);
+  base::Thread thread("network_thread");
+  CHECK(thread.StartWithOptions(options));
+
+  bool result = false;
+  base::RunLoop run_loop;

[eroman, 2017/01/11 01:57:00]

I don't think we should have a message loop on the main thread.

Instead:

### Setup:
(1) Start up a base::Thread for the network thread
(2) Post a task to the network thread that creates the URLRequestContext, and
the CertNetFetcherImpl. Use a WaitableEvent to block until this initialization
is complete (or alternately subclass a Thread).

(3) Use the CertNetFetcherImpl to initialize the CertIssuerSourceAia
(4) call PathBuilder::Run()

### Cleanup
(5) Post a task to the network thread that destroys the CertNetFetcherImpl and
URLRequestContext (no need for a WaitableEvent, since joining the thread will
block until it runs).


If you have trouble refactoring this feel free to just "#if 0" out the code, and
I can switch it over after you land your AIA change.

[estark, 2017/01/12 01:06:31]

Thanks. I #if'd it out for now. I'm happy to come back and update this but would
like to unblock the AIA change.

[eroman, 2017/01/12 02:11:37]

Sure, sounds good.
(The tool is just used by mattm and me -- temporarily commenting out support for
AIA is fine).

+  scoped_refptr<URLRequestContextGetterForAia> url_request_context_getter(
+      new URLRequestContextGetterForAia(thread.task_runner()));
+
+  // A CertNetFetcher must be created on the network thread before verification
+  // can proceed on the main thread (and the fetcher must be shutdown on the
+  // network thread when verification completes). Thus, the flow is as follows.
+  //
+  // The StartVerifyOnNetworkThread task runs on the network thread and creates
+  // a CertNetFetcher using the URLRequestContext managed by
+  // |url_request_context_getter|. When the CertNetFetcher is created, a
+  // reference to it is posted back to this thread to run
+  // DoVerifyOnWorkerThread using the created CertNetFetcher. When the
+  // verification is completed, DoVerifyOnWorkerThread writes the result to
+  // |result|, shuts down the CertNetFetcher on the network thread, and then
+  // quits |run_loop|.
+  thread.task_runner()->PostTask(
+      FROM_HERE, base::Bind(&StartVerifyOnNetworkThread,
+                            base::ThreadTaskRunnerHandle::Get(),
+                            url_request_context_getter, target_der_cert,
+                            intermediate_der_certs, root_der_certs, at_time,
+                            dump_prefix_path, run_loop.QuitClosure(), &result));
+  run_loop.Run();
+
+  // Stop the temporary network thread.
+  url_request_context_getter->ShutDown();
+  thread.Stop();
+
+  return result;
 }