Allow CertNetFetcher to be shut down from the network thread

In order to be suitable for use from within CertVerifyProc, this CL makes
CertNetFetcher shareable between the network and worker threads. The
worker thread will start and wait for fetch requests, but the network
thread needs to be able to create and shutdown the fetcher. Shutdown is
needed to be able to destroy the URLRequestContext cleanly: since the
CertNetFetcher will have a reference to the URLRequestContext, the network
thread needs to be able to shut down the fetcher and cancel outstanding
requests before destroying the URLRequestContext.

Thus this CL makes CertNetFetcher refcounted (replacing its refcounted
bridge class CertNetFetcherCore) and gives it a Shutdown method to cancel
its outstanding requests and prevent new ones from starting.

Note that there is still a race where a worker thread could issue a fetch
request while the network thread is shutting down. In this case,
the fetch task would never run on the network thread and WaitForResult would
hang indefinitely. I'm not sure what to do about this except give
WaitForResult() a timeout like nss_ocsp does.

BUG=657549

Review-Url: https://codereview.chromium.org/2595723002
Cr-Commit-Position: refs/heads/master@{#443490}

net/cert/cert_net_fetcher.h

@@ -20,16 +20,15 @@ class GURL;
 namespace net {
 
 // CertNetFetcher is a synchronous interface for fetching AIA URLs and CRL
-// URLs.
+// URLs. It is shared between a caller thread (which starts and waits for
+// fetches), and a network thread (which does the actual fetches). It can be
+// shutdown from the network thread to cancel outstanding requests.
 //
 // A Request object is returned when starting a fetch. The consumer can
 // use this as a handle for aborting the request (by freeing it), or reading
 // the result of the request (WaitForResult)
-//
-// This interface is expected to be operated from a single thread.
-//
-// The CertNetFetcher must outlive all Request objects it creates.
-class NET_EXPORT CertNetFetcher {
+class NET_EXPORT CertNetFetcher
+    : public base::RefCountedThreadSafe<CertNetFetcher> {
  public:
   class Request {
    public:
@@ -47,7 +46,12 @@ class NET_EXPORT CertNetFetcher {
 
   CertNetFetcher() {}
 
-  virtual ~CertNetFetcher() {}
+  // Shuts down the CertNetFetcher and cancels outstanding network requests. It
+  // is not guaranteed that any outstanding or subsequent
+  // Request::WaitForResult() calls will be completed. Shutdown() must be called
+  // from the network thread. It can be called more than once, but must be
+  // called before the CertNetFetcher is destroyed.
+  virtual void Shutdown() = 0;
 
   // The Fetch*() methods start a request which can be cancelled by
   // deleting the returned Request. Here is the meaning of the common
@@ -76,7 +80,11 @@ class NET_EXPORT CertNetFetcher {
       int timeout_milliseconds,
       int max_response_bytes) = 0;
 
+ protected:
+  virtual ~CertNetFetcher() {}
+
  private:
+  friend class base::RefCountedThreadSafe<CertNetFetcher>;
   DISALLOW_COPY_AND_ASSIGN(CertNetFetcher);
 };
 

net/cert/internal/cert_issuer_source_aia.cc

@@ -93,8 +93,9 @@ bool AiaRequest::AddCompletedFetchToResults(Error error,
 
 }  // namespace
 
-CertIssuerSourceAia::CertIssuerSourceAia(CertNetFetcher* cert_fetcher)
-    : cert_fetcher_(cert_fetcher) {}
+CertIssuerSourceAia::CertIssuerSourceAia(
+    scoped_refptr<CertNetFetcher> cert_fetcher)
+    : cert_fetcher_(std::move(cert_fetcher)) {}
 
 CertIssuerSourceAia::~CertIssuerSourceAia() = default;
 

net/cert/internal/cert_issuer_source_aia.h

@@ -16,10 +16,10 @@ class CertNetFetcher;
 class NET_EXPORT CertIssuerSourceAia : public CertIssuerSource {
  public:
   // Creates CertIssuerSource that will use |cert_fetcher| to retrieve issuers
-  // using AuthorityInfoAccess URIs. |cert_fetcher| must outlive the
-  // CertIssuerSourceAia.  CertIssuerSourceAia must be created and used only on
-  // a single thread, which is the thread |cert_fetcher| will be operated from.
-  explicit CertIssuerSourceAia(CertNetFetcher* cert_fetcher);
+  // using AuthorityInfoAccess URIs. CertIssuerSourceAia must be created and
+  // used only on a single thread, which is the thread |cert_fetcher| will be
+  // operated from.
+  explicit CertIssuerSourceAia(scoped_refptr<CertNetFetcher> cert_fetcher);
   ~CertIssuerSourceAia() override;
 
   // CertIssuerSource implementation:
@@ -29,7 +29,7 @@ class NET_EXPORT CertIssuerSourceAia : public CertIssuerSource {
                          std::unique_ptr<Request>* out_req) override;
 
  private:
-  CertNetFetcher* cert_fetcher_;
+  scoped_refptr<CertNetFetcher> cert_fetcher_;
 
   DISALLOW_COPY_AND_ASSIGN(CertIssuerSourceAia);
 };

net/cert_net/cert_net_fetcher_impl.h

@@ -7,21 +7,21 @@
 
 #include <memory>
 
+#include "base/memory/ref_counted.h"
 #include "net/base/net_export.h"
 
 namespace net {
 
 class CertNetFetcher;
-class URLRequestContextGetter;
+class URLRequestContext;
 
 // Creates a CertNetFetcher that issues requests through the provided
-// URLRequestContext.
-//
-// The returned CertNetFetcher is to be operated on a thread *other* than the
-// thread used for the URLRequestContext (since it gives a blocking interface
-// to URL fetching).
-NET_EXPORT std::unique_ptr<CertNetFetcher> CreateCertNetFetcher(
-    URLRequestContextGetter* context_getter);
+// URLRequestContext. The URLRequestContext must stay valid until the returned
+// CertNetFetcher's Shutdown method is called. The CertNetFetcher is to be
+// created and shutdown on the network thread. Its Fetch methods are to be used
+// on a *different* thread, since it gives a blocking interface to URL fetching.
+NET_EXPORT scoped_refptr<CertNetFetcher> CreateCertNetFetcher(
+    URLRequestContext* context);
 
 }  // namespace net
 

net/test/url_request/url_request_hanging_read_job.cc

@@ -92,6 +92,8 @@ void URLRequestHangingReadJob::GetResponseInfoConst(
 }
 
 void URLRequestHangingReadJob::StartAsync() {
+  if (is_done())
+    return;
   set_expected_content_size(content_length_);
   NotifyHeadersComplete();
 }

net/tools/cert_verify_tool/verify_using_path_builder.cc

@@ -281,6 +281,9 @@ bool VerifyUsingPathBuilder(
 
   // Initialize an AIA fetcher, that uses a separate thread for running the
   // networking message loop.
+// TODO(estark): update this code to use the new CertNetFetcher
+// interface that takes a URLRequestContext*.
+#if 0
   base::Thread::Options options(base::MessageLoop::TYPE_IO, 0);
   base::Thread thread("network_thread");
   CHECK(thread.StartWithOptions(options));
@@ -290,13 +293,16 @@ bool VerifyUsingPathBuilder(
       CreateCertNetFetcher(url_request_context_getter.get());
   net::CertIssuerSourceAia aia_cert_issuer_source(cert_net_fetcher.get());
   path_builder.AddCertIssuerSource(&aia_cert_issuer_source);
+#endif
 
   // Run the path builder.
   path_builder.Run();
 
+#if 0
   // Stop the temporary network thread..
   url_request_context_getter->ShutDown();
   thread.Stop();
+#endif
 
   // TODO(crbug.com/634443): Display any errors/warnings associated with path
   //                         building that were not part of a particular