Lazily create storage partition in FeedbackUploader

Don't create the storage partition until it's actually needed in
DispatchReport().

Bug: 1011889
Change-Id: I9da607cceb7504642731e8afdc987bd0f90aa497
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1845807
Commit-Queue: Evan Stade <estade@chromium.org>
Reviewed-by: Ahmed Fakhry <afakhry@chromium.org>
Cr-Commit-Position: refs/heads/master@{#704020}

chrome/browser/feedback/feedback_uploader_chrome.cc

@@ -14,8 +14,6 @@
 #include "components/signin/public/identity_manager/primary_account_access_token_fetcher.h"
 #include "content/public/browser/browser_task_traits.h"
 #include "content/public/browser/browser_thread.h"
-#include "net/url_request/url_fetcher.h"
-#include "services/network/public/cpp/shared_url_loader_factory.h"
 
 namespace feedback {
 
@@ -34,10 +32,9 @@ void QueueSingleReport(base::WeakPtr<feedback::FeedbackUploader> uploader,
 }  // namespace
 
 FeedbackUploaderChrome::FeedbackUploaderChrome(
-    scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
     content::BrowserContext* context,
     scoped_refptr<base::SingleThreadTaskRunner> task_runner)
-    : FeedbackUploader(url_loader_factory, context, task_runner) {
+    : FeedbackUploader(context, task_runner) {
   DCHECK(!context->IsOffTheRecord());
 
   task_runner->PostTask(

chrome/browser/feedback/feedback_uploader_chrome.h

@@ -23,7 +23,6 @@ namespace feedback {
 class FeedbackUploaderChrome : public FeedbackUploader {
  public:
   FeedbackUploaderChrome(
-      scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
       content::BrowserContext* context,
       scoped_refptr<base::SingleThreadTaskRunner> task_runner);
   ~FeedbackUploaderChrome() override;

chrome/browser/feedback/feedback_uploader_factory_chrome.cc

@@ -8,7 +8,6 @@
 #include "chrome/browser/feedback/feedback_uploader_chrome.h"
 #include "chrome/browser/profiles/profile.h"
 #include "content/public/browser/browser_context.h"
-#include "content/public/browser/storage_partition.h"
 
 namespace feedback {
 
@@ -46,10 +45,7 @@ bool FeedbackUploaderFactoryChrome::ServiceIsNULLWhileTesting() const {
 
 KeyedService* FeedbackUploaderFactoryChrome::BuildServiceInstanceFor(
     content::BrowserContext* context) const {
-  return new FeedbackUploaderChrome(
-      content::BrowserContext::GetDefaultStoragePartition(context)
-          ->GetURLLoaderFactoryForBrowserProcess(),
-      context, task_runner_);
+  return new FeedbackUploaderChrome(context, task_runner_);
 }
 
 }  // namespace feedback

components/feedback/feedback_data_unittest.cc

@@ -34,10 +34,11 @@ class MockUploader : public FeedbackUploader {
       scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
       content::BrowserContext* context,
       base::OnceClosure on_report_sent)
-      : FeedbackUploader(url_loader_factory,
-                         context,
+      : FeedbackUploader(context,
                          FeedbackUploaderFactory::CreateUploaderTaskRunner()),
-        on_report_sent_(std::move(on_report_sent)) {}
+        on_report_sent_(std::move(on_report_sent)) {
+    set_url_loader_factory_for_test(url_loader_factory);
+  }
   ~MockUploader() override {}
 
   // feedback::FeedbackUploader:

components/feedback/feedback_uploader.cc

@@ -15,7 +15,6 @@
 #include "net/base/load_flags.h"
 #include "net/url_request/url_fetcher.h"
 #include "services/network/public/cpp/resource_request.h"
-#include "services/network/public/cpp/shared_url_loader_factory.h"
 #include "services/network/public/cpp/simple_url_loader.h"
 
 namespace feedback {
@@ -60,11 +59,9 @@ GURL GetFeedbackPostGURL() {
 }  // namespace
 
 FeedbackUploader::FeedbackUploader(
-    scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
     content::BrowserContext* context,
     scoped_refptr<base::SingleThreadTaskRunner> task_runner)
-    : url_loader_factory_(std::move(url_loader_factory)),
-      context_(context),
+    : context_(context),
       feedback_reports_path_(GetPathFromContext(context)),
       task_runner_(task_runner),
       feedback_post_url_(GetFeedbackPostGURL()),
@@ -189,6 +186,15 @@ void FeedbackUploader::DispatchReport() {
                                            kProtoBufMimeType);
   auto it = uploads_in_progress_.insert(uploads_in_progress_.begin(),
                                         std::move(simple_url_loader));
+
+  // Creating the StoragePartitionImpl is costly, so don't do it until
+  // necessary (most importantly, avoid doing so during startup).
+  if (!url_loader_factory_) {
+    url_loader_factory_ =
+        content::BrowserContext::GetDefaultStoragePartition(context_)
+            ->GetURLLoaderFactoryForBrowserProcess();
+  }
+
   simple_url_loader_ptr->DownloadToStringOfUnboundedSizeUntilCrashAndDie(
       url_loader_factory_.get(),
       base::BindOnce(&FeedbackUploader::OnDispatchComplete,

components/feedback/feedback_uploader.h

@@ -17,6 +17,7 @@
 #include "base/time/time.h"
 #include "base/timer/timer.h"
 #include "components/keyed_service/core/keyed_service.h"
+#include "services/network/public/cpp/shared_url_loader_factory.h"
 #include "url/gurl.h"
 
 namespace content {
@@ -26,7 +27,6 @@ class BrowserContext;
 namespace network {
 struct ResourceRequest;
 class SimpleURLLoader;
-class SharedURLLoaderFactory;
 }  // namespace network
 
 namespace feedback {
@@ -39,10 +39,8 @@ class FeedbackReport;
 class FeedbackUploader : public KeyedService,
                          public base::SupportsWeakPtr<FeedbackUploader> {
  public:
-  FeedbackUploader(
-      scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
-      content::BrowserContext* context,
-      scoped_refptr<base::SingleThreadTaskRunner> task_runner);
+  FeedbackUploader(content::BrowserContext* context,
+                   scoped_refptr<base::SingleThreadTaskRunner> task_runner);
   ~FeedbackUploader() override;
 
   static void SetMinimumRetryDelayForTesting(base::TimeDelta delay);
@@ -67,6 +65,12 @@ class FeedbackUploader : public KeyedService,
 
   base::TimeDelta retry_delay() const { return retry_delay_; }
 
+  // Tests inject a TestURLLoaderFactory so they can mock the network response.
+  void set_url_loader_factory_for_test(
+      scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory) {
+    url_loader_factory_ = url_loader_factory;
+  }
+
  protected:
   // Virtual to give implementers a chance to do work before the report is
   // disptached. Implementers can then call

components/feedback/feedback_uploader_dispatch_unittest.cc

@@ -92,8 +92,8 @@ TEST_F(FeedbackUploaderDispatchTest, VariationHeaders) {
   CreateFieldTrialWithId("Test", "Group1", 123);
 
   FeedbackUploader uploader(
-      shared_url_loader_factory(), context(),
-      FeedbackUploaderFactory::CreateUploaderTaskRunner());
+      context(), FeedbackUploaderFactory::CreateUploaderTaskRunner());
+  uploader.set_url_loader_factory_for_test(shared_url_loader_factory());
 
   net::HttpRequestHeaders headers;
   test_url_loader_factory()->SetInterceptor(
@@ -110,8 +110,8 @@ TEST_F(FeedbackUploaderDispatchTest, VariationHeaders) {
 TEST_F(FeedbackUploaderDispatchTest, 204Response) {
   FeedbackUploader::SetMinimumRetryDelayForTesting(kTestRetryDelay);
   FeedbackUploader uploader(
-      shared_url_loader_factory(), context(),
-      FeedbackUploaderFactory::CreateUploaderTaskRunner());
+      context(), FeedbackUploaderFactory::CreateUploaderTaskRunner());
+  uploader.set_url_loader_factory_for_test(shared_url_loader_factory());
 
   EXPECT_EQ(kTestRetryDelay, uploader.retry_delay());
   // Successful reports should not introduce any retries, and should not
@@ -133,8 +133,8 @@ TEST_F(FeedbackUploaderDispatchTest, 204Response) {
 TEST_F(FeedbackUploaderDispatchTest, 400Response) {
   FeedbackUploader::SetMinimumRetryDelayForTesting(kTestRetryDelay);
   FeedbackUploader uploader(
-      shared_url_loader_factory(), context(),
-      FeedbackUploaderFactory::CreateUploaderTaskRunner());
+      context(), FeedbackUploaderFactory::CreateUploaderTaskRunner());
+  uploader.set_url_loader_factory_for_test(shared_url_loader_factory());
 
   EXPECT_EQ(kTestRetryDelay, uploader.retry_delay());
   // Failed reports due to client errors are not retried. No backoff delay
@@ -156,8 +156,8 @@ TEST_F(FeedbackUploaderDispatchTest, 400Response) {
 TEST_F(FeedbackUploaderDispatchTest, 500Response) {
   FeedbackUploader::SetMinimumRetryDelayForTesting(kTestRetryDelay);
   FeedbackUploader uploader(
-      shared_url_loader_factory(), context(),
-      FeedbackUploaderFactory::CreateUploaderTaskRunner());
+      context(), FeedbackUploaderFactory::CreateUploaderTaskRunner());
+  uploader.set_url_loader_factory_for_test(shared_url_loader_factory());
 
   EXPECT_EQ(kTestRetryDelay, uploader.retry_delay());
   // Failed reports due to server errors are retried.

components/feedback/feedback_uploader_factory.cc

@@ -11,7 +11,6 @@
 #include "components/feedback/feedback_uploader.h"
 #include "components/keyed_service/content/browser_context_dependency_manager.h"
 #include "content/public/browser/browser_context.h"
-#include "content/public/browser/storage_partition.h"
 
 namespace feedback {
 
@@ -53,10 +52,7 @@ FeedbackUploaderFactory::~FeedbackUploaderFactory() {}
 
 KeyedService* FeedbackUploaderFactory::BuildServiceInstanceFor(
     content::BrowserContext* context) const {
-  return new FeedbackUploader(
-      content::BrowserContext::GetDefaultStoragePartition(context)
-          ->GetURLLoaderFactoryForBrowserProcess(),
-      context, task_runner_);
+  return new FeedbackUploader(context, task_runner_);
 }
 
 content::BrowserContext* FeedbackUploaderFactory::GetBrowserContextToUse(

components/feedback/feedback_uploader_unittest.cc

@@ -41,9 +41,10 @@ class MockFeedbackUploader : public FeedbackUploader {
   MockFeedbackUploader(
       scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
       content::BrowserContext* context)
-      : FeedbackUploader(url_loader_factory,
-                         context,
-                         FeedbackUploaderFactory::CreateUploaderTaskRunner()) {}
+      : FeedbackUploader(context,
+                         FeedbackUploaderFactory::CreateUploaderTaskRunner()) {
+    set_url_loader_factory_for_test(url_loader_factory);
+  }
   ~MockFeedbackUploader() override {}
 
   void RunMessageLoop() {