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 c35e213e9221be95de36fefe34b0dac3bf282b26..3d1071a42f201dc402e6205a5449bd4743dd09f2 100644
--- a/chromeos/printing/ppd_provider.cc
+++ b/chromeos/printing/ppd_provider.cc
@@ -13,6 +13,7 @@
 #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"
@@ -37,21 +38,23 @@ 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 {
@@ -59,13 +62,15 @@ class PpdProviderImpl : public PpdProvider {
   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 {}
@@ -76,14 +81,82 @@ class PpdProviderImpl : public PpdProvider {
     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>>();
+    bool post_result = io_task_runner_->PostTaskAndReply(
+        FROM_HERE, base::Bind(&PpdProviderImpl::ResolveDoCacheLookup,
+                              weak_factory_.GetWeakPtr(), ppd_reference,
+                              cache_result.get()),
+        base::Bind(&PpdProviderImpl::ResolveCacheLookupDone,
+                   weak_factory_.GetWeakPtr(), ppd_reference, cb,
+                   std::move(cache_result)));
+    DCHECK(post_result);
+  }
+
+  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::QueryAvailableDoCacheLookup,
+                              weak_factory_.GetWeakPtr(), cache_result.get()),
+        base::Bind(&PpdProviderImpl::QueryAvailableCacheLookupDone,
+                   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));
+  }
 
-    base::Optional<base::FilePath> tmp = cache_->Find(ppd_reference);
-    if (tmp) {
+ private:
+  // Trivial wrappers around PpdCache::Find() and
+  // PpdCache::FindAvailablePrinters().  We need these wrappers both because we
+  // use weak_ptrs to manage lifetime, and so we need to bind callbacks to
+  // *this*, and because weak_ptr's preclude return values in posted tasks, so
+  // we have to use a parameter to return state.
+  void ResolveDoCacheLookup(
+      const Printer::PpdReference& reference,
+      base::Optional<base::FilePath>* cache_result) const {
+    *cache_result = cache_->Find(reference);
+  }
+
+  void QueryAvailableDoCacheLookup(
+      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.
+  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;
     }
 
@@ -94,108 +167,50 @@ 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(
-          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,
+        resolve_fetcher_delegate_.get());
 
-    resolve_fetcher_ =
-        net::URLFetcher::Create(GetQuirksServerPpdLookupURL(ppd_reference),
-                                net::URLFetcher::GET, &forwarding_delegate_);
     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_));
+    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;
@@ -205,7 +220,7 @@ 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());
+      done_callback.Run(PpdProvider::SERVER_ERROR, base::FilePath());
       return;
     }
 
@@ -217,32 +232,63 @@ class PpdProviderImpl : public PpdProvider {
       // 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 QueryAvailableCacheLookupDone(
+      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_));
+    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;
     }
 
@@ -253,8 +299,7 @@ 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());
+      done_callback.Run(PpdProvider::SERVER_ERROR, AvailablePrintersMap());
       return;
     }
 
@@ -282,7 +327,7 @@ class PpdProviderImpl : public PpdProvider {
         }
       }
     }
-    query_done_callback_.Run(PpdProvider::SUCCESS, *result);
+    done_callback.Run(PpdProvider::SUCCESS, *result);
     if (!result->empty()) {
       cache_->StoreAvailablePrinters(std::move(result));
     } else {
@@ -323,46 +368,45 @@ class PpdProviderImpl : public PpdProvider {
 
   // 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_;
-};
 
-void ForwardingURLFetcherDelegate::OnURLFetchComplete(
-    const net::URLFetcher* source) {
-  owner_->OnURLFetchComplete(source);
-}
+  base::WeakPtrFactory<PpdProviderImpl> weak_factory_;
+};
 
 }  // namespace
 
@@ -370,10 +414,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> 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