Update PpdProvider threading model.

chromeos/printing/ppd_provider.cc

Index: chromeos/printing/ppd_provider.cc
diff --git a/chromeos/printing/ppd_provider.cc b/chromeos/printing/ppd_provider.cc
index fcae71105236f94f1b8b861539916d6c54a39cad..68fea3d903d44564d9676281132c2e47aa3782b8 100644
--- a/chromeos/printing/ppd_provider.cc
+++ b/chromeos/printing/ppd_provider.cc
@@ -14,6 +14,7 @@
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/threading/sequenced_task_runner_handle.h"
+#include "base/threading/thread_restrictions.h"
 #include "base/time/time.h"
 #include "base/values.h"
 #include "chromeos/printing/ppd_cache.h"
@@ -59,13 +60,16 @@ class PpdProviderImpl : public PpdProvider {
   PpdProviderImpl(
       const std::string& api_key,
       scoped_refptr<net::URLRequestContextGetter> url_context_getter,
+      scoped_refptr<base::SequencedTaskRunner> disk_task_runner,
       std::unique_ptr<PpdCache> cache,
       const PpdProvider::Options& options)
       : api_key_(api_key),
         forwarding_delegate_(this),
         url_context_getter_(url_context_getter),
+        disk_task_runner_(disk_task_runner),
         cache_(std::move(cache)),
-        options_(options) {
+        options_(options),
+        weak_factory_(this) {
     CHECK_GT(options_.max_ppd_contents_size_, 0U);
   }
   ~PpdProviderImpl() override {}
@@ -73,6 +77,21 @@ class PpdProviderImpl : public PpdProvider {
   void Resolve(const Printer::PpdReference& ppd_reference,
                const PpdProvider::ResolveCallback& cb) override {
     CHECK(!cb.is_null());
+    CHECK(base::SequencedTaskRunnerHandle::IsSet())
+        << "Resolve must be called from a SequencedTaskRunner context";
+    CHECK(resolve_fetcher_ == nullptr)
+        << "Can't have concurrent PpdProvider Resolve calls";
+    resolve_done_task_runner_ = base::SequencedTaskRunnerHandle::Get();

[stevenjb, 2016/11/11 19:26:43]

There is a lot of complexity here. I am wondering if we can simplify it some
using existing tools.

[Carlson, 2016/11/11 23:06:50]

I appreciate you taking the time to look carefully for stuff like this.  I'm
still finding my way around this codebase and likely doing many things in rather
suboptimal ways.

+    disk_task_runner_->PostTask(
+        FROM_HERE, base::Bind(&PpdProviderImpl::ResolveImpl,
+                              weak_factory_.GetWeakPtr(), ppd_reference, cb));

[stevenjb, 2016/11/11 19:26:43]

Here we would normally use TaskRunner::PostTaskAndReplyWithResult.

[Carlson, 2016/11/11 23:06:50]

I'm not sure that works for what we want here.  Or, rather, I think it doesn't
work, but would be happy to find out otherwise.

If I use PostTaskAndReplyWithResult, then I'm going to end up with the callback
on the disk thread, right?  I thought it was idiomatic in Chrome to have the
callback for an API like this be invoked on the same thread, or at least same
sequence, as the API function.  I don't see how to do that with
PostTaskAndReply*

What am I missing?

[stevenjb, 2016/11/11 23:25:37]

See the documentaiton for TaskRunner::PostTaskAndReply, that's exactly what it
does:

  // Posts |task| on the current TaskRunner.  On completion, |reply|
  // is posted to the thread that called PostTaskAndReply()...

+  }
+
+  // Do the work of Resolve on disk_task_runner
+  void ResolveImpl(const Printer::PpdReference& ppd_reference,
+                   const PpdProvider::ResolveCallback& cb) {
+    base::ThreadRestrictions::AssertIOAllowed();
+    CHECK(!cb.is_null());
     CHECK(resolve_sequence_checker_.CalledOnValidSequence());
     CHECK(base::SequencedTaskRunnerHandle::IsSet())
         << "Resolve must be called from a SequencedTaskRunner context";
@@ -82,7 +101,7 @@ class PpdProviderImpl : public PpdProvider {
     base::Optional<base::FilePath> tmp = cache_->Find(ppd_reference);
     if (tmp) {
       // Cache hit.  Schedule the callback now and return.
-      base::SequencedTaskRunnerHandle::Get()->PostTask(
+      resolve_done_task_runner_->PostTask(
           FROM_HERE, base::Bind(cb, PpdProvider::SUCCESS, tmp.value()));

[stevenjb, 2016/11/11 19:26:43]

We would need to pass a unique_ptr<FilePath> to this, set it, and return
PpdProvider::SUCCESS here.

       return;
     }
@@ -94,7 +113,7 @@ class PpdProviderImpl : public PpdProvider {
     // probably means the quirks server one doesn't work for some reason, so we
     // shouldn't silently use it.
     if (!ppd_reference.user_supplied_ppd_url.empty()) {
-      base::SequencedTaskRunnerHandle::Get()->PostTask(
+      resolve_done_task_runner_->PostTask(
           FROM_HERE, base::Bind(cb, PpdProvider::NOT_FOUND, base::FilePath()));

[stevenjb, 2016/11/11 19:26:44]

This would return PpdProvider::NOT_FOUND.

       return;
     }
@@ -111,27 +130,36 @@ class PpdProviderImpl : public PpdProvider {
         net::LOAD_DO_NOT_SAVE_COOKIES | net::LOAD_DO_NOT_SEND_COOKIES |
         net::LOAD_DO_NOT_SEND_AUTH_DATA);
     resolve_fetcher_->Start();
-  };
+  }
 
   void AbortResolve() override {
     // UrlFetcher guarantees that when the object has been destroyed, no further
     // callbacks will occur.
     resolve_fetcher_.reset();
+    resolve_done_task_runner_ = nullptr;
   }
 
   void QueryAvailable(const QueryAvailableCallback& cb) override {
     CHECK(!cb.is_null());
-    CHECK(query_sequence_checker_.CalledOnValidSequence());
-    CHECK(base::SequencedTaskRunnerHandle::IsSet())
-        << "QueryAvailable() must be called from a SequencedTaskRunner context";
     CHECK(query_fetcher_ == nullptr)
         << "Can't have concurrent PpdProvider QueryAvailable() calls";
+    CHECK(base::SequencedTaskRunnerHandle::IsSet())
+        << "QueryAvailable() must be called from a SequencedTaskRunner context";
+    query_done_task_runner_ = base::SequencedTaskRunnerHandle::Get();
+    disk_task_runner_->PostTask(FROM_HERE,
+                                base::Bind(&PpdProviderImpl::QueryAvailableImpl,
+                                           weak_factory_.GetWeakPtr(), cb));
+  }
+
+  void QueryAvailableImpl(const QueryAvailableCallback& cb) {
+    base::ThreadRestrictions::AssertIOAllowed();
+    CHECK(query_sequence_checker_.CalledOnValidSequence());
 
     base::Optional<PpdProvider::AvailablePrintersMap> result =
         cache_->FindAvailablePrinters();
     if (result) {
       // Satisfy from cache.
-      base::SequencedTaskRunnerHandle::Get()->PostTask(
+      query_done_task_runner_->PostTask(
           FROM_HERE, base::Bind(cb, PpdProvider::SUCCESS, result.value()));
       return;
     }
@@ -153,6 +181,7 @@ class PpdProviderImpl : public PpdProvider {
     // UrlFetcher guarantees that when the object has been destroyed, no further
     // callbacks will occur.
     query_fetcher_.reset();
+    query_done_task_runner_ = nullptr;
   }
 
   bool CachePpd(const Printer::PpdReference& ppd_reference,
@@ -182,20 +211,25 @@ class PpdProviderImpl : public PpdProvider {
   // fetch.
   void OnResolveFetchComplete() {
     CHECK(resolve_sequence_checker_.CalledOnValidSequence());
-    // Scope the allocated |resolve_fetcher_| into this function so we clean it
-    // up when we're done here instead of leaving it around until the next
-    // Resolve() call.
-    auto fetcher = std::move(resolve_fetcher_);
+    // Scope the allocated |resolve_fetcher_| and |resolve_done_task_runner_|
+    // into this function so we clean it up when we're done here instead of
+    // leaving it around until the next Resolve() call.
+    auto task_runner(std::move(resolve_done_task_runner_));
+    auto fetcher(std::move(resolve_fetcher_));
     std::string contents;
     if (!ValidateAndGetResponseAsString(*fetcher, &contents)) {
       // Something went wrong with the fetch.
-      resolve_done_callback_.Run(PpdProvider::SERVER_ERROR, base::FilePath());
+      task_runner->PostTask(
+          FROM_HERE, base::Bind(resolve_done_callback_,
+                                PpdProvider::SERVER_ERROR, base::FilePath()));
       return;
     }
 
     auto dict = base::DictionaryValue::From(base::JSONReader::Read(contents));
     if (dict == nullptr) {
-      resolve_done_callback_.Run(PpdProvider::SERVER_ERROR, base::FilePath());
+      task_runner->PostTask(
+          FROM_HERE, base::Bind(resolve_done_callback_,
+                                PpdProvider::SERVER_ERROR, base::FilePath()));
       return;
     }
     std::string ppd_contents;
@@ -205,7 +239,9 @@ class PpdProviderImpl : public PpdProvider {
           dict->GetString(kJSONLastUpdatedKey, &last_updated_time_string) &&
           base::StringToUint64(last_updated_time_string, &last_updated_time))) {
       // Malformed response.  TODO(justincarlson) - LOG something here?
-      resolve_done_callback_.Run(PpdProvider::SERVER_ERROR, base::FilePath());
+      task_runner->PostTask(
+          FROM_HERE, base::Bind(resolve_done_callback_,
+                                PpdProvider::SERVER_ERROR, base::FilePath()));
       return;
     }
 
@@ -224,25 +260,31 @@ class PpdProviderImpl : public PpdProvider {
     auto ppd_file = cache_->Store(resolve_reference_, ppd_contents);
     if (!ppd_file) {
       // Failed to store.
-      resolve_done_callback_.Run(PpdProvider::INTERNAL_ERROR, base::FilePath());
+      task_runner->PostTask(
+          FROM_HERE, base::Bind(resolve_done_callback_,
+                                PpdProvider::INTERNAL_ERROR, base::FilePath()));
       return;
     }
-    resolve_done_callback_.Run(PpdProvider::SUCCESS, ppd_file.value());
+    task_runner->PostTask(
+        FROM_HERE, base::Bind(resolve_done_callback_, PpdProvider::SUCCESS,
+                              ppd_file.value()));
   }
 
   // Called on the same thread as QueryAvailable() when the fetcher completes
   // its fetch.
   void OnQueryAvailableFetchComplete() {
     CHECK(query_sequence_checker_.CalledOnValidSequence());
-    // Scope the object fetcher into this function so we clean it up when we're
-    // done here instead of leaving it around until the next QueryAvailable()
-    // call.
-    auto fetcher = std::move(query_fetcher_);
+    // Scope the object fetcher and task_runner into this function so we clean
+    // it up when we're done here instead of leaving it around until the next
+    // QueryAvailable() call.
+    auto task_runner(std::move(query_done_task_runner_));
+    auto fetcher(std::move(query_fetcher_));
     std::string contents;
     if (!ValidateAndGetResponseAsString(*fetcher, &contents)) {
       // Something went wrong with the fetch.
-      query_done_callback_.Run(PpdProvider::SERVER_ERROR,
-                               AvailablePrintersMap());
+      task_runner->PostTask(
+          FROM_HERE, base::Bind(query_done_callback_, PpdProvider::SERVER_ERROR,
+                                AvailablePrintersMap()));
       return;
     }
 
@@ -253,8 +295,9 @@ class PpdProviderImpl : public PpdProvider {
     const base::ListValue* top_list;
     if (top_dict == nullptr || !top_dict->GetList(kJSONTopListKey, &top_list)) {
       LOG(ERROR) << "Malformed response from quirks server";
-      query_done_callback_.Run(PpdProvider::SERVER_ERROR,
-                               PpdProvider::AvailablePrintersMap());
+      task_runner->PostTask(
+          FROM_HERE, base::Bind(query_done_callback_, PpdProvider::SERVER_ERROR,
+                                AvailablePrintersMap()));
       return;
     }
 
@@ -282,6 +325,7 @@ class PpdProviderImpl : public PpdProvider {
         }
       }
     }
+
     if (!result.empty()) {
       cache_->StoreAvailablePrinters(result);
     } else {
@@ -291,7 +335,8 @@ class PpdProviderImpl : public PpdProvider {
       LOG(ERROR) << "Available printers map is unexpectedly empty.  Refusing "
                     "to cache this.";
     }
-    query_done_callback_.Run(PpdProvider::SUCCESS, result);
+    task_runner->PostTask(FROM_HERE, base::Bind(query_done_callback_,
+                                                PpdProvider::SUCCESS, result));
   }
 
   // Generate a url to look up a manufacturer/model from the quirks server
@@ -326,13 +371,17 @@ class PpdProviderImpl : public PpdProvider {
   // Reference we're currently trying to resolve.
   Printer::PpdReference resolve_reference_;
 
+  // TaskRunner on which to invoke the Resolve callback;
+  scoped_refptr<base::SequencedTaskRunner> resolve_done_task_runner_;
+
   // Callback to invoke on completion.
   PpdProvider::ResolveCallback resolve_done_callback_;
 
   // Check that Resolve() and its callback are sequenced appropriately.
   base::SequenceChecker resolve_sequence_checker_;
 
-  // Fetcher for the current call, if any.
+  // Fetcher for the current call, if any.  resolve_fetcher_ is null if there
+  // is no running Resolve().
   std::unique_ptr<net::URLFetcher> resolve_fetcher_;
 
   // State held across a QueryAvailable() call.
@@ -340,6 +389,9 @@ class PpdProviderImpl : public PpdProvider {
   // Callback to invoke on completion.
   PpdProvider::QueryAvailableCallback query_done_callback_;
 
+  // TaskRunner on which to invoke the Query Available callback;
+  scoped_refptr<base::SequencedTaskRunner> query_done_task_runner_;
+
   // Check that QueryAvailable() and its callback are sequenced appropriately.
   base::SequenceChecker query_sequence_checker_;
 
@@ -353,10 +405,13 @@ class PpdProviderImpl : public PpdProvider {
 
   ForwardingURLFetcherDelegate forwarding_delegate_;
   scoped_refptr<net::URLRequestContextGetter> url_context_getter_;
+  scoped_refptr<base::SequencedTaskRunner> disk_task_runner_;
   std::unique_ptr<PpdCache> cache_;
 
   // Construction-time options, immutable.
   const PpdProvider::Options options_;
+
+  base::WeakPtrFactory<PpdProviderImpl> weak_factory_;
 };
 
 void ForwardingURLFetcherDelegate::OnURLFetchComplete(
@@ -370,10 +425,11 @@ void ForwardingURLFetcherDelegate::OnURLFetchComplete(
 std::unique_ptr<PpdProvider> PpdProvider::Create(
     const std::string& api_key,
     scoped_refptr<net::URLRequestContextGetter> url_context_getter,
+    scoped_refptr<base::SequencedTaskRunner> disk_task_runner,
     std::unique_ptr<PpdCache> cache,
     const PpdProvider::Options& options) {
-  return base::MakeUnique<PpdProviderImpl>(api_key, url_context_getter,
-                                           std::move(cache), options);
+  return base::MakeUnique<PpdProviderImpl>(
+      api_key, url_context_getter, disk_task_runner, std::move(cache), options);
 }
 
 }  // namespace printing