Allow CertNetFetcher to be shutdown from the network thread

net/cert_net/cert_net_fetcher_impl.cc

Index: net/cert_net/cert_net_fetcher_impl.cc
diff --git a/net/cert_net/cert_net_fetcher_impl.cc b/net/cert_net/cert_net_fetcher_impl.cc
index bedd2af413bee6299b8c48e469830103ca155c47..0a595022aa4925c59894187542d4e16a7a7d33e8 100644
--- a/net/cert_net/cert_net_fetcher_impl.cc
+++ b/net/cert_net/cert_net_fetcher_impl.cc
@@ -13,34 +13,25 @@
 // caller can use to cancel the fetch, or wait for it to complete
 // (blocking).
 //
+// The CertNetFetcherImpl is shared between a network thread and a
+// caller thread that waits for fetches to happen on the network thread.
+//
 // The classes are mainly organized based on their thread affinity:
 //
 // ---------------
-// Lives on caller thread
+// Straddles caller thread and network thread
 // ---------------
 //
 // CertNetFetcherImpl (implements CertNetFetcher)
-//   * Main entry point
-//   * Provides a service to start/cancel/wait for URL fetches
+//   * Main entry point. Has constructors that allow it be created on
+//     either thread, but can only be shutdown from the network thread.
+//   * Provides a service to start/cancel/wait for URL fetches, to be
+//     used on the caller thread.
 //   * Returns callers a CertNetFetcher::Request as a handle
 //   * Requests can run in parallel, however will block the current thread when
 //     reading results.
 //   * Posts tasks to network thread to coordinate actual work
 //
-// CertNetFetcherRequestImpl (implements CertNetFetcher::Request)
-//   * Wrapper for cancelling events, or waiting for a request to complete
-//   * Waits on a WaitableEvent to complete requests.
-//
-// ---------------
-// Straddles caller thread and network thread
-// ---------------
-//
-// CertNetFetcherCore
-//   * Reference-counted bridge between CertNetFetcherImpl and the dependencies
-//     on network thread.
-//   * Small wrapper to holds the state that is conceptually owned by
-//     CertNetFetcherImpl, but belongs on the network thread.
-//
 // RequestCore
 //   * Reference-counted bridge between CertNetFetcherRequestImpl and the
 //     dependencies on the network thread
@@ -48,6 +39,14 @@
 //     completion, and pointers for canceling work on network thread.
 //
 // ---------------
+// Lives on caller thread
+// ---------------
+//
+// CertNetFetcherRequestImpl (implements CertNetFetcher::Request)
+//   * Wrapper for cancelling events, or waiting for a request to complete
+//   * Waits on a WaitableEvent to complete requests.
+//
+// ---------------
 // Lives on network thread
 // ---------------
 //
@@ -129,13 +128,16 @@ class AsyncCertNetFetcherImpl {
   ~AsyncCertNetFetcherImpl();
 
   // Starts an asynchronous request to fetch the given URL. On completion
-  // |callback| will be invoked.
+  // request->OnJobCompleted() will be invoked.
   //
-  // Completion of the request will never occur synchronously. In other words it
-  // is guaranteed that |callback| will only be invoked once the Fetch*() method
-  // has returned.
+  // Completion of the request will never occur synchronously. In other
+  // words it is guaranteed that request->OnJobCompleted() will only be
+  // invoked once the Fetch*() method has returned.
   void Fetch(std::unique_ptr<RequestParams> request_params,
-             RequestCore* request);
+             scoped_refptr<RequestCore> request);
+
+  // Cancels outstanding jobs.
+  void Shutdown();
 
  private:
   friend class Job;
@@ -297,9 +299,8 @@ class Job : public URLRequest::Delegate {
   const RequestParams& request_params() const { return *request_params_; }
 
   // Create a request and attaches it to the job. When the job completes it will
-  // notify the request of completion through OnJobCompleted. Note that the Job
-  // does NOT own the request.
-  void AttachRequest(RequestCore* request);
+  // notify the request of completion through OnJobCompleted.
+  void AttachRequest(scoped_refptr<RequestCore> request);
 
   // Removes |request| from the job.
   void DetachRequest(RequestCore* request);
@@ -309,6 +310,10 @@ class Job : public URLRequest::Delegate {
   // notified of completion.
   void StartURLRequest(URLRequestContext* context);
 
+  // Cancels the URLRequest. If StartURLRequest() is invoked subsequently,
+  // it will immediately call OnJobCompleted with an error.
+  void Cancel();
+
  private:
   // Implementation of URLRequest::Delegate
   void OnReceivedRedirect(URLRequest* request,
@@ -340,8 +345,8 @@ class Job : public URLRequest::Delegate {
   // OnUrlRequestCompleted().
   void FailRequest(Error error);
 
-  // The requests attached to this job (non-owned).
-  std::vector<RequestCore*> requests_;
+  // The requests attached to this job.
+  std::vector<scoped_refptr<RequestCore>> requests_;
 
   // The input parameters for starting a URLRequest.
   std::unique_ptr<RequestParams> request_params_;
@@ -359,6 +364,10 @@ class Job : public URLRequest::Delegate {
   // Non-owned pointer to the AsyncCertNetFetcherImpl that created this job.
   AsyncCertNetFetcherImpl* parent_;
 
+  // When set, StartURLRequest() will post a task to run
+  // OnJobCompleted() with an error and return immediately.
+  bool cancelled_ = false;
+
   DISALLOW_COPY_AND_ASSIGN(Job);
 };
 
@@ -387,7 +396,7 @@ Job::~Job() {
   Stop();
 }
 
-void Job::AttachRequest(RequestCore* request) {
+void Job::AttachRequest(scoped_refptr<RequestCore> request) {
   requests_.push_back(request);
   request->AttachedToJob(this);
 }
@@ -406,6 +415,12 @@ void Job::DetachRequest(RequestCore* request) {
 }
 
 void Job::StartURLRequest(URLRequestContext* context) {
+  if (cancelled_) {
+    timer_.Start(
+        FROM_HERE, base::TimeDelta(),
+        base::Bind(&Job::OnJobCompleted, base::Unretained(this), ERR_ABORTED));
+    return;
+  }
   Error error = CanFetchUrl(request_params_->url);
   if (error != OK) {
     // TODO(eroman): Don't post a task for this case.
@@ -432,6 +447,12 @@ void Job::StartURLRequest(URLRequestContext* context) {
         base::Bind(&Job::FailRequest, base::Unretained(this), ERR_TIMED_OUT));
 }
 
+void Job::Cancel() {
+  cancelled_ = true;
+  if (url_request_)
+    url_request_->Cancel();
+}
+
 void Job::OnReceivedRedirect(URLRequest* request,
                              const RedirectInfo& redirect_info,
                              bool* defer_redirect) {
@@ -525,7 +546,7 @@ void Job::OnJobCompleted(Error error) {
 
   std::unique_ptr<Job> delete_this = parent_->RemoveJob(this);
 
-  for (auto* request : requests_) {
+  for (auto request : requests_) {
     request->OnJobCompleted(this, error, response_body_);
   }
 
@@ -555,7 +576,7 @@ bool JobComparator::operator()(const Job* job1, const Job* job2) const {
 
 void AsyncCertNetFetcherImpl::Fetch(
     std::unique_ptr<RequestParams> request_params,
-    RequestCore* request) {
+    scoped_refptr<RequestCore> request) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   // If there is an in-progress job that matches the request parameters use it.
@@ -571,6 +592,13 @@ void AsyncCertNetFetcherImpl::Fetch(
   return job->AttachRequest(request);
 }
 
+void AsyncCertNetFetcherImpl::Shutdown() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  for (const auto& job : jobs_) {
+    job.first->Cancel();
+  }
+}
+
 struct JobToRequestParamsComparator {
   bool operator()(const JobSet::value_type& job,
                   const RequestParams& value) const {
@@ -622,60 +650,28 @@ class CertNetFetcherRequestImpl : public CertNetFetcher::Request {
   scoped_refptr<RequestCore> core_;
 };
 
-class CertNetFetcherCore
-    : public base::RefCountedThreadSafe<CertNetFetcherCore> {
+class CertNetFetcherImpl : public CertNetFetcher {
  public:
-  explicit CertNetFetcherCore(URLRequestContextGetter* context_getter)
-      : context_getter_(context_getter) {}
-
-  void Abandon() {
-    GetNetworkTaskRunner()->PostTask(
-        FROM_HERE,
-        base::Bind(&CertNetFetcherCore::DoAbandonOnNetworkThread, this));
-  }
-
-  scoped_refptr<base::SingleThreadTaskRunner> GetNetworkTaskRunner() {
-    return context_getter_->GetNetworkTaskRunner();
+  CertNetFetcherImpl(scoped_refptr<base::SingleThreadTaskRunner> task_runner,
+                     URLRequestContext* context)
+      : task_runner_(task_runner), context_(context) {}
+
+  explicit CertNetFetcherImpl(URLRequestContextGetter* context_getter) {
+    context_getter->GetNetworkTaskRunner()->PostTask(

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

There is a possible race with:

  (1) Create  (on network thread)
  (2) call Shutdown()  on network thread (before the init task has run).

Per comment in header file, I think we should only support creation from the
network thread (initialization can be more direct then).

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

Done.

+        FROM_HERE, base::Bind(&CertNetFetcherImpl::InitializeOnNetworkThread,
+                              this, context_getter));
   }
 
-  void DoFetchOnNetworkThread(std::unique_ptr<RequestParams> request_params,
-                              scoped_refptr<RequestCore> request) {
-    DCHECK(GetNetworkTaskRunner()->RunsTasksOnCurrentThread());
-
-    if (!impl_) {
-      impl_.reset(
-          new AsyncCertNetFetcherImpl(context_getter_->GetURLRequestContext()));
+  void Shutdown() override {
+    DCHECK(task_runner_->RunsTasksOnCurrentThread());
+    base::AutoLock auto_lock(shutdown_lock_);
+    shutdown_ = true;
+    if (impl_) {
+      impl_->Shutdown();
     }
-
-    // Don't need to retain a reference to |request| because consume is
-    // expected to keep it alive.
-    impl_->Fetch(std::move(request_params), request.get());
-  }
-
- private:
-  friend class base::RefCountedThreadSafe<CertNetFetcherCore>;
-
-  void DoAbandonOnNetworkThread() {
-    DCHECK(GetNetworkTaskRunner()->RunsTasksOnCurrentThread());
-    impl_.reset();
+    context_ = nullptr;
   }
 
-  ~CertNetFetcherCore() { DCHECK(!impl_); }
-
-  scoped_refptr<URLRequestContextGetter> context_getter_;
-
-  std::unique_ptr<AsyncCertNetFetcherImpl> impl_;
-
-  DISALLOW_COPY_AND_ASSIGN(CertNetFetcherCore);
-};
-
-class CertNetFetcherImpl : public CertNetFetcher {
- public:
-  explicit CertNetFetcherImpl(URLRequestContextGetter* context_getter)
-      : core_(new CertNetFetcherCore(context_getter)) {}
-
-  ~CertNetFetcherImpl() override { core_->Abandon(); }
-
   std::unique_ptr<Request> FetchCaIssuers(const GURL& url,
                                           int timeout_milliseconds,
                                           int max_response_bytes) override {
@@ -720,28 +716,68 @@ class CertNetFetcherImpl : public CertNetFetcher {
   }
 
  private:
+  ~CertNetFetcherImpl() override {}
+
+  void InitializeOnNetworkThread(URLRequestContextGetter* context_getter) {
+    DCHECK(context_getter->GetNetworkTaskRunner()->RunsTasksOnCurrentThread());
+    task_runner_ = context_getter->GetNetworkTaskRunner();
+    context_ = context_getter->GetURLRequestContext();
+  }
+
+  void DoFetchOnNetworkThread(std::unique_ptr<RequestParams> request_params,
+                              scoped_refptr<RequestCore> request) {
+    DCHECK(task_runner_->RunsTasksOnCurrentThread());
+
+    if (!context_) {
+      request->Cancel();
+      return;
+    }
+
+    if (!impl_) {
+      impl_.reset(new AsyncCertNetFetcherImpl(context_));
+    }
+
+    impl_->Fetch(std::move(request_params), request);
+  }
+
   std::unique_ptr<Request> DoFetch(
       std::unique_ptr<RequestParams> request_params) {
-    auto task_runner = core_->GetNetworkTaskRunner();
-    scoped_refptr<RequestCore> request_core = new RequestCore(task_runner);
+    scoped_refptr<RequestCore> request_core = new RequestCore(task_runner_);
 
-    task_runner->PostTask(
-        FROM_HERE,
-        base::Bind(&CertNetFetcherCore::DoFetchOnNetworkThread, core_,
-                   base::Passed(&request_params), request_core));
+    base::AutoLock auto_lock(shutdown_lock_);
+    if (shutdown_)
+      return nullptr;
+
+    if (!task_runner_->PostTask(
+            FROM_HERE,
+            base::Bind(&CertNetFetcherImpl::DoFetchOnNetworkThread, this,
+                       base::Passed(&request_params), request_core))) {
+      return nullptr;
+    }
 
     return base::MakeUnique<CertNetFetcherRequestImpl>(std::move(request_core));
   }
 
  private:
-  scoped_refptr<CertNetFetcherCore> core_;
+  scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
+  // Not owned. |context_| must stay valid until Shutdown() is called.
+  URLRequestContext* context_ = nullptr;
+  std::unique_ptr<AsyncCertNetFetcherImpl> impl_;
+  base::Lock shutdown_lock_;
+  bool shutdown_ = false;
 };
 
 }  // namespace
 
-std::unique_ptr<CertNetFetcher> CreateCertNetFetcher(
+scoped_refptr<CertNetFetcher> CreateCertNetFetcher(
+    scoped_refptr<base::SingleThreadTaskRunner> task_runner,
+    URLRequestContext* context) {
+  return make_scoped_refptr(new CertNetFetcherImpl(task_runner, context));
+}
+
+scoped_refptr<CertNetFetcher> CreateCertNetFetcherOnCallerThread(
     URLRequestContextGetter* context_getter) {
-  return base::MakeUnique<CertNetFetcherImpl>(context_getter);
+  return make_scoped_refptr(new CertNetFetcherImpl(context_getter));
 }
 
 }  // namespace net