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..250e6ea15566206ca94fe47ea0881cf8dce44c23 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"
@@ -56,16 +57,18 @@ class ForwardingURLFetcherDelegate : public net::URLFetcherDelegate {
 
 class PpdProviderImpl : public PpdProvider {
  public:
-  PpdProviderImpl(
-      const std::string& api_key,
-      scoped_refptr<net::URLRequestContextGetter> url_context_getter,
-      std::unique_ptr<PpdCache> cache,
-      const PpdProvider::Options& options)
+  PpdProviderImpl(const std::string& api_key,
+                  net::URLRequestContextGetter* url_context_getter,
+                  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 +76,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)

[skau, 2016/11/09 00:01:09]

Can you add that you're using resolve_fetcher_ == null to check for running
fetches on the member variable comment.

[Carlson, 2016/11/10 19:22:50]

Done.

+        << "Can't have concurrent PpdProvider Resolve calls";
+    resolve_done_task_runner_ = base::SequencedTaskRunnerHandle::Get();
+    disk_task_runner_->PostTask(
+        FROM_HERE, base::Bind(&PpdProviderImpl::ResolveImpl,
+                              weak_factory_.GetWeakPtr(), ppd_reference, cb));
+  }
+
+  // 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 +100,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()));
       return;
     }
@@ -94,7 +112,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()));
       return;
     }
@@ -111,7 +129,7 @@ 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
@@ -121,17 +139,25 @@ class PpdProviderImpl : public PpdProvider {
 
   void QueryAvailable(const QueryAvailableCallback& cb) override {

[skau, 2016/11/10 02:38:41]

It looks like model and manufacturer is being queried concurrently and causing a
crash.  Can you look into it?

[Carlson, 2016/11/10 19:22:50]

We chatted offline about this.  Being fixed not in this cl.

     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;
     }
@@ -189,13 +215,17 @@ class PpdProviderImpl : public PpdProvider {
     std::string contents;
     if (!ValidateAndGetResponseAsString(*fetcher, &contents)) {
       // Something went wrong with the fetch.
-      resolve_done_callback_.Run(PpdProvider::SERVER_ERROR, base::FilePath());
+      resolve_done_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());
+      resolve_done_task_runner_->PostTask(
+          FROM_HERE, base::Bind(resolve_done_callback_,
+                                PpdProvider::SERVER_ERROR, base::FilePath()));
       return;
     }
     std::string ppd_contents;
@@ -205,7 +235,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());
+      resolve_done_task_runner_->PostTask(
+          FROM_HERE, base::Bind(resolve_done_callback_,
+                                PpdProvider::SERVER_ERROR, base::FilePath()));
       return;
     }
 
@@ -224,10 +256,14 @@ 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());
+      resolve_done_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());
+    resolve_done_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
@@ -241,8 +277,9 @@ class PpdProviderImpl : public PpdProvider {
     std::string contents;
     if (!ValidateAndGetResponseAsString(*fetcher, &contents)) {
       // Something went wrong with the fetch.
-      query_done_callback_.Run(PpdProvider::SERVER_ERROR,
-                               AvailablePrintersMap());
+      query_done_task_runner_->PostTask(
+          FROM_HERE, base::Bind(query_done_callback_, PpdProvider::SERVER_ERROR,
+                                AvailablePrintersMap()));
       return;
     }
 
@@ -253,8 +290,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());
+      query_done_task_runner_->PostTask(
+          FROM_HERE, base::Bind(query_done_callback_, PpdProvider::SERVER_ERROR,
+                                AvailablePrintersMap()));
       return;
     }
 
@@ -291,7 +329,9 @@ class PpdProviderImpl : public PpdProvider {
       LOG(ERROR) << "Available printers map is unexpectedly empty.  Refusing "
                     "to cache this.";
     }
-    query_done_callback_.Run(PpdProvider::SUCCESS, result);
+    query_done_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,6 +366,9 @@ 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_;
 
@@ -340,6 +383,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 +399,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(
@@ -369,11 +418,12 @@ void ForwardingURLFetcherDelegate::OnURLFetchComplete(
 // static
 std::unique_ptr<PpdProvider> PpdProvider::Create(
     const std::string& api_key,
-    scoped_refptr<net::URLRequestContextGetter> url_context_getter,
+    net::URLRequestContextGetter* url_context_getter,
+    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