Convert certificate reporting service to use SimpleURLLoader.

With this change, CertificateReportingService now only lives on the UI thread.

Bug: 825242
TBR=rhalavati for test annotation removal

Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Change-Id: I3945ab88a8d2f0507b4a2572df6c79edd7aab036
Reviewed-on: https://chromium-review.googlesource.com/996021
Commit-Queue: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Mustafa Emre Acer <meacer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#548473}

chrome/browser/net/trial_comparison_cert_verifier.h

@@ -17,7 +17,7 @@ namespace net {
 class CertVerifyProc;
 }
 
-class NET_EXPORT TrialComparisonCertVerifier : public net::CertVerifier {
+class TrialComparisonCertVerifier : public net::CertVerifier {
  public:
   // These values are persisted to logs. Entries should not be renumbered and
   // numeric values should never be reused.

chrome/browser/net/trial_comparison_cert_verifier_unittest.cc

@@ -181,22 +181,17 @@ class TrialComparisonCertVerifierTest : public testing::Test {
         net::X509Certificate::FORMAT_AUTO);
     ASSERT_TRUE(cert_chain_2_);
 
-    reporting_service_test_helper_.SetUpInterceptor();
+    reporting_service_test_helper_ =
+        base::MakeRefCounted<CertificateReportingServiceTestHelper>();
     CertificateReportingServiceFactory::GetInstance()
         ->SetReportEncryptionParamsForTesting(
             reporting_service_test_helper()->server_public_key(),
             reporting_service_test_helper()->server_public_key_version());
+    CertificateReportingServiceFactory::GetInstance()
+        ->SetURLLoaderFactoryForTesting(reporting_service_test_helper_);
     reporting_service_test_helper()->SetFailureMode(
         certificate_reporting_test_utils::REPORTS_SUCCESSFUL);
 
-    url_request_context_getter_ =
-        base::MakeRefCounted<net::TestURLRequestContextGetter>(
-            (content::BrowserThread::GetTaskRunnerForThread(
-                content::BrowserThread::IO)));
-
-    TestingBrowserProcess::GetGlobal()->SetSystemRequestContext(
-        url_request_context_getter_.get());
-
     sb_service_ = base::MakeRefCounted<safe_browsing::TestSafeBrowsingService>(
         // Doesn't matter, just need to choose one.
         safe_browsing::V4FeatureList::V4UsageStatus::V4_DISABLED);
@@ -236,7 +231,7 @@ class TrialComparisonCertVerifierTest : public testing::Test {
   }
 
   CertificateReportingServiceTestHelper* reporting_service_test_helper() {
-    return &reporting_service_test_helper_;
+    return reporting_service_test_helper_.get();
   }
 
   CertificateReportingService* service() const {
@@ -252,12 +247,12 @@ class TrialComparisonCertVerifierTest : public testing::Test {
 
  private:
   content::TestBrowserThreadBundle thread_bundle_;
-  scoped_refptr<net::URLRequestContextGetter> url_request_context_getter_;
   scoped_refptr<safe_browsing::SafeBrowsingService> sb_service_;
   std::unique_ptr<TestingProfileManager> profile_manager_;
   TestingProfile* profile_;
 
-  CertificateReportingServiceTestHelper reporting_service_test_helper_;
+  scoped_refptr<CertificateReportingServiceTestHelper>
+      reporting_service_test_helper_;
 };
 
 TEST_F(TrialComparisonCertVerifierTest, NotOptedIn) {

chrome/browser/safe_browsing/certificate_reporting_service.h

@@ -17,7 +17,7 @@
 #include "base/time/time.h"
 #include "chrome/browser/ssl/certificate_error_reporter.h"
 #include "components/keyed_service/core/keyed_service.h"
-#include "net/url_request/url_request_context_getter.h"
+#include "services/network/public/cpp/shared_url_loader_factory.h"
 
 class PrefService;
 class Profile;
@@ -26,8 +26,8 @@ namespace base {
 class Clock;
 }
 
-namespace net {
-class URLRequestContextGetter;
+namespace network {
+class SharedURLLoaderFactory;
 }
 
 namespace safe_browsing {
@@ -41,16 +41,11 @@ class SafeBrowsingService;
 //
 // Lifetime and dependencies:
 //
-// CertificateReportingService uses the url request context from SafeBrowsing
+// CertificateReportingService uses the URLLoaderFactory from SafeBrowsing
 // service. SafeBrowsingService is created before CertificateReportingService,
-// but is also shut down before any KeyedService is shut down. This means that
-// CertificateReportingService cannot depend on SafeBrowsing's url request being
-// available at all times, and it should know when SafeBrowsing shuts down. It
-// does this by subscribing to SafeBrowsingService shut downs when it's
-// created. When SafeBrowsingService shuts down, CertificateReportingService
-// also shuts down.
+// but is also shut down before any KeyedService is shut down.
 //
-// This class also observes SafeBrowsing preference changes to enable/disable
+// This class observes SafeBrowsing preference changes to enable/disable
 // reporting. It does this by subscribing to changes in SafeBrowsing and
 // extended reporting preferences.
 class CertificateReportingService : public KeyedService {
@@ -142,6 +137,9 @@ class CertificateReportingService : public KeyedService {
     // Getter and setters for testing:
     size_t inflight_report_count_for_testing() const;
     BoundedReportList* GetQueueForTesting() const;
+    // Sets a closure that is called when there are no more inflight reports.
+    void SetClosureWhenNoInflightReportsForTesting(
+        const base::Closure& closure);
 
    private:
     void SendInternal(const Report& report);
@@ -149,7 +147,6 @@ class CertificateReportingService : public KeyedService {
     // non-HTTP 200 response code. See
     // TransportSecurityState::ReportSenderInterface for parameters.
     void ErrorCallback(int report_id,
-                       const GURL& url,
                        int net_error,
                        int http_response_code);
     // Called when a report upload is successful.
@@ -167,6 +164,8 @@ class CertificateReportingService : public KeyedService {
 
     std::map<int, Report> inflight_reports_;
 
+    base::Closure no_in_flight_reports_;
+
     base::WeakPtrFactory<Reporter> weak_factory_;
 
     DISALLOW_COPY_AND_ASSIGN(Reporter);
@@ -177,7 +176,7 @@ class CertificateReportingService : public KeyedService {
 
   CertificateReportingService(
       safe_browsing::SafeBrowsingService* safe_browsing_service,
-      scoped_refptr<net::URLRequestContextGetter> url_request_context_getter,
+      scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
       Profile* profile,
       uint8_t server_public_key[/* 32 */],
       uint32_t server_public_key_version,
@@ -208,40 +207,18 @@ class CertificateReportingService : public KeyedService {
   static GURL GetReportingURLForTesting();
 
  private:
-  void InitializeOnIOThread(
-      bool enabled,
-      scoped_refptr<net::URLRequestContextGetter> url_request_context_getter,
-      size_t max_queued_report_count,
-      base::TimeDelta max_report_age,
-      base::Clock* const clock,
-      uint8_t* server_public_key,
-      uint32_t server_public_key_version);
-
-  // Resets the reporter on the IO thread. Changes in SafeBrowsing or extended
-  // reporting enabled states cause the reporter to be reset.
-  // If |enabled| is false or |url_request_context_getter| is null, report is
-  // set to null, effectively cancelling all in flight uploads and clearing the
+  // Resets the reporter. Changes in SafeBrowsing or extended reporting enabled
+  // states cause the reporter to be reset. If |enabled| is false, report is set
+  // to null, effectively cancelling all in flight uploads and clearing the
   // pending reports queue.
-  void ResetOnIOThread(bool enabled,
-                       net::URLRequestContext* url_request_context,
-                       size_t max_queued_report_count,
-                       base::TimeDelta max_report_age,
-                       base::Clock* const clock,
-                       uint8_t* server_public_key,
-                       uint32_t server_public_key_version);
+  void Reset(bool enabled);
 
   void OnPreferenceChanged();
 
   const PrefService& pref_service_;
-  net::URLRequestContext* url_request_context_;
+  scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory_;
   std::unique_ptr<Reporter> reporter_;
 
-  // Subscription for url request context shutdowns. When this subscription is
-  // notified, it means SafeBrowsingService is shutting down, and this service
-  // must also shut down.
-  std::unique_ptr<base::CallbackList<void(void)>::Subscription>
-      safe_browsing_service_shutdown_subscription_;
-
   // Subscription for state changes. When this subscription is notified, it
   // means SafeBrowsingService is enabled/disabled or one of the preferences
   // related to it is changed.

chrome/browser/safe_browsing/certificate_reporting_service_browsertest.cc

@@ -82,7 +82,8 @@ class CertificateReportingServiceBrowserTest
     https_server_.ServeFilesFromSourceDirectory("chrome/test/data");
     ASSERT_TRUE(https_server_.Start());
 
-    test_helper()->SetUpInterceptor();
+    test_helper_ =
+        base::MakeRefCounted<CertificateReportingServiceTestHelper>();
 
     CertificateReportingServiceFactory::GetInstance()
         ->SetReportEncryptionParamsForTesting(
@@ -92,6 +93,8 @@ class CertificateReportingServiceBrowserTest
         ->SetServiceResetCallbackForTesting(
             base::Bind(&CertificateReportingServiceObserver::OnServiceReset,
                        base::Unretained(&service_observer_)));
+    CertificateReportingServiceFactory::GetInstance()
+        ->SetURLLoaderFactoryForTesting(test_helper_);
 
     event_histogram_tester_.reset(new EventHistogramTester());
     InProcessBrowserTest::SetUpOnMainThread();
@@ -126,7 +129,23 @@ class CertificateReportingServiceBrowserTest
     }
   }
 
-  CertificateReportingServiceTestHelper* test_helper() { return &test_helper_; }
+  CertificateReportingServiceTestHelper* test_helper() {
+    return test_helper_.get();
+  }
+
+  void WaitForNoReports() {
+    if (!service()->GetReporterForTesting() ||
+        !service()
+             ->GetReporterForTesting()
+             ->inflight_report_count_for_testing())
+      return;
+
+    base::RunLoop run_loop;
+    service()
+        ->GetReporterForTesting()
+        ->SetClosureWhenNoInflightReportsForTesting(run_loop.QuitClosure());
+    run_loop.Run();
+  }
 
  protected:
   CertificateReportingServiceFactory* factory() {
@@ -157,7 +176,10 @@ class CertificateReportingServiceBrowserTest
       content::WaitForInterstitialDetach(contents);
   }
 
-  void SendPendingReports() { service()->SendPending(); }
+  void SendPendingReports() {
+    WaitForNoReports();
+    service()->SendPending();
+  }
 
   // Changes opt-in status and waits for the cert reporting service to reset.
   // Can only be used after the service is initialized. When changing the
@@ -216,7 +238,7 @@ class CertificateReportingServiceBrowserTest
 
   int num_expected_failed_report_ = -1;
 
-  CertificateReportingServiceTestHelper test_helper_;
+  scoped_refptr<CertificateReportingServiceTestHelper> test_helper_;
 
   CertificateReportingServiceObserver service_observer_;
 
@@ -434,6 +456,8 @@ IN_PROC_BROWSER_TEST_P(CertificateReportingServiceBrowserTest,
   test_helper()->WaitForRequestsDestroyed(
       ReportExpectation::Successful({{"report1", RetryStatus::RETRIED}}));
 
+  WaitForNoReports();
+
   // report0 was submitted once, failed once, then cleared.
   // report1 was submitted twice, failed once, succeeded once.
   event_histogram_tester()->SetExpectedValues(
@@ -512,6 +536,8 @@ IN_PROC_BROWSER_TEST_P(CertificateReportingServiceBrowserTest,
   test_helper()->WaitForRequestsDestroyed(
       ReportExpectation::Successful({{"report3", RetryStatus::NOT_RETRIED}}));
 
+  WaitForNoReports();
+
   // report0 was submitted once, failed once, dropped once.
   // report1 was submitted twice, failed twice, dropped once.
   // report2 was submitted twice, failed twice, dropped once.
@@ -593,6 +619,8 @@ IN_PROC_BROWSER_TEST_P(CertificateReportingServiceBrowserTest,
   test_helper()->WaitForRequestsDestroyed(ReportExpectation::Successful(
       {{"report2", RetryStatus::RETRIED}, {"report3", RetryStatus::RETRIED}}));
 
+  WaitForNoReports();
+
   // report0 was submitted once, failed once, dropped once.
   // report1 was submitted twice, failed twice, dropped once.
   // report2 was submitted thrice, failed twice, succeeded once.
@@ -623,6 +651,8 @@ IN_PROC_BROWSER_TEST_P(CertificateReportingServiceBrowserTest,
   test_helper()->WaitForRequestsDestroyed(
       ReportExpectation::Delayed({{"report0", RetryStatus::NOT_RETRIED}}));
 
+  WaitForNoReports();
+
   // report0 was submitted once and succeeded once.
   event_histogram_tester()->SetExpectedValues(
       1 /* submitted */, 0 /* failed */, 1 /* successful */, 0 /* dropped */);
@@ -674,6 +704,11 @@ IN_PROC_BROWSER_TEST_P(CertificateReportingServiceBrowserTest, Delayed_Reset) {
 
   // Disable SafeBrowsing. This should clear all pending reports.
   ToggleSafeBrowsingAndWaitForServiceReset(false);
+
+  // In production, the request would have already went out to the network. For
+  // this test, we manually resume it which will cause it to be cancelled.
+  test_helper()->ResumeDelayedRequest();
+
   test_helper()->WaitForRequestsDestroyed(
       ReportExpectation::Delayed({{"report0", RetryStatus::NOT_RETRIED}}));
 
@@ -695,6 +730,8 @@ IN_PROC_BROWSER_TEST_P(CertificateReportingServiceBrowserTest, Delayed_Reset) {
   test_helper()->WaitForRequestsDestroyed(
       ReportExpectation::Delayed({{"report1", RetryStatus::NOT_RETRIED}}));
 
+  WaitForNoReports();
+
   // report0 was submitted once and delayed, then cleared.
   // report1 was submitted once and delayed, then succeeded.
   event_histogram_tester()->SetExpectedValues(

chrome/browser/safe_browsing/certificate_reporting_service_factory.cc

@@ -2,13 +2,15 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "chrome/browser/safe_browsing/certificate_reporting_service_factory.h"
+
 #include "base/time/default_clock.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/safe_browsing/certificate_reporting_service.h"
-#include "chrome/browser/safe_browsing/certificate_reporting_service_factory.h"
 #include "chrome/browser/safe_browsing/safe_browsing_service.h"
 #include "components/keyed_service/content/browser_context_dependency_manager.h"
+#include "services/network/public/cpp/shared_url_loader_factory.h"
 
 namespace {
 
@@ -65,6 +67,11 @@ void CertificateReportingServiceFactory::SetServiceResetCallbackForTesting(
   service_reset_callback_ = service_reset_callback;
 }
 
+void CertificateReportingServiceFactory::SetURLLoaderFactoryForTesting(
+    scoped_refptr<network::SharedURLLoaderFactory> factory) {
+  url_loader_factory_ = factory;
+}
+
 CertificateReportingServiceFactory::CertificateReportingServiceFactory()
     : BrowserContextKeyedServiceFactory(
           "cert_reporting::Factory",
@@ -83,7 +90,9 @@ KeyedService* CertificateReportingServiceFactory::BuildServiceInstanceFor(
   safe_browsing::SafeBrowsingService* safe_browsing_service =
       g_browser_process->safe_browsing_service();
   return new CertificateReportingService(
-      safe_browsing_service, safe_browsing_service->url_request_context(),
+      safe_browsing_service,
+      url_loader_factory_.get() ? url_loader_factory_
+                                : safe_browsing_service->GetURLLoaderFactory(),
       static_cast<Profile*>(profile), server_public_key_,
       server_public_key_version_, max_queued_report_count_, queued_report_ttl_,
       clock_, service_reset_callback_);

chrome/browser/safe_browsing/certificate_reporting_service_factory.h

@@ -7,6 +7,7 @@
 
 #include "base/memory/singleton.h"
 #include "components/keyed_service/content/browser_context_keyed_service_factory.h"
+#include "services/network/public/cpp/shared_url_loader_factory.h"
 
 namespace base {
 class Clock;
@@ -36,6 +37,8 @@ class CertificateReportingServiceFactory
   void SetMaxQueuedReportCountForTesting(size_t max_report_count);
   void SetServiceResetCallbackForTesting(
       const base::Callback<void()>& service_reset_callback);
+  void SetURLLoaderFactoryForTesting(
+      scoped_refptr<network::SharedURLLoaderFactory> factory);
 
  private:
   friend struct base::DefaultSingletonTraits<
@@ -58,6 +61,7 @@ class CertificateReportingServiceFactory
   base::TimeDelta queued_report_ttl_;
   size_t max_queued_report_count_;
   base::Callback<void()> service_reset_callback_;
+  scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(CertificateReportingServiceFactory);
 };

chrome/browser/safe_browsing/certificate_reporting_service_test_utils.h

@@ -17,6 +17,7 @@
 #include "net/base/network_delegate_impl.h"
 #include "net/url_request/url_request_interceptor.h"
 #include "net/url_request/url_request_job.h"
+#include "services/network/public/cpp/shared_url_loader_factory.h"
 
 namespace net {
 class NetworkDelegate;
@@ -139,51 +140,6 @@ class DelayableCertReportURLRequestJob : public net::URLRequestJob {
   DISALLOW_COPY_AND_ASSIGN(DelayableCertReportURLRequestJob);
 };
 
-// A job interceptor that returns a failed, succesful or delayed request job.
-// Used to simulate report uploads that fail, succeed or hang.
-// The caller is responsible for guaranteeing that |this| is kept alive for
-// all posted tasks and URLRequestJob objects.
-class CertReportJobInterceptor : public net::URLRequestInterceptor {
- public:
-  CertReportJobInterceptor(ReportSendingResult expected_report_result,
-                           const uint8_t* server_private_key);
-  ~CertReportJobInterceptor() override;
-
-  // net::URLRequestInterceptor method:
-  net::URLRequestJob* MaybeInterceptRequest(
-      net::URLRequest* request,
-      net::NetworkDelegate* network_delegate) const override;
-
-  // Sets the failure mode for reports. Must be called on the UI thread.
-  void SetFailureMode(ReportSendingResult expected_report_result);
-  // Resumes any hanging URL request and runs callback when the request
-  // is resumed (i.e. response starts). Must be called on the UI thread.
-  void Resume();
-
-  RequestObserver* request_created_observer() const;
-  RequestObserver* request_destroyed_observer() const;
-
- private:
-  void SetFailureModeOnIOThread(ReportSendingResult expected_report_result);
-  void ResumeOnIOThread();
-  void RequestCreated(const std::string& serialized_report,
-                      ReportSendingResult expected_report_result) const;
-  void RequestDestructed(const std::string& serialized_report,
-                         ReportSendingResult expected_report_result) const;
-
-  ReportSendingResult expected_report_result_;
-
-  // Private key to decrypt certificate reports.
-  const uint8_t* server_private_key_;
-
-  mutable RequestObserver request_created_observer_;
-  mutable RequestObserver request_destroyed_observer_;
-
-  mutable base::WeakPtr<DelayableCertReportURLRequestJob> delayed_request_;
-
-  DISALLOW_COPY_AND_ASSIGN(CertReportJobInterceptor);
-};
-
 // Class to wait for the CertificateReportingService to reset.
 class CertificateReportingServiceObserver {
  public:
@@ -204,14 +160,11 @@ class CertificateReportingServiceObserver {
   std::unique_ptr<base::RunLoop> run_loop_;
 };
 
-// Base class for CertificateReportingService tests. Sets up an interceptor to
-// keep track of reports that are being sent.
-class CertificateReportingServiceTestHelper {
+// Base class for CertificateReportingService tests.
+class CertificateReportingServiceTestHelper
+    : public network::SharedURLLoaderFactory {
  public:
   CertificateReportingServiceTestHelper();
-  ~CertificateReportingServiceTestHelper();
-
-  void SetUpInterceptor();
 
   // Changes the behavior of report uploads to fail, succeed or hang.
   void SetFailureMode(ReportSendingResult expected_report_result);
@@ -235,10 +188,30 @@ class CertificateReportingServiceTestHelper {
   uint32_t server_public_key_version() const;
 
  private:
-  CertReportJobInterceptor* interceptor() { return url_request_interceptor_; }
-  void SetUpInterceptorOnIOThread();
+  friend class base::RefCounted<CertificateReportingServiceTestHelper>;
+  ~CertificateReportingServiceTestHelper() override;
+
+  void SendResponse(network::mojom::URLLoaderClientPtr client, bool fail);
+
+  // network::SharedURLLoaderFactory
+  std::unique_ptr<network::SharedURLLoaderFactoryInfo> Clone() override;
+  void CreateLoaderAndStart(network::mojom::URLLoaderRequest request,
+                            int32_t routing_id,
+                            int32_t request_id,
+                            uint32_t options,
+                            const network::ResourceRequest& url_request,
+                            network::mojom::URLLoaderClientPtr client,
+                            const net::MutableNetworkTrafficAnnotationTag&
+                                traffic_annotation) override;
+
+  ReportSendingResult expected_report_result_;
+
+  network::mojom::URLLoaderClientPtr delayed_client_;
+  std::string delayed_report_;
+  ReportSendingResult delayed_result_;
 
-  CertReportJobInterceptor* url_request_interceptor_;
+  RequestObserver request_created_observer_;
+  RequestObserver request_destroyed_observer_;
 
   uint8_t server_public_key_[32];
   uint8_t server_private_key_[32];

chrome/browser/safe_browsing/safe_browsing_service.cc

@@ -176,7 +176,6 @@ void SafeBrowsingService::Initialize() {
 
 void SafeBrowsingService::ShutDown() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  shutdown_callback_list_.Notify();
 
   // Remove Profile creation/destruction observers.
   profiles_registrar_.RemoveAll();
@@ -546,13 +545,6 @@ SafeBrowsingService::RegisterStateCallback(
   return state_callback_list_.Add(callback);
 }
 
-std::unique_ptr<SafeBrowsingService::ShutdownSubscription>
-SafeBrowsingService::RegisterShutdownCallback(
-    const base::Callback<void(void)>& callback) {
-  DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  return shutdown_callback_list_.Add(callback);
-}
-
 void SafeBrowsingService::RefreshState() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   // Check if any profile requires the service to be active.

chrome/browser/safe_browsing/safe_browsing_service.h

@@ -210,12 +210,6 @@ class SafeBrowsingService : public base::RefCountedThreadSafe<
   std::unique_ptr<StateSubscription> RegisterStateCallback(
       const base::Callback<void(void)>& callback);
 
-  // Adds a listener for when SafeBrowsingService starts shutting down.
-  // The callbacks run on the UI thread, and give the subscribers an opportunity
-  // to clean up any references they hold to SafeBrowsingService.
-  std::unique_ptr<ShutdownSubscription> RegisterShutdownCallback(
-      const base::Callback<void(void)>& callback);
-
   // Sends serialized download report to backend.
   virtual void SendSerializedDownloadReport(const std::string& report);
 
@@ -350,10 +344,6 @@ class SafeBrowsingService : public base::RefCountedThreadSafe<
   // Should only be accessed on the UI thread.
   base::CallbackList<void(void)> state_callback_list_;
 
-  // Callbacks when SafeBrowsing service starts shutting down.
-  // Should only be accessed on the UI thread.
-  base::CallbackList<void(void)> shutdown_callback_list_;
-
   // The UI manager handles showing interstitials.  Accessed on both UI and IO
   // thread.
   scoped_refptr<SafeBrowsingUIManager> ui_manager_;

chrome/browser/ssl/DEPS

@@ -2,6 +2,7 @@ include_rules = [
   "+components/captive_portal",
   "+components/certificate_transparency",
   "+components/wifi",
+  "+services/network/test",
 ]
 
 specific_include_rules = {

chrome/browser/ssl/certificate_error_reporter.cc

@@ -13,10 +13,12 @@
 
 #include "base/logging.h"
 #include "base/strings/string_piece.h"
+#include "chrome/browser/net/chrome_report_sender.h"
 #include "components/encrypted_messages/encrypted_message.pb.h"
 #include "components/encrypted_messages/message_encrypter.h"
 #include "net/traffic_annotation/network_traffic_annotation.h"
 #include "net/url_request/report_sender.h"
+#include "services/network/public/cpp/shared_url_loader_factory.h"
 
 namespace {
 
@@ -72,26 +74,22 @@ constexpr net::NetworkTrafficAnnotationTag
 }  // namespace
 
 CertificateErrorReporter::CertificateErrorReporter(
-    net::URLRequestContext* request_context,
+    scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
     const GURL& upload_url)
-    : CertificateErrorReporter(
-          upload_url,
-          kServerPublicKey,
-          kServerPublicKeyVersion,
-          std::make_unique<net::ReportSender>(
-              request_context,
-              kSafeBrowsingCertificateErrorReportingTrafficAnnotation)) {}
+    : CertificateErrorReporter(url_loader_factory,
+                               upload_url,
+                               kServerPublicKey,
+                               kServerPublicKeyVersion) {}
 
 CertificateErrorReporter::CertificateErrorReporter(
+    scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
     const GURL& upload_url,
     const uint8_t server_public_key[/* 32 */],
-    const uint32_t server_public_key_version,
-    std::unique_ptr<net::ReportSender> certificate_report_sender)
-    : certificate_report_sender_(std::move(certificate_report_sender)),
+    const uint32_t server_public_key_version)
+    : url_loader_factory_(url_loader_factory),
       upload_url_(upload_url),
       server_public_key_(server_public_key),
       server_public_key_version_(server_public_key_version) {
-  DCHECK(certificate_report_sender_);
   DCHECK(!upload_url.is_empty());
 }
 
@@ -99,31 +97,32 @@ CertificateErrorReporter::~CertificateErrorReporter() {}
 
 void CertificateErrorReporter::SendExtendedReportingReport(
     const std::string& serialized_report,
-    const base::Callback<void()>& success_callback,
-    const base::Callback<void(const GURL&, int, int)>& error_callback) {
-  if (upload_url_.SchemeIsCryptographic()) {
-    certificate_report_sender_->Send(upload_url_, "application/octet-stream",
-                                     serialized_report, success_callback,
-                                     error_callback);
-    return;
-  }
-  encrypted_messages::EncryptedMessage encrypted_report;
-  // By mistake, the HKDF label here ends up with an extra null byte on
-  // the end, due to using sizeof(kHkdfLabel) in the StringPiece
-  // constructor instead of strlen(kHkdfLabel). This has since been changed
-  // to strlen() + 1, but will need to be fixed in future to just be strlen.
-  // TODO(estark): fix this...
-  //  https://crbug.com/517746
-  if (!encrypted_messages::EncryptSerializedMessage(
-          server_public_key_, server_public_key_version_,
-          base::StringPiece(kHkdfLabel, strlen(kHkdfLabel) + 1),
-          serialized_report, &encrypted_report)) {
-    LOG(ERROR) << "Failed to encrypt serialized report.";
-    return;
-  }
+    base::OnceCallback<void()> success_callback,
+    base::OnceCallback<void(int, int)> error_callback) {
   std::string serialized_encrypted_report;
-  encrypted_report.SerializeToString(&serialized_encrypted_report);
-  certificate_report_sender_->Send(upload_url_, "application/octet-stream",
-                                   serialized_encrypted_report,
-                                   success_callback, error_callback);
+  const std::string* string_to_send = &serialized_report;
+  if (!upload_url_.SchemeIsCryptographic()) {
+    encrypted_messages::EncryptedMessage encrypted_report;
+    // By mistake, the HKDF label here ends up with an extra null byte on
+    // the end, due to using sizeof(kHkdfLabel) in the StringPiece
+    // constructor instead of strlen(kHkdfLabel). This has since been changed
+    // to strlen() + 1, but will need to be fixed in future to just be strlen.
+    // TODO(estark): fix this...
+    //  https://crbug.com/517746
+    if (!encrypted_messages::EncryptSerializedMessage(
+            server_public_key_, server_public_key_version_,
+            base::StringPiece(kHkdfLabel, strlen(kHkdfLabel) + 1),
+            serialized_report, &encrypted_report)) {
+      LOG(ERROR) << "Failed to encrypt serialized report.";
+      return;
+    }
+
+    encrypted_report.SerializeToString(&serialized_encrypted_report);
+    string_to_send = &serialized_encrypted_report;
+  }
+
+  SendReport(url_loader_factory_,
+             kSafeBrowsingCertificateErrorReportingTrafficAnnotation,
+             upload_url_, "application/octet-stream", *string_to_send,
+             std::move(success_callback), std::move(error_callback));
 }

chrome/browser/ssl/certificate_error_reporter.h

@@ -13,11 +13,11 @@
 
 #include "base/callback.h"
 #include "base/macros.h"
-#include "net/url_request/report_sender.h"
+#include "base/memory/ref_counted.h"
 #include "url/gurl.h"
 
-namespace net {
-class URLRequestContext;
+namespace network {
+class SharedURLLoaderFactory;
 }
 
 // Provides functionality for sending reports about invalid SSL
@@ -25,19 +25,19 @@ class URLRequestContext;
 class CertificateErrorReporter {
  public:
   // Creates a certificate error reporter that will send certificate
-  // error reports to |upload_url|, using |request_context| as the
+  // error reports to |upload_url|, using |url_loader_factory| as the
   // context for the reports.
-  CertificateErrorReporter(net::URLRequestContext* request_context,
-                           const GURL& upload_url);
+  CertificateErrorReporter(
+      scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
+      const GURL& upload_url);
 
-  // Allows tests to use a server public key with known private key and
-  // a mock ReportSender. |server_public_key| must outlive
-  // the ErrorReporter.
+  // Allows tests to use a server public key with known private key.
+  // |server_public_key| must outlive the ErrorReporter.
   CertificateErrorReporter(
+      scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
       const GURL& upload_url,
       const uint8_t server_public_key[/* 32 */],
-      const uint32_t server_public_key_version,
-      std::unique_ptr<net::ReportSender> certificate_report_sender);
+      const uint32_t server_public_key_version);
 
   virtual ~CertificateErrorReporter();
 
@@ -63,16 +63,14 @@ class CertificateErrorReporter {
   // net error and HTTP response code parameters.
   virtual void SendExtendedReportingReport(
       const std::string& serialized_report,
-      const base::Callback<void()>& success_callback,
-      const base::Callback<void(const GURL&,
-                                int /* net_error */,
-                                int /* http_response_code */)>& error_callback);
+      base::OnceCallback<void()> success_callback,
+      base::OnceCallback<void(int /* net_error */,
+                              int /* http_response_code */)> error_callback);
 
   void set_upload_url_for_testing(const GURL& url) { upload_url_ = url; }
 
  private:
-  std::unique_ptr<net::ReportSender> certificate_report_sender_;
-
+  scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory_;
   GURL upload_url_;
 
   const uint8_t* server_public_key_;

services/network/test/test_url_loader_factory.cc

@@ -96,13 +96,16 @@ bool TestURLLoaderFactory::CreateLoaderAndStartInternal(
     return false;
 
   CHECK(it->second.redirects.empty()) << "TODO(jam): handle redirects";
-  client->OnReceiveResponse(it->second.head, nullptr);
-  mojo::DataPipe data_pipe(it->second.content.size());
-  uint32_t bytes_written = it->second.content.size();
-  CHECK_EQ(MOJO_RESULT_OK, data_pipe.producer_handle->WriteData(
-                               it->second.content.data(), &bytes_written,
-                               MOJO_WRITE_DATA_FLAG_ALL_OR_NONE));
-  client->OnStartLoadingResponseBody(std::move(data_pipe.consumer_handle));
+
+  if (it->second.status.error_code == net::OK) {
+    client->OnReceiveResponse(it->second.head, nullptr);
+    mojo::DataPipe data_pipe(it->second.content.size());
+    uint32_t bytes_written = it->second.content.size();
+    CHECK_EQ(MOJO_RESULT_OK, data_pipe.producer_handle->WriteData(
+                                 it->second.content.data(), &bytes_written,
+                                 MOJO_WRITE_DATA_FLAG_ALL_OR_NONE));
+    client->OnStartLoadingResponseBody(std::move(data_pipe.consumer_handle));
+  }
   client->OnComplete(it->second.status);
   return true;
 }

tools/traffic_annotation/summary/annotations.xml

@@ -36,7 +36,6 @@ Refer to README.md for content description and update process.
  <item id="cast_socket" hash_code="115192205" type="0" content_hash_code="63056899" os_list="linux,windows" file_path="components/cast_channel/cast_socket.cc"/>
  <item id="cast_udp_socket" hash_code="22573197" type="0" content_hash_code="75328301" os_list="linux,windows" file_path="components/mirroring/service/udp_socket_client.cc"/>
  <item id="cast_udp_transport" hash_code="5576536" type="0" content_hash_code="107643273" os_list="linux,windows" file_path="media/cast/net/udp_transport_impl.cc"/>
- <item id="certificate_reporting_service_test" hash_code="98123372" type="0" content_hash_code="136253658" os_list="linux,windows" file_path="chrome/browser/safe_browsing/certificate_reporting_service.cc"/>
  <item id="certificate_verifier" hash_code="113553577" type="0" content_hash_code="62346354" os_list="linux,windows" file_path="net/cert_net/cert_net_fetcher_impl.cc"/>
  <item id="chrome_apps_socket_api" hash_code="8591273" type="0" content_hash_code="70868355" os_list="linux,windows" file_path="extensions/browser/api/socket/socket.cc"/>
  <item id="chrome_cleaner" hash_code="27071967" type="0" content_hash_code="111240292" os_list="windows" file_path="chrome/browser/safe_browsing/chrome_cleaner/chrome_cleaner_fetcher_win.cc"/>