Allow CertNetFetcher to be shutdown from the network thread

net/cert/cert_net_fetcher.h

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef NET_CERT_CERT_NET_FETCHER_H_
 #define NET_CERT_CERT_NET_FETCHER_H_
 
 #include <stdint.h>
 
 #include <memory>
 #include <vector>
 
 #include "base/callback.h"
 #include "base/macros.h"
 #include "net/base/net_errors.h"
 #include "net/base/net_export.h"
 
 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:
     virtual ~Request() {}
 
     // WaitForResult() can be called at most once.
     //
     // It will block and wait for the (network) request to complete, and
     // then write the result into the provided out-parameters.
     virtual void WaitForResult(Error* error, std::vector<uint8_t>* bytes) = 0;
   };
 
   // This value can be used in place of timeout or max size limits.
   enum { DEFAULT = -1 };
 
   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
   // parameters:
   //
   //   * url -- The http:// URL to fetch.
   //   * timeout_seconds -- The maximum allowed duration for the fetch job. If
   //         this delay is exceeded then the request will fail. To use a default
   //         timeout pass DEFAULT.
   //   * 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,
       int timeout_milliseconds,
       int max_response_bytes) = 0;
 
   virtual WARN_UNUSED_RESULT std::unique_ptr<Request> FetchCrl(
       const GURL& url,
       int timeout_milliseconds,
       int max_response_bytes) = 0;
 
   virtual WARN_UNUSED_RESULT std::unique_ptr<Request> FetchOcsp(
       const GURL& url,
       int timeout_milliseconds,
       int max_response_bytes) = 0;
 
+ protected:
+  virtual ~CertNetFetcher() {}
+
  private:
+  friend class base::RefCountedThreadSafe<CertNetFetcher>;
   DISALLOW_COPY_AND_ASSIGN(CertNetFetcher);
 };
 
 }  // namespace net
 
 #endif  // NET_CERT_CERT_NET_FETCHER_H_