Allow CertNetFetcher to be shutdown from the network thread

net/cert/cert_net_fetcher.h

Index: net/cert/cert_net_fetcher.h
diff --git a/net/cert/cert_net_fetcher.h b/net/cert/cert_net_fetcher.h
index 56a74948117db4f6ae95432dd9a721967a2cc0f3..81f8cbbcd5807dacd22f7dae63636bcef8a52084 100644
--- a/net/cert/cert_net_fetcher.h
+++ b/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,8 @@ class NET_EXPORT CertNetFetcher {
 
   CertNetFetcher() {}
 
-  virtual ~CertNetFetcher() {}
+  // Shuts down the CertNetFetcher and cancels outstanding requests.

[eroman, 2017/01/03 20:42:37]

Please document what the expectation is for outstanding requests:
 - Are they completed with an abortion error
 - Are they left hanging
 - Something in between

[estark, 2017/01/05 19:08:39]

Done. We can't guarantee the completion of requests that have not yet been
created on the network thread (as I mentioned in the CL description), so I
documented that requests can be left hanging. I think it might be better to make
the Wait() a TimedWait(), however, so that requests aren't left hanging forever.
WDYT?

+  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
@@ -60,6 +60,9 @@ class NET_EXPORT CertNetFetcher {
   //   * max_response_bytes -- The maximum size of the response body. If this
   //     size is exceeded then the request will fail. To use a default timeout
   //     pass DEFAULT.
+  //
+  // These methods may return nullptr if a request couldn't be started,

[eroman, 2017/01/03 20:42:37]

I am not sure this is the best interface.

Because shutdown happens asynchronously, it is unpredictable whether a worker
thread is going to see shutdown manifest as a nullptr return here.

In my opinion it is simpler and more consistent for clients to treat these the
same, and not need to special case nullptr.

When a request fails due to shutdown (either because it has already shut down,
or because it is shut down after returning the Result pointer), we should mark
the request as completed by signalling the waitable event.

[estark, 2017/01/05 19:08:39]

Done. Now if the fetcher has already been shutdown or the PostTask fails,
DoFetch immediately signals the request with ERR_ABORTED and returns it.

+  // e.g. because the fetcher has already been shutdown.
 
   virtual WARN_UNUSED_RESULT std::unique_ptr<Request> FetchCaIssuers(
       const GURL& url,
@@ -76,7 +79,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);
 };