Use per-profile network context in DownloadFeedbackService

This CL changes the DownloadFeedbackService to know about the profile it
is performing feedback for, and to use that profile to get the appropriate
URLLoaderFactory.

Bug: 1049833
Change-Id: I63b958cbfbcec2129315c7a680ca25eb1221d8af
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2347224
Commit-Queue: Daniel Rubery <drubery@chromium.org>
Reviewed-by: Varun Khaneja <vakh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#798388}

chrome/browser/safe_browsing/download_protection/download_feedback_service.cc

@@ -70,9 +70,9 @@ DownloadFeedbackPings* DownloadFeedbackPings::FromDownload(
 }  // namespace
 
 DownloadFeedbackService::DownloadFeedbackService(
-    scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
+    DownloadProtectionService* download_protection_service,
     base::TaskRunner* file_task_runner)
-    : url_loader_factory_(url_loader_factory),
+    : download_protection_service_(download_protection_service),
       file_task_runner_(file_task_runner) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 }
@@ -124,6 +124,7 @@ bool DownloadFeedbackService::GetPingsForDownloadForTesting(
 }
 
 void DownloadFeedbackService::BeginFeedbackForDownload(
+    Profile* profile,
     download::DownloadItem* download,
     DownloadCommands::Command download_command) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
@@ -134,7 +135,7 @@ void DownloadFeedbackService::BeginFeedbackForDownload(
   download->StealDangerousDownload(
       download_command == DownloadCommands::DISCARD,
       base::BindOnce(&DownloadFeedbackService::BeginFeedbackOrDeleteFile,
-                     file_task_runner_, weak_ptr_factory_.GetWeakPtr(),
+                     file_task_runner_, weak_ptr_factory_.GetWeakPtr(), profile,
                      pings->ping_request(), pings->ping_response()));
   if (download_command == DownloadCommands::KEEP) {
     DownloadItemModel model(download);
@@ -146,13 +147,14 @@ void DownloadFeedbackService::BeginFeedbackForDownload(
 void DownloadFeedbackService::BeginFeedbackOrDeleteFile(
     const scoped_refptr<base::TaskRunner>& file_task_runner,
     const base::WeakPtr<DownloadFeedbackService>& service,
+    Profile* profile,
     const std::string& ping_request,
     const std::string& ping_response,
     const base::FilePath& path) {
   if (service) {
     if (path.empty())
       return;
-    service->BeginFeedback(ping_request, ping_response, path);
+    service->BeginFeedback(profile, ping_request, ping_response, path);
   } else {
     file_task_runner->PostTask(
         FROM_HERE, base::BindOnce(base::GetDeleteFileCallback(), path));
@@ -165,13 +167,14 @@ void DownloadFeedbackService::StartPendingFeedback() {
       &DownloadFeedbackService::FeedbackComplete, base::Unretained(this)));
 }
 
-void DownloadFeedbackService::BeginFeedback(const std::string& ping_request,
+void DownloadFeedbackService::BeginFeedback(Profile* profile,
+                                            const std::string& ping_request,
                                             const std::string& ping_response,
                                             const base::FilePath& path) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-  std::unique_ptr<DownloadFeedback> feedback(
-      DownloadFeedback::Create(url_loader_factory_, file_task_runner_.get(),
-                               path, ping_request, ping_response));
+  std::unique_ptr<DownloadFeedback> feedback(DownloadFeedback::Create(
+      download_protection_service_->GetURLLoaderFactory(profile),
+      file_task_runner_.get(), path, ping_request, ping_response));
   active_feedback_.push(std::move(feedback));
 
   if (active_feedback_.size() == 1)

chrome/browser/safe_browsing/download_protection/download_feedback_service.h

@@ -24,10 +24,6 @@ namespace download {
 class DownloadItem;
 }
 
-namespace network {
-class SharedURLLoaderFactory;
-}
-
 namespace safe_browsing {
 
 class DownloadFeedback;
@@ -38,7 +34,7 @@ class DownloadFeedback;
 class DownloadFeedbackService {
  public:
   DownloadFeedbackService(
-      scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
+      DownloadProtectionService* download_protection_service,
       base::TaskRunner* file_task_runner);
   ~DownloadFeedbackService();
 
@@ -64,26 +60,31 @@ class DownloadFeedbackService {
       std::string* ping,
       std::string* response);
 
-  // Begin download feedback for |download|. Then delete download file if
-  // |download_command| is DISCARD, or run the KEEP command otherwise.This must
-  // only be called if IsEnabledForDownload is true for |download|.
-  void BeginFeedbackForDownload(download::DownloadItem* download,
+  // Begin download feedback for the given |download| in the given |profile|.
+  // Then delete download file if |download_command| is DISCARD, or run the KEEP
+  // command otherwise. This must only be called if IsEnabledForDownload is true
+  // for |download|.
+  void BeginFeedbackForDownload(Profile* profile,
+                                download::DownloadItem* download,
                                 DownloadCommands::Command download_command);
 
  private:
   static void BeginFeedbackOrDeleteFile(
       const scoped_refptr<base::TaskRunner>& file_task_runner,
       const base::WeakPtr<DownloadFeedbackService>& service,
+      Profile* profile,
       const std::string& ping_request,
       const std::string& ping_response,
       const base::FilePath& path);
   void StartPendingFeedback();
-  void BeginFeedback(const std::string& ping_request,
+  void BeginFeedback(Profile* profile,
+                     const std::string& ping_request,
                      const std::string& ping_response,
                      const base::FilePath& path);
   void FeedbackComplete();
 
-  scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory_;
+  // Safe because the DownloadProtectionService owns this.
+  DownloadProtectionService* download_protection_service_;
   scoped_refptr<base::TaskRunner> file_task_runner_;
 
   // Currently active & pending uploads. The first item is active, remaining

chrome/browser/safe_browsing/download_protection/download_feedback_service_unittest.cc

@@ -20,6 +20,7 @@
 #include "base/task/post_task.h"
 #include "base/task/thread_pool.h"
 #include "chrome/browser/safe_browsing/download_protection/download_feedback.h"
+#include "chrome/test/base/testing_profile.h"
 #include "components/download/public/common/mock_download_item.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/test/browser_task_environment.h"
@@ -105,6 +106,16 @@ class FakeDownloadFeedbackFactory : public DownloadFeedbackFactory {
   std::vector<FakeDownloadFeedback*> feedbacks_;
 };
 
+class FakeDownloadProtectionService : public DownloadProtectionService {
+ public:
+  FakeDownloadProtectionService() : DownloadProtectionService(nullptr) {}
+
+  scoped_refptr<network::SharedURLLoaderFactory> GetURLLoaderFactory(
+      content::BrowserContext* browser_context) override {
+    return nullptr;
+  }
+};
+
 bool WillStorePings(DownloadCheckResult result,
                     bool upload_requested,
                     int64_t size) {
@@ -155,6 +166,8 @@ class DownloadFeedbackServiceTest : public testing::Test {
   content::BrowserTaskEnvironment task_environment_;
   scoped_refptr<base::SequencedTaskRunner> file_task_runner_;
   FakeDownloadFeedbackFactory download_feedback_factory_;
+  TestingProfile profile_;
+  FakeDownloadProtectionService fake_download_service_;
 };
 
 TEST_F(DownloadFeedbackServiceTest, MaybeStorePingsForDownload) {
@@ -216,12 +229,13 @@ TEST_F(DownloadFeedbackServiceTest, SingleFeedbackCompleteAndDiscardDownload) {
         download_discarded_callback = std::move(arg);
       });
 
-  DownloadFeedbackService service(nullptr, file_task_runner_.get());
+  DownloadFeedbackService service(&fake_download_service_,
+                                  file_task_runner_.get());
   service.MaybeStorePingsForDownload(DownloadCheckResult::UNCOMMON,
                                      true /* upload_requested */, &item,
                                      ping_request, ping_response);
   ASSERT_TRUE(DownloadFeedbackService::IsEnabledForDownload(item));
-  service.BeginFeedbackForDownload(&item, DownloadCommands::DISCARD);
+  service.BeginFeedbackForDownload(&profile_, &item, DownloadCommands::DISCARD);
   ASSERT_FALSE(download_discarded_callback.is_null());
   EXPECT_EQ(0U, num_feedbacks());
 
@@ -261,12 +275,13 @@ TEST_F(DownloadFeedbackServiceTest, SingleFeedbackCompleteAndKeepDownload) {
   GURL empty_url;
   EXPECT_CALL(item, GetURL()).WillOnce(ReturnRef(empty_url));
 
-  DownloadFeedbackService service(nullptr, file_task_runner_.get());
+  DownloadFeedbackService service(&fake_download_service_,
+                                  file_task_runner_.get());
   service.MaybeStorePingsForDownload(DownloadCheckResult::UNCOMMON,
                                      true /* upload_requested */, &item,
                                      ping_request, ping_response);
   ASSERT_TRUE(DownloadFeedbackService::IsEnabledForDownload(item));
-  service.BeginFeedbackForDownload(&item, DownloadCommands::KEEP);
+  service.BeginFeedbackForDownload(&profile_, &item, DownloadCommands::KEEP);
   ASSERT_FALSE(download_discarded_callback.is_null());
   EXPECT_EQ(0U, num_feedbacks());
 
@@ -312,10 +327,12 @@ TEST_F(DownloadFeedbackServiceTest, MultiplePendingFeedbackComplete) {
   }
 
   {
-    DownloadFeedbackService service(nullptr, file_task_runner_.get());
+    DownloadFeedbackService service(&fake_download_service_,
+                                    file_task_runner_.get());
     for (size_t i = 0; i < kNumDownloads; ++i) {
       SCOPED_TRACE(i);
-      service.BeginFeedbackForDownload(&item[i], DownloadCommands::DISCARD);
+      service.BeginFeedbackForDownload(&profile_, &item[i],
+                                       DownloadCommands::DISCARD);
       ASSERT_FALSE(download_discarded_callback[i].is_null());
     }
     EXPECT_EQ(0U, num_feedbacks());
@@ -384,10 +401,12 @@ TEST_F(DownloadFeedbackServiceTest, MultiFeedbackWithIncomplete) {
   }
 
   {
-    DownloadFeedbackService service(nullptr, file_task_runner_.get());
+    DownloadFeedbackService service(&fake_download_service_,
+                                    file_task_runner_.get());
     for (size_t i = 0; i < kNumDownloads; ++i) {
       SCOPED_TRACE(i);
-      service.BeginFeedbackForDownload(&item[i], DownloadCommands::DISCARD);
+      service.BeginFeedbackForDownload(&profile_, &item[i],
+                                       DownloadCommands::DISCARD);
       ASSERT_FALSE(download_discarded_callback[i].is_null());
     }
     EXPECT_EQ(0U, num_feedbacks());

chrome/browser/safe_browsing/download_protection/download_protection_service.cc

@@ -109,7 +109,7 @@ DownloadProtectionService::DownloadProtectionService(
       binary_feature_extractor_(new BinaryFeatureExtractor()),
       download_request_timeout_ms_(kDownloadRequestTimeoutMs),
       feedback_service_(new DownloadFeedbackService(
-          sb_service_ ? sb_service_->GetURLLoaderFactory() : nullptr,
+          this,
           base::ThreadPool::CreateSequencedTaskRunner(
               {base::MayBlock(), base::TaskPriority::BEST_EFFORT})
               .get())),
@@ -569,7 +569,8 @@ bool DownloadProtectionService::MaybeBeginFeedbackForDownload(
   bool is_extended_reporting =
       ExtendedReportingPrefExists(*prefs) && IsExtendedReportingEnabled(*prefs);
   if (!profile->IsOffTheRecord() && is_extended_reporting) {
-    feedback_service_->BeginFeedbackForDownload(download, download_command);
+    feedback_service_->BeginFeedbackForDownload(profile, download,
+                                                download_command);
     return true;
   }
   return false;

chrome/browser/safe_browsing/download_protection/download_protection_service.h

@@ -203,7 +203,7 @@ class DownloadProtectionService {
       DeepScanningRequest::DeepScanTrigger trigger,
       enterprise_connectors::AnalysisSettings analysis_settings);
 
-  scoped_refptr<network::SharedURLLoaderFactory> GetURLLoaderFactory(
+  virtual scoped_refptr<network::SharedURLLoaderFactory> GetURLLoaderFactory(
       content::BrowserContext* browser_context);
 
  private: