Releasing cached URLLoaderFactory* in PpdProvider

The URLLoaderFactory interface grabbed from g_process can become
invalidated, like on network changes, so we can't save it. This change
modifies PpdProvider to grab it fresh before every fetch.

Test: chromeos_unittests passes
Bug: chromium:1123567
Change-Id: I2718548c45b61b34d78c17966037e8ccb91d654f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2389020Reviewed-by: Piotr Pawliczek <pawliczek@chromium.org>
Reviewed-by: Sean Kau <skau@chromium.org>
Commit-Queue: Luum Habtemariam <luum@chromium.org>
Cr-Commit-Position: refs/heads/master@{#805835}

chrome/browser/chromeos/printing/ppd_provider_factory.cc

@@ -11,17 +11,26 @@
 #include "chromeos/printing/ppd_cache.h"
 #include "chromeos/printing/ppd_provider.h"
 #include "components/version_info/version_info.h"
+#include "content/public/browser/browser_thread.h"
 #include "google_apis/google_api_keys.h"
 
 namespace chromeos {
+namespace {
+
+network::mojom::URLLoaderFactory* GetURLLoaderFactory() {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  return g_browser_process->system_network_context_manager()
+      ->GetURLLoaderFactory();
+}
+
+}  // namespace
 
 scoped_refptr<PpdProvider> CreatePpdProvider(Profile* profile) {
   base::FilePath ppd_cache_path =
       profile->GetPath().Append(FILE_PATH_LITERAL("PPDCache"));
 
   return PpdProvider::Create(g_browser_process->GetApplicationLocale(),
-                             g_browser_process->system_network_context_manager()
-                                 ->GetURLLoaderFactory(),
+                             base::BindRepeating(&GetURLLoaderFactory),
                              PpdCache::Create(ppd_cache_path),
                              version_info::GetVersion());
 }

chromeos/printing/ppd_provider.cc

@@ -367,12 +367,12 @@ class PpdProviderImpl : public PpdProvider {
   };
 
   PpdProviderImpl(const std::string& browser_locale,
-                  network::mojom::URLLoaderFactory* loader_factory,
+                  LoaderFactoryGetter loader_factory_getter,
                   scoped_refptr<PpdCache> ppd_cache,
                   const base::Version& current_version,
                   const PpdProvider::Options& options)
       : browser_locale_(browser_locale),
-        loader_factory_(loader_factory),
+        loader_factory_getter_(loader_factory_getter),
         ppd_cache_(ppd_cache),
         disk_task_runner_(base::ThreadPool::CreateSequencedTaskRunner(
             {base::TaskPriority::USER_VISIBLE, base::MayBlock(),
@@ -786,7 +786,7 @@ class PpdProviderImpl : public PpdProvider {
 
       // TODO(luum): consider using unbounded size
       fetcher_->DownloadToString(
-          loader_factory_,
+          loader_factory_getter_.Run(),
           base::BindOnce(&PpdProviderImpl::OnURLFetchComplete, this),
           network::SimpleURLLoader::kMaxBoundedStringDownloadSize);
 
@@ -1734,7 +1734,7 @@ class PpdProviderImpl : public PpdProvider {
   // BrowserContext::GetApplicationLocale();
   const std::string browser_locale_;
 
-  network::mojom::URLLoaderFactory* loader_factory_;
+  LoaderFactoryGetter loader_factory_getter_;
 
   // For file:// fetches, a staging buffer and result flag for loading the file.
   std::string file_fetch_contents_;
@@ -1780,11 +1780,12 @@ PrinterSearchData::~PrinterSearchData() = default;
 // static
 scoped_refptr<PpdProvider> PpdProvider::Create(
     const std::string& browser_locale,
-    network::mojom::URLLoaderFactory* loader_factory,
+    LoaderFactoryGetter loader_factory_getter,
     scoped_refptr<PpdCache> ppd_cache,
     const base::Version& current_version,
     const PpdProvider::Options& options) {
-  return scoped_refptr<PpdProvider>(new PpdProviderImpl(
-      browser_locale, loader_factory, ppd_cache, current_version, options));
+  return scoped_refptr<PpdProvider>(
+      new PpdProviderImpl(browser_locale, loader_factory_getter, ppd_cache,
+                          current_version, options));
 }
 }  // namespace chromeos

chromeos/printing/ppd_provider.h

@@ -176,11 +176,16 @@ class CHROMEOS_EXPORT PpdProvider : public base::RefCounted<PpdProvider> {
                               const std::string& manufacturer,
                               const std::string& model)>;
 
+  // Called to get the current URLLoaderFactory on demand. Needs to be
+  // Repeating since it gets called once per fetch.
+  using LoaderFactoryGetter =
+      base::RepeatingCallback<network::mojom::URLLoaderFactory*()>;
+
   // Create and return a new PpdProvider with the given cache and options.
   // A references to |url_context_getter| is taken.
   static scoped_refptr<PpdProvider> Create(
       const std::string& browser_locale,
-      network::mojom::URLLoaderFactory* loader_factory,
+      LoaderFactoryGetter loader_factory_getter,
       scoped_refptr<PpdCache> cache,
       const base::Version& current_version,
       const Options& options = Options());

chromeos/printing/ppd_provider_unittest.cc

@@ -16,6 +16,7 @@
 #include "base/run_loop.h"
 #include "base/single_thread_task_runner.h"
 #include "base/strings/stringprintf.h"
+#include "base/test/bind_test_util.h"
 #include "base/test/task_environment.h"
 #include "base/test/test_message_loop.h"
 #include "base/threading/sequenced_task_runner_handle.h"
@@ -94,8 +95,15 @@ class PpdProviderTest : public ::testing::Test {
     } else {
       ppd_cache_ = PpdCache::Create(ppd_cache_temp_dir_.GetPath());
     }
-    return PpdProvider::Create(locale, &loader_factory_, ppd_cache_,
-                               base::Version("40.8.6753.09"), provider_options);
+    return PpdProvider::Create(
+        locale, base::BindLambdaForTesting([this]() {
+          // Casting here is necessary b/c RepeatingCallback needs help
+          // coercing types.
+          // Casting is safe since its a guaranteed upcast.
+          return dynamic_cast<network::mojom::URLLoaderFactory*>(
+              &loader_factory_);
+        }),
+        ppd_cache_, base::Version("40.8.6753.09"), provider_options);
   }
 
   // Fill loader_factory_ with preset contents for test URLs