Convert Incident Reporting to use SimpleURLLoader instead of URLFetcher.

Bug: 825242
Change-Id: Idf2df12e459f26b91aafbb7cb434e79f26850ae8
Reviewed-on: https://chromium-review.googlesource.com/989118
Commit-Queue: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Greg Thompson <grt@chromium.org>
Cr-Commit-Position: refs/heads/master@{#547720}

chrome/browser/safe_browsing/incident_reporting/DEPS

+specific_include_rules = {
+  ".*test\.cc": [
+    "+services/network/test",
+  ]
+}

chrome/browser/safe_browsing/incident_reporting/incident_report_uploader_impl.cc

@@ -14,7 +14,8 @@
 #include "net/base/load_flags.h"
 #include "net/http/http_status_code.h"
 #include "net/traffic_annotation/network_traffic_annotation.h"
-#include "net/url_request/url_fetcher.h"
+#include "services/network/public/cpp/shared_url_loader_factory.h"
+#include "services/network/public/cpp/simple_url_loader.h"
 
 namespace safe_browsing {
 
@@ -68,35 +69,33 @@ IncidentReportUploaderImpl::~IncidentReportUploaderImpl() {
 std::unique_ptr<IncidentReportUploader>
 IncidentReportUploaderImpl::UploadReport(
     const OnResultCallback& callback,
-    const scoped_refptr<net::URLRequestContextGetter>& request_context_getter,
+    const scoped_refptr<network::SharedURLLoaderFactory>& url_loader_factory,
     const ClientIncidentReport& report) {
   std::string post_data;
   if (!report.SerializeToString(&post_data))
     return std::unique_ptr<IncidentReportUploader>();
-  return std::unique_ptr<IncidentReportUploader>(new IncidentReportUploaderImpl(
-      callback, request_context_getter, post_data));
+  return std::unique_ptr<IncidentReportUploader>(
+      new IncidentReportUploaderImpl(callback, url_loader_factory, post_data));
 }
 
 IncidentReportUploaderImpl::IncidentReportUploaderImpl(
     const OnResultCallback& callback,
-    const scoped_refptr<net::URLRequestContextGetter>& request_context_getter,
+    const scoped_refptr<network::SharedURLLoaderFactory>& url_loader_factory,
     const std::string& post_data)
-    : IncidentReportUploader(callback),
-      url_fetcher_(
-          net::URLFetcher::Create(kTestUrlFetcherId,
-                                  GetIncidentReportUrl(),
-                                  net::URLFetcher::POST,
-                                  this,
-                                  kSafeBrowsingIncidentTrafficAnnotation)),
-      time_begin_(base::TimeTicks::Now()) {
-  data_use_measurement::DataUseUserData::AttachToFetcher(
-      url_fetcher_.get(), data_use_measurement::DataUseUserData::SAFE_BROWSING);
+    : IncidentReportUploader(callback) {
+  auto resource_request = std::make_unique<network::ResourceRequest>();
+  resource_request->url = GetIncidentReportUrl();
+  resource_request->method = "POST";
+  resource_request->load_flags = net::LOAD_DISABLE_CACHE;
+  url_loader_ = network::SimpleURLLoader::Create(
+      std::move(resource_request), kSafeBrowsingIncidentTrafficAnnotation);
+  url_loader_->AttachStringForUpload(post_data, "application/octet-stream");
+  url_loader_->DownloadToStringOfUnboundedSizeUntilCrashAndDie(
+      url_loader_factory.get(),
+      base::BindOnce(&IncidentReportUploaderImpl::OnURLLoaderComplete,
+                     base::Unretained(this)));
+  time_begin_ = base::TimeTicks::Now();
   UMA_HISTOGRAM_COUNTS("SBIRS.ReportPayloadSize", post_data.size());
-  url_fetcher_->SetLoadFlags(net::LOAD_DISABLE_CACHE);
-  url_fetcher_->SetAutomaticallyRetryOn5xx(false);
-  url_fetcher_->SetRequestContext(request_context_getter.get());
-  url_fetcher_->SetUploadData("application/octet-stream", post_data);
-  url_fetcher_->Start();
 }
 
 // static
@@ -108,23 +107,34 @@ GURL IncidentReportUploaderImpl::GetIncidentReportUrl() {
   return url.Resolve("?key=" + net::EscapeQueryParamValue(api_key, true));
 }
 
-void IncidentReportUploaderImpl::OnURLFetchComplete(
-    const net::URLFetcher* source) {
-  // Take ownership of the fetcher in this scope (source == url_fetcher_).
-  std::unique_ptr<net::URLFetcher> url_fetcher(std::move(url_fetcher_));
+void IncidentReportUploaderImpl::OnURLLoaderComplete(
+    std::unique_ptr<std::string> response_body) {
+  // Take ownership of the loader in this scope.
+  std::unique_ptr<network::SimpleURLLoader> url_loader(std::move(url_loader_));
+  int response_code = 0;
+  if (url_loader->ResponseInfo() && url_loader->ResponseInfo()->headers)
+    response_code = url_loader->ResponseInfo()->headers->response_code();
 
+  std::string response_body_str;
+  if (response_body.get())
+    response_body_str = std::move(*response_body.get());
+
+  OnURLLoaderCompleteInternal(response_body_str, response_code,
+                              url_loader->NetError());
+}
+
+void IncidentReportUploaderImpl::OnURLLoaderCompleteInternal(
+    const std::string& response_body,
+    int response_code,
+    int net_error) {
   UMA_HISTOGRAM_TIMES("SBIRS.ReportUploadTime",
                       base::TimeTicks::Now() - time_begin_);
 
   Result result = UPLOAD_REQUEST_FAILED;
   std::unique_ptr<ClientIncidentResponse> response;
-
-  if (source->GetStatus().is_success() &&
-      source->GetResponseCode() == net::HTTP_OK) {
-    std::string data;
-    source->GetResponseAsString(&data);
+  if (net_error == net::OK && response_code == net::HTTP_OK) {
     response.reset(new ClientIncidentResponse());
-    if (!response->ParseFromString(data)) {
+    if (!response->ParseFromString(response_body)) {
       response.reset();
       result = UPLOAD_INVALID_RESPONSE;
     } else {

chrome/browser/safe_browsing/incident_reporting/incident_report_uploader_impl.h

@@ -9,16 +9,16 @@
 #include <string>
 
 #include "base/compiler_specific.h"
+#include "base/gtest_prod_util.h"
 #include "base/macros.h"
 #include "base/time/time.h"
 #include "chrome/browser/safe_browsing/incident_reporting/incident_report_uploader.h"
-#include "net/url_request/url_fetcher_delegate.h"
 #include "url/gurl.h"
 
-namespace net {
-class URLFetcher;
-class URLRequestContextGetter;
-}  // namespace net
+namespace network {
+class SharedURLLoaderFactory;
+class SimpleURLLoader;
+}  // namespace network
 
 namespace safe_browsing {
 
@@ -28,8 +28,7 @@ class ClientIncidentReport;
 // method. The upload can be cancelled by deleting the returned instance. The
 // instance is no longer usable after the delegate is notified, and may safely
 // be destroyed by the delegate.
-class IncidentReportUploaderImpl : public IncidentReportUploader,
-                                   public net::URLFetcherDelegate {
+class IncidentReportUploaderImpl : public IncidentReportUploader {
  public:
   // The id associated with the URLFetcher, for use by tests.
   static const int kTestUrlFetcherId;
@@ -41,24 +40,30 @@ class IncidentReportUploaderImpl : public IncidentReportUploader,
   // for transmission, in which case the delegate is not notified.
   static std::unique_ptr<IncidentReportUploader> UploadReport(
       const OnResultCallback& callback,
-      const scoped_refptr<net::URLRequestContextGetter>& request_context_getter,
+      const scoped_refptr<network::SharedURLLoaderFactory>& url_loader_factory,
       const ClientIncidentReport& report);
 
  private:
+  FRIEND_TEST_ALL_PREFIXES(IncidentReportUploaderImplTest, Success);
+
   IncidentReportUploaderImpl(
       const OnResultCallback& callback,
-      const scoped_refptr<net::URLRequestContextGetter>& request_context_getter,
+      const scoped_refptr<network::SharedURLLoaderFactory>& url_loader_factory,
       const std::string& post_data);
 
   // Returns the URL to which incident reports are to be sent.
   static GURL GetIncidentReportUrl();
 
-  // net::URLFetcherDelegate methods.
-  void OnURLFetchComplete(const net::URLFetcher* source) override;
+  // Callback when SimpleURLLoader gets the response.
+  void OnURLLoaderComplete(std::unique_ptr<std::string> response_body);
+
+  void OnURLLoaderCompleteInternal(const std::string& response_body,
+                                   int response_code,
+                                   int net_error);
 
-  // The underlying URL fetcher. The instance is alive from construction through
-  // OnURLFetchComplete.
-  std::unique_ptr<net::URLFetcher> url_fetcher_;
+  // The underlying URLLoader. The instance is alive from construction through
+  // OnURLLoaderComplete.
+  std::unique_ptr<network::SimpleURLLoader> url_loader_;
 
   // The time at which the upload was initiated.
   base::TimeTicks time_begin_;

chrome/browser/safe_browsing/incident_reporting/incident_report_uploader_impl_unittest.cc

@@ -7,21 +7,24 @@
 #include <string>
 #include <utility>
 
+#include "base/bind.h"
 #include "base/test/test_simple_task_runner.h"
+#include "base/threading/thread_task_runner_handle.h"
 #include "components/safe_browsing/proto/csd.pb.h"
+#include "content/public/common/weak_wrapper_shared_url_loader_factory.h"
 #include "net/base/load_flags.h"
 #include "net/http/http_status_code.h"
-#include "net/url_request/test_url_fetcher_factory.h"
-#include "net/url_request/url_request_context_getter.h"
-#include "net/url_request/url_request_status.h"
+#include "services/network/public/cpp/shared_url_loader_factory.h"
+#include "services/network/test/test_url_loader_factory.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+namespace safe_browsing {
+
 class IncidentReportUploaderImplTest : public testing::Test {
  public:
   // safe_browsing::IncidentReportUploader::OnResultCallback implementation.
-  void OnReportUploadResult(
-      safe_browsing::IncidentReportUploader::Result result,
-      std::unique_ptr<safe_browsing::ClientIncidentResponse> response) {
+  void OnReportUploadResult(IncidentReportUploader::Result result,
+                            std::unique_ptr<ClientIncidentResponse> response) {
     result_ = result;
     response_ = std::move(response);
   }
@@ -29,43 +32,39 @@ class IncidentReportUploaderImplTest : public testing::Test {
  protected:
   IncidentReportUploaderImplTest()
       : task_runner_(new base::TestSimpleTaskRunner),
-        result_(safe_browsing::IncidentReportUploader::UPLOAD_REQUEST_FAILED) {}
+        handle_(task_runner_),
+        result_(IncidentReportUploader::UPLOAD_REQUEST_FAILED) {}
 
   scoped_refptr<base::TestSimpleTaskRunner> task_runner_;
-  net::TestURLFetcherFactory url_fetcher_factory_;
-  safe_browsing::IncidentReportUploader::Result result_;
-  std::unique_ptr<safe_browsing::ClientIncidentResponse> response_;
+  base::ThreadTaskRunnerHandle handle_;
+  IncidentReportUploader::Result result_;
+  std::unique_ptr<ClientIncidentResponse> response_;
 };
 
 TEST_F(IncidentReportUploaderImplTest, Success) {
-  safe_browsing::ClientIncidentReport report;
-  std::unique_ptr<safe_browsing::IncidentReportUploader> instance(
-      safe_browsing::IncidentReportUploaderImpl::UploadReport(
-          base::Bind(&IncidentReportUploaderImplTest::OnReportUploadResult,
-                     base::Unretained(this)),
-          NULL, report));
-
-  net::TestURLFetcher* fetcher = url_fetcher_factory_.GetFetcherByID(
-      safe_browsing::IncidentReportUploaderImpl::kTestUrlFetcherId);
-  ASSERT_NE(static_cast<net::TestURLFetcher*>(NULL), fetcher);
+  network::TestURLLoaderFactory test_url_loader_factory;
+  auto url_loader_factory =
+      base::MakeRefCounted<content::WeakWrapperSharedURLLoaderFactory>(
+          &test_url_loader_factory);
 
-  safe_browsing::ClientIncidentReport uploaded_report;
+  ClientIncidentReport report;
+  auto instance(IncidentReportUploaderImpl::UploadReport(
+      base::Bind(&IncidentReportUploaderImplTest::OnReportUploadResult,
+                 base::Unretained(this)),
+      url_loader_factory, report));
 
-  EXPECT_EQ(net::LOAD_DISABLE_CACHE, fetcher->GetLoadFlags());
-  EXPECT_TRUE(uploaded_report.ParseFromString(fetcher->upload_data()));
-
-  fetcher->set_status(net::URLRequestStatus(net::URLRequestStatus::SUCCESS, 0));
-  fetcher->set_response_code(net::HTTP_OK);
   std::string response;
-  safe_browsing::ClientIncidentResponse().SerializeToString(&response);
-  fetcher->SetResponseString(response);
+  ClientIncidentResponse().SerializeToString(&response);
 
-  fetcher->delegate()->OnURLFetchComplete(fetcher);
+  static_cast<IncidentReportUploaderImpl*>(instance.get())
+      ->OnURLLoaderCompleteInternal(response, net::HTTP_OK, net::OK);
 
-  EXPECT_EQ(safe_browsing::IncidentReportUploader::UPLOAD_SUCCESS, result_);
+  EXPECT_EQ(IncidentReportUploader::UPLOAD_SUCCESS, result_);
   EXPECT_TRUE(response_);
 }
 
+}  // namespace safe_browsing
+
 // TODO(grt):
 // bad status/response code
 // confirm data in request is in upload test

chrome/browser/safe_browsing/incident_reporting/incident_reporting_service.cc

@@ -41,7 +41,7 @@
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/download_item_utils.h"
 #include "content/public/browser/notification_service.h"
-#include "net/url_request/url_request_context_getter.h"
+#include "services/network/public/cpp/shared_url_loader_factory.h"
 #include "services/preferences/public/mojom/tracked_preference_validation_delegate.mojom.h"
 
 namespace safe_browsing {
@@ -319,9 +319,9 @@ bool IncidentReportingService::IsEnabledForProfile(Profile* profile) {
 
 IncidentReportingService::IncidentReportingService(
     SafeBrowsingService* safe_browsing_service)
-    : url_request_context_getter_(
-          safe_browsing_service ? safe_browsing_service->url_request_context()
-                                : nullptr),
+    : url_loader_factory_(safe_browsing_service
+                              ? safe_browsing_service->GetURLLoaderFactory()
+                              : nullptr),
       collect_environment_data_fn_(&CollectEnvironmentData),
       environment_collection_task_runner_(GetBackgroundTaskRunner()),
       environment_collection_pending_(),
@@ -404,11 +404,9 @@ void IncidentReportingService::AddDownloadManager(
 
 IncidentReportingService::IncidentReportingService(
     SafeBrowsingService* safe_browsing_service,
-    const scoped_refptr<net::URLRequestContextGetter>& request_context_getter,
     base::TimeDelta delayed_task_interval,
     const scoped_refptr<base::TaskRunner>& delayed_task_runner)
-    : url_request_context_getter_(request_context_getter),
-      collect_environment_data_fn_(&CollectEnvironmentData),
+    : collect_environment_data_fn_(&CollectEnvironmentData),
       environment_collection_task_runner_(GetBackgroundTaskRunner()),
       environment_collection_pending_(),
       collation_timeout_pending_(),
@@ -509,10 +507,9 @@ IncidentReportingService::CreateDownloadFinder(
 std::unique_ptr<IncidentReportUploader>
 IncidentReportingService::StartReportUpload(
     const IncidentReportUploader::OnResultCallback& callback,
-    const scoped_refptr<net::URLRequestContextGetter>& request_context_getter,
     const ClientIncidentReport& report) {
-  return IncidentReportUploaderImpl::UploadReport(
-      callback, request_context_getter, report);
+  return IncidentReportUploaderImpl::UploadReport(callback, url_loader_factory_,
+                                                  report);
 }
 
 bool IncidentReportingService::IsProcessingReport() const {
@@ -965,7 +962,7 @@ void IncidentReportingService::UploadReportIfUploadingEnabled(
   context->uploader = StartReportUpload(
       base::Bind(&IncidentReportingService::OnReportUploadResult,
                  weak_ptr_factory_.GetWeakPtr(), context),
-      url_request_context_getter_, *context->report);
+      *context->report);
   if (!context->uploader) {
     OnReportUploadResult(context,
                          IncidentReportUploader::UPLOAD_INVALID_REQUEST,

chrome/browser/safe_browsing/incident_reporting/incident_reporting_service.h

@@ -40,8 +40,8 @@ class NotificationDetails;
 class NotificationSource;
 }
 
-namespace net {
-class URLRequestContextGetter;
+namespace network {
+class SharedURLLoaderFactory;
 }
 
 namespace prefs {
@@ -116,7 +116,6 @@ class IncidentReportingService : public content::NotificationObserver {
   // be specified.
   IncidentReportingService(
       SafeBrowsingService* safe_browsing_service,
-      const scoped_refptr<net::URLRequestContextGetter>& request_context_getter,
       base::TimeDelta delayed_task_interval,
       const scoped_refptr<base::TaskRunner>& delayed_task_runner);
 
@@ -148,7 +147,6 @@ class IncidentReportingService : public content::NotificationObserver {
   // Initiates an upload. Overridden by unit tests to provide a fake uploader.
   virtual std::unique_ptr<IncidentReportUploader> StartReportUpload(
       const IncidentReportUploader::OnResultCallback& callback,
-      const scoped_refptr<net::URLRequestContextGetter>& request_context_getter,
       const ClientIncidentReport& report);
 
   // Returns true if a report is currently being processed.
@@ -271,8 +269,8 @@ class IncidentReportingService : public content::NotificationObserver {
                const content::NotificationSource& source,
                const content::NotificationDetails& details) override;
 
-  // Accessor for an URL context with which reports will be sent.
-  scoped_refptr<net::URLRequestContextGetter> url_request_context_getter_;
+  // Accessor for an URLLoaderFactory with which reports will be sent.
+  scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory_;
 
   // A pointer to a function that collects environment data. The function will
   // be run by |environment_collection_task_runner_|. This is ordinarily

chrome/browser/safe_browsing/incident_reporting/incident_reporting_service_unittest.cc

@@ -38,7 +38,6 @@
 #include "components/sync_preferences/testing_pref_service_syncable.h"
 #include "content/public/test/test_browser_thread_bundle.h"
 #include "extensions/browser/quota_service.h"
-#include "net/url_request/url_request_context_getter.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 #if defined(OS_WIN)
@@ -77,7 +76,6 @@ class IncidentReportingServiceTest : public testing::Test {
         const CreateDownloadFinderCallback& create_download_finder_callback,
         const StartUploadCallback& start_upload_callback)
         : IncidentReportingService(nullptr,
-                                   nullptr,
                                    base::TimeDelta::FromMilliseconds(5),
                                    task_runner),
           pre_profile_add_callback_(pre_profile_add_callback),
@@ -124,8 +122,6 @@ class IncidentReportingServiceTest : public testing::Test {
 
     std::unique_ptr<safe_browsing::IncidentReportUploader> StartReportUpload(
         const safe_browsing::IncidentReportUploader::OnResultCallback& callback,
-        const scoped_refptr<net::URLRequestContextGetter>&
-            request_context_getter,
         const safe_browsing::ClientIncidentReport& report) override {
       return start_upload_callback_.Run(callback, report);
     }