Update PpdProvider threading model.

chromeos/printing/ppd_provider.cc

 // Copyright 2016 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.
 
 #include "chromeos/printing/ppd_provider.h"
 
 #include <utility>
 
 #include "base/files/file_util.h"
 #include "base/json/json_parser.h"
 #include "base/memory/ptr_util.h"
 #include "base/sequence_checker.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
+#include "base/task_runner_util.h"
 #include "base/threading/sequenced_task_runner_handle.h"
 #include "base/time/time.h"
 #include "base/values.h"
 #include "chromeos/printing/ppd_cache.h"
 #include "net/base/load_flags.h"
 #include "net/http/http_status_code.h"
 #include "net/url_request/url_fetcher.h"
 #include "net/url_request/url_fetcher_delegate.h"
 #include "net/url_request/url_request_context_getter.h"
 #include "url/gurl.h"
 
 namespace chromeos {
 namespace printing {
 namespace {
 
 // Expected fields from the quirks server.
 const char kJSONPPDKey[] = "compressedPpd";
 const char kJSONLastUpdatedKey[] = "lastUpdatedTime";
 const char kJSONTopListKey[] = "manufacturers";
 const char kJSONManufacturer[] = "manufacturer";
 const char kJSONModelList[] = "models";
 
 class PpdProviderImpl;
 
-// URLFetcherDelegate that just forwards the complete callback back to
-// the PpdProvider that owns the delegate.
+// URLFetcherDelegate that just invokes a callback when the fetch is complete.
 class ForwardingURLFetcherDelegate : public net::URLFetcherDelegate {
  public:
-  explicit ForwardingURLFetcherDelegate(PpdProviderImpl* owner)
-      : owner_(owner) {}
+  explicit ForwardingURLFetcherDelegate(
+      const base::Callback<void()>& done_callback)
+      : done_callback_(done_callback) {}
   ~ForwardingURLFetcherDelegate() override {}
 
   // URLFetcherDelegate API method.  Defined below since we need the
   // PpdProviderImpl definition first.
-  void OnURLFetchComplete(const net::URLFetcher* source) override;
+  void OnURLFetchComplete(const net::URLFetcher* source) override {
+    done_callback_.Run();
+  }
 
  private:
-  // owner of this delegate.
-  PpdProviderImpl* const owner_;
+  // Callback to be run on fetch complete.
+  base::Callback<void()> done_callback_;
 };
 
 class PpdProviderImpl : public PpdProvider {
  public:
   PpdProviderImpl(
       const std::string& api_key,
       scoped_refptr<net::URLRequestContextGetter> url_context_getter,
+      scoped_refptr<base::SequencedTaskRunner> io_task_runner,
       std::unique_ptr<PpdCache> cache,
       const PpdProvider::Options& options)
       : api_key_(api_key),
-        forwarding_delegate_(this),
         url_context_getter_(url_context_getter),
+        io_task_runner_(io_task_runner),
         cache_(std::move(cache)),
-        options_(options) {
+        options_(options),
+        weak_factory_(this) {
     CHECK_GT(options_.max_ppd_contents_size_, 0U);
   }
   ~PpdProviderImpl() override {}
 
   void Resolve(const Printer::PpdReference& ppd_reference,
                const PpdProvider::ResolveCallback& cb) override {
     CHECK(!cb.is_null());
     CHECK(resolve_sequence_checker_.CalledOnValidSequence());
     CHECK(base::SequencedTaskRunnerHandle::IsSet())
         << "Resolve must be called from a SequencedTaskRunner context";
-    CHECK(resolve_fetcher_ == nullptr)
+    CHECK(!resolve_inflight_)
         << "Can't have concurrent PpdProvider Resolve calls";
+    resolve_inflight_ = true;
+    auto cache_result = base::MakeUnique<base::Optional<base::FilePath>>();
+    CHECK(io_task_runner_->PostTaskAndReply(

[stevenjb, 2016/11/14 23:08:39]

Don't wrap non test function calls with CHECK, instead store the result and
CHECK it separately. (i.e. changing CHECK to DCHECK shouldn't change the logic).

[Carlson, 2016/11/14 23:27:41]

Done, but for my own edification, is this just your personal preference, or
something style-guide related?

[stevenjb, 2016/11/14 23:55:25]

It's not explicitly in the guidelines, however this actually ought to be a
DCHECK, see the description:

https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

So, because CHECK should be used sparingly, the pattern of CHECK(DoSomething())
is rare.

Also, FWIW, I rarely see the result of PostTask checked; if PostTask is failing,
it will almost certainly be caught at a higher level.

+        FROM_HERE, base::Bind(&PpdProviderImpl::CacheFindWrapper,

[stevenjb, 2016/11/14 23:08:39]

nit: This will be easier to follow if the method is named something like
ResolveDoCacheLookup.

[Carlson, 2016/11/14 23:27:41]

Done.

+                              weak_factory_.GetWeakPtr(), ppd_reference,
+                              cache_result.get()),
+        base::Bind(&PpdProviderImpl::ResolveCacheLookupDone,
+                   weak_factory_.GetWeakPtr(), ppd_reference, cb,
+                   std::move(cache_result))));
+  }
 
-    base::Optional<base::FilePath> tmp = cache_->Find(ppd_reference);
-    if (tmp) {
+  void QueryAvailable(const QueryAvailableCallback& cb) override {
+    CHECK(!cb.is_null());
+    CHECK(base::SequencedTaskRunnerHandle::IsSet())
+        << "QueryAvailable() must be called from a SequencedTaskRunner context";
+    auto cache_result = base::MakeUnique<
+        base::Optional<const PpdProvider::AvailablePrintersMap*>>();
+    CHECK(!query_inflight_)
+        << "Can't have concurrent PpdProvider QueryAvailable calls";
+    query_inflight_ = true;
+    CHECK(io_task_runner_->PostTaskAndReply(
+        FROM_HERE, base::Bind(&PpdProviderImpl::CacheFindAvailableWrapper,

[stevenjb, 2016/11/14 23:08:39]

QueryAvailableDoCacheLookup

[Carlson, 2016/11/14 23:27:41]

Done.

+                              weak_factory_.GetWeakPtr(), cache_result.get()),
+        base::Bind(&PpdProviderImpl::QueryCacheLookupDone,

[stevenjb, 2016/11/14 23:08:39]

QueryAvailableCacheLookupDone

[Carlson, 2016/11/14 23:27:41]

Done.

+                   weak_factory_.GetWeakPtr(), cb, std::move(cache_result))));
+  }
+
+  bool CachePpd(const Printer::PpdReference& ppd_reference,
+                const base::FilePath& ppd_path) override {
+    std::string buf;
+    if (!base::ReadFileToStringWithMaxSize(ppd_path, &buf,
+                                           options_.max_ppd_contents_size_)) {
+      return false;
+    }
+    return static_cast<bool>(cache_->Store(ppd_reference, buf));
+  }
+
+ private:
+  // Trivial wrappers around PpdCache::Find() and
+  // PpdCache::FindAvailablePrinters().  We need these wrappers both because we
+  // use weak_ptrs to manage lifetime, and so so we need to bind callbacks to

[stevenjb, 2016/11/14 23:08:39]

s/so so/because/

[Carlson, 2016/11/14 23:27:41]

Done.

+  // *this*, and because weak_ptr's preclude return values in posted tasks, so
+  // we have to use a parameter to return state.

[stevenjb, 2016/11/14 23:08:39]

FWIW, this isn't that uncommon a pattern, you could probably simplify the
explanation, e.g.:

// Do the cache lookup on the IO thread and set |cache_result| to pass
// too ResolveCacheLookupDone.

The only reason that "weak_ptr's preclude return values in posted tasks" was an
issue is because I suggested PostTaskAndReplyWithResult without considering the
lifetime of |cache_| (which is why this needs to be a member function). It's
subtle, and something you will remember next time it comes up, but not
necessarily relevant to the next maintainer :)

[Carlson, 2016/11/14 23:27:41]

From the perspective of someone new to this code base, I'm actually pretty
convinced that the Chromium code base is rather heavy on undocumented convention
and subtleties that surprise a new person to the code.  So I'd prefer to err on
the side of too much information about why this is, rather than too little.

[stevenjb, 2016/11/14 23:55:25]

Acknowledged, but over-documentation is it's own problem. Anyway, it's fine.

+  void CacheFindWrapper(const Printer::PpdReference& reference,
+                        base::Optional<base::FilePath>* cache_result) const {
+    *cache_result = cache_->Find(reference);
+  }

[stevenjb, 2016/11/14 23:08:39]

blank line

[Carlson, 2016/11/14 23:27:41]

Done.

+  void CacheFindAvailableWrapper(
+      base::Optional<const PpdProvider::AvailablePrintersMap*>* cache_result)
+      const {
+    auto tmp = cache_->FindAvailablePrinters();
+    if (tmp != nullptr) {
+      *cache_result = tmp;
+    } else {
+      *cache_result = base::nullopt;
+    }
+  }
+
+  // Callback that happens when the Resolve() cache lookup completes.  If the
+  // cache satisfied the request, finish the Resolve, otherwise start a URL
+  // request to satisfy the request.  This runs on the same thread as Resolve()
+  // was invoked on.

[stevenjb, 2016/11/14 23:08:39]

Last sentence is unnecessary, we have a CHECK on line 152 that makes that clear.

[Carlson, 2016/11/14 23:27:41]

Sort of, the CHECK below just guarantees sequence, not thread affinity, but
generally again I'd really like to err on the side of too much explanation than
too little here.

[stevenjb, 2016/11/14 23:55:25]

Sure, but in practice this will only get called from the UI thread or a
SingleThreadTaskRunner for the UI thread. Again, we aim for sparse, focused
comments that are not clear from inspection. This is called from
io_task_runner_->PostTaskAndReply, which is well documented.

+  void ResolveCacheLookupDone(
+      const Printer::PpdReference& ppd_reference,
+      const PpdProvider::ResolveCallback& done_callback,
+      const std::unique_ptr<base::Optional<base::FilePath>>& cache_result) {
+    CHECK(resolve_sequence_checker_.CalledOnValidSequence());
+    if (*cache_result) {
       // Cache hit.  Schedule the callback now and return.
-      base::SequencedTaskRunnerHandle::Get()->PostTask(
-          FROM_HERE, base::Bind(cb, PpdProvider::SUCCESS, tmp.value()));
+      resolve_inflight_ = false;
+      done_callback.Run(PpdProvider::SUCCESS, cache_result->value());
       return;
     }
 
     // We don't have a way to automatically resolve user-supplied PPDs yet.  So
     // if we have one specified, and it's not cached, we fail out rather than
     // fall back to quirks-server based resolution.  The reasoning here is that
     // if the user has specified a PPD when a quirks-server one exists, it
     // 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(
-          FROM_HERE, base::Bind(cb, PpdProvider::NOT_FOUND, base::FilePath()));
+      resolve_inflight_ = false;
+      done_callback.Run(PpdProvider::NOT_FOUND, base::FilePath());
       return;
     }
 
-    resolve_reference_ = ppd_reference;
-    resolve_done_callback_ = cb;
+    // Missed in the cache, so start a URLRequest to resolve the request.
+    resolve_fetcher_delegate_ = base::MakeUnique<ForwardingURLFetcherDelegate>(
+        base::Bind(&PpdProviderImpl::OnResolveFetchComplete,
+                   weak_factory_.GetWeakPtr(), ppd_reference, done_callback));
 
-    resolve_fetcher_ =
-        net::URLFetcher::Create(GetQuirksServerPpdLookupURL(ppd_reference),
-                                net::URLFetcher::GET, &forwarding_delegate_);
+    resolve_fetcher_ = net::URLFetcher::Create(
+        GetQuirksServerPpdLookupURL(ppd_reference), net::URLFetcher::GET,
+        resolve_fetcher_delegate_.get());
+
     resolve_fetcher_->SetRequestContext(url_context_getter_.get());
     resolve_fetcher_->SetLoadFlags(
         net::LOAD_BYPASS_CACHE | net::LOAD_DISABLE_CACHE |
         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();
   }
 
-  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";
-
-    const PpdProvider::AvailablePrintersMap* result =
-        cache_->FindAvailablePrinters();
-    if (result != nullptr) {
-      // Satisfy from cache.
-      base::SequencedTaskRunnerHandle::Get()->PostTask(
-          FROM_HERE, base::Bind(cb, PpdProvider::SUCCESS, *result));
-      return;
-    }
-    // Not in the cache, ask QuirksServer.
-    query_done_callback_ = cb;
-
-    query_fetcher_ =
-        net::URLFetcher::Create(GetQuirksServerPpdListURL(),
-                                net::URLFetcher::GET, &forwarding_delegate_);
-    query_fetcher_->SetRequestContext(url_context_getter_.get());
-    query_fetcher_->SetLoadFlags(
-        net::LOAD_BYPASS_CACHE | net::LOAD_DISABLE_CACHE |
-        net::LOAD_DO_NOT_SAVE_COOKIES | net::LOAD_DO_NOT_SEND_COOKIES |
-        net::LOAD_DO_NOT_SEND_AUTH_DATA);
-    query_fetcher_->Start();
-  }
-
-  void AbortQueryAvailable() override {
-    // UrlFetcher guarantees that when the object has been destroyed, no further
-    // callbacks will occur.
-    query_fetcher_.reset();
-  }
-
-  bool CachePpd(const Printer::PpdReference& ppd_reference,
-                const base::FilePath& ppd_path) override {
-    std::string buf;
-    if (!base::ReadFileToStringWithMaxSize(ppd_path, &buf,
-                                           options_.max_ppd_contents_size_)) {
-      return false;
-    }
-    return static_cast<bool>(cache_->Store(ppd_reference, buf));
-  }
-
-  // Route to the proper fetch complete handler based on which fetcher caused
-  // it.
-  void OnURLFetchComplete(const net::URLFetcher* fetcher) {
-    if (fetcher == resolve_fetcher_.get()) {
-      OnResolveFetchComplete();
-    } else if (fetcher == query_fetcher_.get()) {
-      OnQueryAvailableFetchComplete();
-    } else {
-      NOTREACHED() << "Unknown fetcher completed.";
-    }
-  }
-
- private:
   // Called on the same thread as Resolve() when the fetcher completes its
   // fetch.
-  void OnResolveFetchComplete() {
+  void OnResolveFetchComplete(
+      const Printer::PpdReference& ppd_reference,
+      const PpdProvider::ResolveCallback& done_callback) {
     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_fetcher_delegate_|
+    // 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_delegate(std::move(resolve_fetcher_delegate_));

[stevenjb, 2016/11/14 23:08:39]

nit: we don't appear to need fetcher_delegate below, so we can just reset()
resolve_fetcher_delegate_.

[Carlson, 2016/11/14 23:27:41]

The delegate holds the callback *currently being invoked*, which I think means
it also implicitly holds the memory for the (const reference) parameters being
passed into this function, doesn't it?

(I may have this wrong, the subtleties of Bind() and related wizardry are deep
and mysterious to me.  :))

[stevenjb, 2016/11/14 23:55:25]

Once the function object is invoked, any bound parameters will be on the stack.
Still, that's potentially confusing. I'm sure there's a better model for this,
but it's fine as-is.

+    auto fetcher(std::move(resolve_fetcher_));
+    resolve_inflight_ = false;
     std::string contents;
     if (!ValidateAndGetResponseAsString(*fetcher, &contents)) {
       // Something went wrong with the fetch.
-      resolve_done_callback_.Run(PpdProvider::SERVER_ERROR, base::FilePath());
+      done_callback.Run(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());
+      done_callback.Run(PpdProvider::SERVER_ERROR, base::FilePath());
       return;
     }
     std::string ppd_contents;
     std::string last_updated_time_string;
     uint64_t last_updated_time;
     if (!(dict->GetString(kJSONPPDKey, &ppd_contents) &&
           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());
+      done_callback.Run(PpdProvider::SERVER_ERROR, base::FilePath());
       return;
     }
 
     if (ppd_contents.size() > options_.max_ppd_contents_size_) {
       // PPD is too big.
       //
       // Note -- if we ever add shared-ppd-sourcing, e.g. we may serve a ppd to
       // a user that's not from an explicitly trusted source, we should also
       // check *uncompressed* size here to head off zip-bombs (e.g. let's
       // compress 1GBs of zeros into a 900kb file and see what cups does when it
       // tries to expand that...)
-      resolve_done_callback_.Run(PpdProvider::SERVER_ERROR, base::FilePath());
+      done_callback.Run(PpdProvider::SERVER_ERROR, base::FilePath());
       return;
     }
 
-    auto ppd_file = cache_->Store(resolve_reference_, ppd_contents);
+    auto ppd_file = cache_->Store(ppd_reference, ppd_contents);
     if (!ppd_file) {
       // Failed to store.
-      resolve_done_callback_.Run(PpdProvider::INTERNAL_ERROR, base::FilePath());
+      done_callback.Run(PpdProvider::INTERNAL_ERROR, base::FilePath());
       return;
     }
-    resolve_done_callback_.Run(PpdProvider::SUCCESS, ppd_file.value());
+    done_callback.Run(PpdProvider::SUCCESS, ppd_file.value());
   }
 
-  // Called on the same thread as QueryAvailable() when the fetcher completes
-  // its fetch.
-  void OnQueryAvailableFetchComplete() {
+  // Called on the same thread as QueryAvailable() when the cache lookup is
+  // done.  If the cache satisfied the request, finish the Query, otherwise
+  // start a URL request to satisfy the Query.  This runs on the same thread as
+  // QueryAvailable() was invoked on.
+  void QueryCacheLookupDone(
+      const PpdProvider::QueryAvailableCallback& done_callback,
+      const std::unique_ptr<
+          base::Optional<const PpdProvider::AvailablePrintersMap*>>&
+          cache_result) {
     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_);
+    if (*cache_result) {
+      query_inflight_ = false;
+      done_callback.Run(PpdProvider::SUCCESS, *cache_result->value());
+      return;
+    }
+    // Missed in the cache, start a query.
+    query_fetcher_delegate_ = base::MakeUnique<ForwardingURLFetcherDelegate>(
+        base::Bind(&PpdProviderImpl::OnQueryAvailableFetchComplete,
+                   weak_factory_.GetWeakPtr(), done_callback));
+
+    query_fetcher_ = net::URLFetcher::Create(GetQuirksServerPpdListURL(),
+                                             net::URLFetcher::GET,
+                                             query_fetcher_delegate_.get());
+    query_fetcher_->SetRequestContext(url_context_getter_.get());
+    query_fetcher_->SetLoadFlags(
+        net::LOAD_BYPASS_CACHE | net::LOAD_DISABLE_CACHE |
+        net::LOAD_DO_NOT_SAVE_COOKIES | net::LOAD_DO_NOT_SEND_COOKIES |
+        net::LOAD_DO_NOT_SEND_AUTH_DATA);
+    query_fetcher_->Start();
+  }
+
+  void OnQueryAvailableFetchComplete(
+      const PpdProvider::QueryAvailableCallback& done_callback) {
+    CHECK(query_sequence_checker_.CalledOnValidSequence());
+    // 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 fetcher_delegate(std::move(query_fetcher_delegate_));

[stevenjb, 2016/11/14 23:08:39]

Also unused, can use reset().

[Carlson, 2016/11/14 23:27:41]

See above

+    auto fetcher(std::move(query_fetcher_));
+    query_inflight_ = false;
     std::string contents;
     if (!ValidateAndGetResponseAsString(*fetcher, &contents)) {
       // Something went wrong with the fetch.
-      query_done_callback_.Run(PpdProvider::SERVER_ERROR,
-                               AvailablePrintersMap());
+      done_callback.Run(PpdProvider::SERVER_ERROR, AvailablePrintersMap());
       return;
     }
 
     // The server gives us JSON in the form of a list of (manufacturer, list of
     // models) tuples.
     auto top_dict =
         base::DictionaryValue::From(base::JSONReader::Read(contents));
     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());
+      done_callback.Run(PpdProvider::SERVER_ERROR, AvailablePrintersMap());
       return;
     }
 
     auto result = base::MakeUnique<PpdProvider::AvailablePrintersMap>();
     for (const std::unique_ptr<base::Value>& entry : *top_list) {
       base::DictionaryValue* dict;
       std::string manufacturer;
       std::string model;
       base::ListValue* model_list;
       if (!entry->GetAsDictionary(&dict) ||
           !dict->GetString(kJSONManufacturer, &manufacturer) ||
           !dict->GetList(kJSONModelList, &model_list)) {
         LOG(ERROR) << "Unexpected contents in quirks server printer list.";
         // Just skip this entry instead of aborting the whole thing.
         continue;
       }
 
       std::vector<std::string>& dest = (*result)[manufacturer];
       for (const std::unique_ptr<base::Value>& model_value : *model_list) {
         if (model_value->GetAsString(&model)) {
           dest.push_back(model);
         } else {
           LOG(ERROR) << "Skipping unknown model for manufacturer "
                      << manufacturer;
         }
       }
     }
-    query_done_callback_.Run(PpdProvider::SUCCESS, *result);
+    done_callback.Run(PpdProvider::SUCCESS, *result);
     if (!result->empty()) {
       cache_->StoreAvailablePrinters(std::move(result));
     } else {
       // An empty map means something is probably wrong; if we cache this map,
       // we'll have an empty map until the cache expires.  So complain and
       // refuse to cache.
       LOG(ERROR) << "Available printers map is unexpectedly empty.  Refusing "
                     "to cache this.";
     }
   }
@@ skipping 20 matching lines @@
   // return true, otherwise return false.
   bool ValidateAndGetResponseAsString(const net::URLFetcher& fetcher,
                                       std::string* contents) {
     return ((fetcher.GetStatus().status() == net::URLRequestStatus::SUCCESS) &&
             (fetcher.GetResponseCode() == net::HTTP_OK) &&
             fetcher.GetResponseAsString(contents));
   }
 
   // State held across a Resolve() call.
 
-  // Reference we're currently trying to resolve.
-  Printer::PpdReference resolve_reference_;
-
-  // 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 and associated delegate for the current Resolve() call, if a fetch
+  // is in progress.  These are both null if no Resolve() is in flight.
   std::unique_ptr<net::URLFetcher> resolve_fetcher_;
+  std::unique_ptr<ForwardingURLFetcherDelegate> resolve_fetcher_delegate_;
+  // Is there a current Resolve() inflight?  Used to help fail-fast in the case
+  // of inappropriate concurrent usage.
+  bool resolve_inflight_ = false;
 
   // State held across a QueryAvailable() call.
 
-  // Callback to invoke on completion.
-  PpdProvider::QueryAvailableCallback query_done_callback_;
-
   // Check that QueryAvailable() and its callback are sequenced appropriately.
   base::SequenceChecker query_sequence_checker_;
 
-  // Fetcher for the current call, if any.
+  // Fetcher and associated delegate for the current QueryAvailable() call, if a
+  // fetch is in progress.  These are both null if no QueryAvailable() is in
+  // flight.
   std::unique_ptr<net::URLFetcher> query_fetcher_;
+  std::unique_ptr<ForwardingURLFetcherDelegate> query_fetcher_delegate_;
+  // Is there a current QueryAvailable() inflight?  Used to help fail-fast in
+  // the case of inappropriate concurrent usage.
+  bool query_inflight_ = false;
 
   // Common state.
 
   // API key for accessing quirks server.
   const std::string api_key_;
 
-  ForwardingURLFetcherDelegate forwarding_delegate_;
   scoped_refptr<net::URLRequestContextGetter> url_context_getter_;
+  scoped_refptr<base::SequencedTaskRunner> io_task_runner_;
   std::unique_ptr<PpdCache> cache_;
 
   // Construction-time options, immutable.
   const PpdProvider::Options options_;
+
+  base::WeakPtrFactory<PpdProviderImpl> weak_factory_;
 };
 
-void ForwardingURLFetcherDelegate::OnURLFetchComplete(
-    const net::URLFetcher* source) {
-  owner_->OnURLFetchComplete(source);
-}
-
 }  // namespace
 
 // static
 std::unique_ptr<PpdProvider> PpdProvider::Create(
     const std::string& api_key,
     scoped_refptr<net::URLRequestContextGetter> url_context_getter,
+    scoped_refptr<base::SequencedTaskRunner> io_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, io_task_runner, std::move(cache), options);
 }
 
 }  // namespace printing
 }  // namespace chromeos