Allow CertNetFetcher to be shutdown from the network thread

net/cert_net/cert_net_fetcher_impl.h

Index: net/cert_net/cert_net_fetcher_impl.h
diff --git a/net/cert_net/cert_net_fetcher_impl.h b/net/cert_net/cert_net_fetcher_impl.h
index d4add24c2ffd96d2dfe8541efe9d6d19d6ce831a..30b564b7c7966707a9d7cb2ccac5d865fddd3dc6 100644
--- a/net/cert_net/cert_net_fetcher_impl.h
+++ b/net/cert_net/cert_net_fetcher_impl.h
@@ -7,20 +7,34 @@
 
 #include <memory>
 
+#include "base/memory/ref_counted.h"
 #include "net/base/net_export.h"
 
+namespace base {
+class SingleThreadTaskRunner;
+}  // namespace base
+
 namespace net {
 
 class CertNetFetcher;
+class URLRequestContext;
 class URLRequestContextGetter;
 
 // 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(
+// URLRequestContext (which must outlived the CertNetFetcher) on the

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

typo outlived.

Also as far as word-smithing, it needn't necessarily "outlive" the
CertNetFetcher, just need to call CertNetFetcher::Shutdown() to dissociate the
URLRequestContext* and cancel requests before it potentially does die.

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

Done.

+// given TaskRunner. The CertNetFetcher is to be created and shutdown on
+// the network thread corresponding to |task_runner| and |context|. 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(
+    scoped_refptr<base::SingleThreadTaskRunner> task_runner,

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

I don't think we should have this variant (that takes a task_runner +
URLRequestContext*)

Because URLRequestContext's lifetime is tied to the network thread, passing raw
pointers to it on anything other than the network thread is suspicious.

I think the only version we should support and expose in this header file is:

  CreateCertNetFetcher(URLRequestContext*)

And this must be called from the network thread.

The URLRequestContextGetter variant was to support creation from other threads,
which per the comment below I think we should remove.

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

Done.

+    URLRequestContext* context);
+
+// Creates a CertNetFetcher that issues requests through the provider
+// |context_getter|'s URLRequestContext. This function is to be called
+// on a thread *other* than the thread used for the URLRequestContext
+// (since it gives a blocking interface to URL fetching).
+NET_EXPORT scoped_refptr<CertNetFetcher> CreateCertNetFetcherOnCallerThread(

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

I don't think we want to expose this function.

The only place that uses it is verify_using_path_builder.cc, and that file can
be changed to invert the initialization order.

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

Done; however, I'm not sure if I did this properly in
verify_using_path_builder.cc. It seems a little convoluted the way that I did
it, not sure if there is a simpler way.

     URLRequestContextGetter* context_getter);
 
 }  // namespace net