Migrate ResetReportUploader to SimpleURLLoader

The previous attempt was reverted because it crashed. This attempt
features a crash test which reproduces the crash when run against
the previous CL, and confirms that this CL does not crash.

The cause of the crash was a reference to simple_url_loader after it
had been std:move'd. This CL instead gets a reference to the newly
moved pointer.

previous CL: https://chromium-review.googlesource.com/c/chromium/src/+/1025068
revert CL: https://chromium-review.googlesource.com/c/chromium/src/+/1030590

Bug: 773295
Test: ResetReportUploaderTest.NoCrash
Change-Id: I7092ec501872dfa0019d37e8f9e10454458499db
Reviewed-on: https://chromium-review.googlesource.com/1042359
Commit-Queue: Mark Pilgrim <pilgrim@chromium.org>
Reviewed-by: Dominic Battré <battre@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#556447}

chrome/browser/profile_resetter/reset_report_uploader.cc

@@ -13,8 +13,9 @@
 #include "net/base/escape.h"
 #include "net/base/load_flags.h"
 #include "net/traffic_annotation/network_traffic_annotation.h"
-#include "net/url_request/url_fetcher.h"
-#include "net/url_request/url_request_context_getter.h"
+#include "services/network/public/cpp/resource_request.h"
+#include "services/network/public/cpp/simple_url_loader.h"
+#include "services/network/public/mojom/url_loader_factory.mojom.h"
 
 namespace {
 const char kResetReportUrl[] =
@@ -31,10 +32,9 @@ GURL GetClientReportUrl(const std::string& report_url) {
 
 }  // namespace
 
-ResetReportUploader::ResetReportUploader(content::BrowserContext* context)
-    : url_request_context_getter_(
-          content::BrowserContext::GetDefaultStoragePartition(context)->
-              GetURLRequestContext()) {}
+ResetReportUploader::ResetReportUploader(
+    scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory)
+    : url_loader_factory_(std::move(url_loader_factory)) {}
 
 ResetReportUploader::~ResetReportUploader() {}
 
@@ -43,6 +43,11 @@ void ResetReportUploader::DispatchReport(
   std::string request_data;
   CHECK(report.SerializeToString(&request_data));
 
+  DispatchReportInternal(request_data);
+}
+
+void ResetReportUploader::DispatchReportInternal(
+    const std::string& request_data) {
   // Create traffic annotation tag.
   net::NetworkTrafficAnnotationTag traffic_annotation =
       net::DefineNetworkTrafficAnnotation("profile_resetter_upload", R"(
@@ -72,19 +77,31 @@ void ResetReportUploader::DispatchReport(
             "send the data."
         })");
 
-  // Note fetcher will be deleted by OnURLFetchComplete.
-  net::URLFetcher* fetcher =
-      net::URLFetcher::Create(GetClientReportUrl(kResetReportUrl),
-                              net::URLFetcher::POST, this, traffic_annotation)
-          .release();
-  fetcher->SetLoadFlags(net::LOAD_DO_NOT_SEND_COOKIES |
-                        net::LOAD_DO_NOT_SAVE_COOKIES |
-                        net::LOAD_DISABLE_CACHE);
-  fetcher->SetRequestContext(url_request_context_getter_.get());
-  fetcher->SetUploadData("application/octet-stream", request_data);
-  fetcher->Start();
+  auto resource_request = std::make_unique<network::ResourceRequest>();
+  resource_request->url = GetClientReportUrl(kResetReportUrl);
+  resource_request->load_flags = net::LOAD_DO_NOT_SEND_COOKIES |
+                                 net::LOAD_DO_NOT_SAVE_COOKIES |
+                                 net::LOAD_DISABLE_CACHE;
+  resource_request->method = "POST";
+  std::unique_ptr<network::SimpleURLLoader> simple_url_loader =
+      network::SimpleURLLoader::Create(std::move(resource_request),
+                                       traffic_annotation);
+  simple_url_loader->AttachStringForUpload(request_data,
+                                           "application/octet-stream");
+  auto it = simple_url_loaders_.insert(simple_url_loaders_.begin(),
+                                       std::move(simple_url_loader));
+  it->get()->DownloadToStringOfUnboundedSizeUntilCrashAndDie(
+      url_loader_factory_.get(),
+      base::BindOnce(&ResetReportUploader::OnSimpleLoaderComplete,
+                     base::Unretained(this), std::move(it)));
+}
+
+void ResetReportUploader::OnSimpleLoaderComplete(
+    SimpleURLLoaderList::iterator it,
+    std::unique_ptr<std::string> response_body) {
+  simple_url_loaders_.erase(it);
 }
 
-void ResetReportUploader::OnURLFetchComplete(const net::URLFetcher* source) {
-  delete source;
+GURL ResetReportUploader::GetClientReportUrlForTesting() {
+  return GetClientReportUrl(kResetReportUrl);
 }

chrome/browser/profile_resetter/reset_report_uploader.h

@@ -5,18 +5,16 @@
 #ifndef CHROME_BROWSER_PROFILE_RESETTER_RESET_REPORT_UPLOADER_H_
 #define CHROME_BROWSER_PROFILE_RESETTER_RESET_REPORT_UPLOADER_H_
 
+#include <list>
+
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "components/keyed_service/core/keyed_service.h"
-#include "net/url_request/url_fetcher_delegate.h"
-
-namespace content {
-class BrowserContext;
-}
+#include "url/gurl.h"
 
-namespace net {
-class URLFetcher;
-class URLRequestContextGetter;
+namespace network {
+class SimpleURLLoader;
+class SharedURLLoaderFactory;
 }
 
 namespace reset_report {
@@ -24,18 +22,27 @@ class ChromeResetReport;
 }
 
 // Service whose job is up upload ChromeResetReports.
-class ResetReportUploader : public KeyedService,
-                            private net::URLFetcherDelegate {
+class ResetReportUploader : public KeyedService {
  public:
-  explicit ResetReportUploader(content::BrowserContext* context);
+  explicit ResetReportUploader(
+      scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory);
   ~ResetReportUploader() override;
 
   void DispatchReport(const reset_report::ChromeResetReport& report);
 
+  // Visible for testing:
+  void DispatchReportInternal(const std::string& request_data);
+  static GURL GetClientReportUrlForTesting();
+
  private:
-  void OnURLFetchComplete(const net::URLFetcher* source) override;
+  using SimpleURLLoaderList =
+      std::list<std::unique_ptr<network::SimpleURLLoader>>;
+
+  void OnSimpleLoaderComplete(SimpleURLLoaderList::iterator it,
+                              std::unique_ptr<std::string> response_body);
 
-  scoped_refptr<net::URLRequestContextGetter> url_request_context_getter_;
+  scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory_;
+  SimpleURLLoaderList simple_url_loaders_;
 
   DISALLOW_COPY_AND_ASSIGN(ResetReportUploader);
 };

chrome/browser/profile_resetter/reset_report_uploader_factory.cc

@@ -7,6 +7,8 @@
 #include "base/memory/singleton.h"
 #include "chrome/browser/profile_resetter/reset_report_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"
 
 // static
 ResetReportUploaderFactory* ResetReportUploaderFactory::GetInstance() {
@@ -29,5 +31,7 @@ ResetReportUploaderFactory::~ResetReportUploaderFactory() {}
 
 KeyedService* ResetReportUploaderFactory::BuildServiceInstanceFor(
     content::BrowserContext* context) const {
-  return new ResetReportUploader(context);
+  return new ResetReportUploader(
+      content::BrowserContext::GetDefaultStoragePartition(context)
+          ->GetURLLoaderFactoryForBrowserProcess());
 }

chrome/browser/profile_resetter/reset_report_uploader_unittest.cc

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/profile_resetter/reset_report_uploader.h"
+
+#include "base/files/file_path.h"
+#include "base/files/file_util.h"
+#include "base/files/scoped_temp_dir.h"
+#include "base/test/scoped_task_environment.h"
+#include "base/threading/thread_task_runner_handle.h"
+#include "content/public/common/weak_wrapper_shared_url_loader_factory.h"
+#include "content/public/test/test_utils.h"
+#include "net/http/http_status_code.h"
+#include "net/traffic_annotation/network_traffic_annotation_test_helper.h"
+#include "services/network/test/test_url_loader_factory.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "url/gurl.h"
+
+class ResetReportUploaderTest : public testing::Test {
+ public:
+  ResetReportUploaderTest()
+      : test_shared_loader_factory_(
+            base::MakeRefCounted<content::WeakWrapperSharedURLLoaderFactory>(
+                &test_url_loader_factory_)) {}
+
+ protected:
+  scoped_refptr<network::SharedURLLoaderFactory> shared_url_loader_factory() {
+    return test_shared_loader_factory_;
+  }
+  network::TestURLLoaderFactory* test_url_loader_factory() {
+    return &test_url_loader_factory_;
+  }
+
+ private:
+  base::test::ScopedTaskEnvironment scoped_task_environment_;
+  network::TestURLLoaderFactory test_url_loader_factory_;
+  scoped_refptr<network::SharedURLLoaderFactory> test_shared_loader_factory_;
+};
+
+TEST_F(ResetReportUploaderTest, NoCrash) {
+  test_url_loader_factory()->AddResponse(
+      ResetReportUploader::GetClientReportUrlForTesting().spec(), "");
+  ResetReportUploader* uploader =
+      new ResetReportUploader(shared_url_loader_factory());
+  uploader->DispatchReportInternal("");
+}

chrome/test/BUILD.gn

@@ -2934,6 +2934,7 @@ test("unit_tests") {
       "../browser/platform_util_unittest.cc",
       "../browser/policy/policy_path_parser_unittest.cc",
       "../browser/profile_resetter/profile_resetter_unittest.cc",
+      "../browser/profile_resetter/reset_report_uploader_unittest.cc",
       "../browser/profile_resetter/triggered_profile_resetter_win_unittest.cc",
       "../browser/renderer_context_menu/render_view_context_menu_unittest.cc",
       "../browser/safe_browsing/chrome_cleaner/chrome_cleaner_controller_impl_win_unittest.cc",