Migrate components/data_reduction_proxy/core/browser/secure_proxy_checker.cc...

Migrate components/data_reduction_proxy/core/browser/secure_proxy_checker.cc to using SimpleURLLoader

URLFetcher will stop working with advent of Network Service, and
SimpleURLLoader is the replacement API for most clients.
This CL migrates DRP's SecureProxyChecker and the
respective unittests away from URLFetcher.

Note that according to tbansal@, using a separate URL request context
that disables SPDY/QUIC is not be needed anymore.
Another requirement of DRP's SecureProxyChecker is that the use of any proxies
is also disabled. For this, the load flag LOAD_BYPASS_PROXY should be enough.

This means that the customized class BasicHTTPURLRequestContextGetter is not
used anymore, and will be removed in a follow up pass.

BUG=879783

Change-Id: I5f62007c284b35f9364caec37984cbf63e813720
Reviewed-on: https://chromium-review.googlesource.com/c/1281643Reviewed-by: Tarun Bansal <tbansal@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Commit-Queue: Antonio Gomes <tonikitoo@igalia.com>
Cr-Commit-Position: refs/heads/master@{#600022}

components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc

@@ -49,7 +49,6 @@
 #include "net/url_request/url_request.h"
 #include "net/url_request/url_request_context.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"
 
 #if defined(OS_ANDROID)
@@ -217,10 +216,9 @@ void DataReductionProxyConfig::InitializeOnIOThread(
   network_properties_manager_ = manager;
   network_properties_manager_->ResetWarmupURLFetchMetrics();
 
-  secure_proxy_checker_.reset(
-      new SecureProxyChecker(basic_url_request_context_getter));
+  secure_proxy_checker_.reset(new SecureProxyChecker(url_loader_factory));
   warmup_url_fetcher_.reset(new WarmupURLFetcher(
-      std::move(url_loader_factory),
+      url_loader_factory,
       base::BindRepeating(
           &DataReductionProxyConfig::HandleWarmupFetcherResponse,
           base::Unretained(this)),
@@ -576,21 +574,20 @@ void DataReductionProxyConfig::HandleWarmupFetcherResponse(
 
 void DataReductionProxyConfig::HandleSecureProxyCheckResponse(
     const std::string& response,
-    const net::URLRequestStatus& status,
+    int net_status,
     int http_response_code) {
   bool success_response =
       base::StartsWith(response, "OK", base::CompareCase::SENSITIVE);
 
-  if (!status.is_success()) {
-    if (status.error() == net::ERR_INTERNET_DISCONNECTED) {
+  if (net_status != net::OK) {
+    if (net_status == net::ERR_INTERNET_DISCONNECTED) {
       RecordSecureProxyCheckFetchResult(INTERNET_DISCONNECTED);
       return;
     }
     // TODO(bengr): Remove once we understand the reasons secure proxy checks
     // are failing. Secure proxy check errors are either due to fetcher-level
     // errors or modified responses. This only tracks the former.
-    base::UmaHistogramSparse(kUMAProxyProbeURLNetError,
-                             std::abs(status.error()));
+    base::UmaHistogramSparse(kUMAProxyProbeURLNetError, std::abs(net_status));
   }
 
   bool secure_proxy_allowed_past =

components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h

@@ -38,7 +38,6 @@ namespace net {
 class ProxyServer;
 class URLRequest;
 class URLRequestContextGetter;
-class URLRequestStatus;
 }  // namespace net
 
 namespace data_reduction_proxy {
@@ -267,7 +266,7 @@ class DataReductionProxyConfig
   // Parses the secure proxy check responses and appropriately configures the
   // Data Reduction Proxy rules.
   void HandleSecureProxyCheckResponse(const std::string& response,
-                                      const net::URLRequestStatus& status,
+                                      int net_status,
                                       int http_response_code);
 
   // Adds the default proxy bypass rules for the Data Reduction Proxy.

components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc

@@ -130,7 +130,7 @@ class DataReductionProxyConfigTest : public testing::Test {
   class TestResponder {
    public:
     void ExecuteCallback(SecureProxyCheckerCallback callback) {
-      callback.Run(response, status, http_response_code);
+      callback.Run(response, status.error(), http_response_code);
     }
 
     std::string response;
@@ -138,6 +138,7 @@ class DataReductionProxyConfigTest : public testing::Test {
     int http_response_code;
   };
 
+  // TODO(tonikitoo): Get rid of the use of net::URLRequestStatus altogether.
   void CheckSecureProxyCheckOnNetworkChange(
       network::mojom::ConnectionType connection_type,
       const std::string& response,

components/data_reduction_proxy/core/browser/data_reduction_proxy_settings_unittest.cc

@@ -32,8 +32,8 @@
 #include "components/data_reduction_proxy/core/common/data_reduction_proxy_switches.h"
 #include "components/prefs/pref_registry_simple.h"
 #include "net/base/proxy_server.h"
-#include "net/socket/socket_test_util.h"
-#include "net/url_request/url_request_test_util.h"
+#include "services/network/public/cpp/weak_wrapper_shared_url_loader_factory.h"
+#include "services/network/test/test_url_loader_factory.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -210,38 +210,37 @@ TEST(DataReductionProxySettingsStandaloneTest, TestEndToEndSecureProxyCheck) {
   };
 
   for (const TestCase& test_case : kTestCases) {
-    net::TestURLRequestContext context(true);
+    network::TestURLLoaderFactory test_url_loader_factory;
+    auto test_shared_url_loader_factory =
+        base::MakeRefCounted<network::WeakWrapperSharedURLLoaderFactory>(
+            &test_url_loader_factory);
 
     std::unique_ptr<DataReductionProxyTestContext> drp_test_context =
         DataReductionProxyTestContext::Builder()
-            .WithURLRequestContext(&context)
+            .WithURLLoaderFactory(test_shared_url_loader_factory)
             .SkipSettingsInitialization()
             .Build();
 
     drp_test_context->DisableWarmupURLFetch();
 
-    net::MockClientSocketFactory mock_socket_factory;
-    context.set_client_socket_factory(&mock_socket_factory);
-    context.Init();
-
     // Start with the Data Reduction Proxy disabled.
     drp_test_context->SetDataReductionProxyEnabled(false);
     drp_test_context->InitSettings();
     drp_test_context->RunUntilIdle();
 
-    net::MockRead mock_reads[] = {
-        net::MockRead(test_case.response_headers),
-        net::MockRead(test_case.response_body),
-        net::MockRead(net::SYNCHRONOUS, test_case.net_error_code),
-    };
-    net::StaticSocketDataProvider socket_data_provider(
-        mock_reads, base::span<net::MockWrite>());
-    mock_socket_factory.AddSocketDataProvider(&socket_data_provider);
-
     // Toggle the pref to trigger the secure proxy check.
     drp_test_context->SetDataReductionProxyEnabled(true);
     drp_test_context->RunUntilIdle();
 
+    network::ResourceResponseHead resource_response_head;
+    std::string headers(test_case.response_headers);
+    resource_response_head.headers = new net::HttpResponseHeaders(
+        net::HttpUtil::AssembleRawHeaders(headers.c_str(), headers.size()));
+    test_url_loader_factory.SimulateResponseWithoutRemovingFromPendingList(
+        test_url_loader_factory.GetPendingRequest(0), resource_response_head,
+        test_case.response_body,
+        network::URLLoaderCompletionStatus(test_case.net_error_code));
+
     if (test_case.expected_restricted) {
       EXPECT_EQ(std::vector<net::ProxyServer>(1, kHttpProxy),
                 drp_test_context->GetConfiguredProxiesForHttp());

components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.cc

@@ -40,10 +40,10 @@
 #include "net/url_request/url_request_intercepting_job_factory.h"
 #include "net/url_request/url_request_job_factory_impl.h"
 #include "net/url_request/url_request_test_util.h"
-#include "services/network/public/cpp/shared_url_loader_factory.h"
+#include "services/network/public/cpp/weak_wrapper_shared_url_loader_factory.h"
 #include "services/network/test/test_network_connection_tracker.h"
 #include "services/network/test/test_network_quality_tracker.h"
-#include "services/network/test/test_shared_url_loader_factory.h"
+#include "services/network/test/test_url_loader_factory.h"
 #include "url/gurl.h"
 
 namespace {
@@ -450,12 +450,10 @@ DataReductionProxyTestContext::Builder::Build() {
         task_runner, std::move(test_request_context));
   }
 
-  if (url_loader_factory_) {
+  // In case no |url_loader_factory_| is specified, an instance will be
+  // created in DataReductionProxyTestContext's ctor.
+  if (url_loader_factory_)
     url_loader_factory = url_loader_factory_;
-  } else {
-    url_loader_factory =
-        base::MakeRefCounted<network::TestSharedURLLoaderFactory>();
-  }
 
   std::unique_ptr<DataReductionProxyConfigurator> configurator(
       new DataReductionProxyConfigurator());
@@ -569,14 +567,24 @@ DataReductionProxyTestContext::DataReductionProxyTestContext(
       task_runner_(task_runner),
       simple_pref_service_(std::move(simple_pref_service)),
       request_context_getter_(request_context_getter),
-      url_loader_factory_(std::move(url_loader_factory)),
       mock_socket_factory_(mock_socket_factory),
       io_data_(std::move(io_data)),
       settings_(std::move(settings)),
       config_storer_(std::move(config_storer)),
       test_network_quality_tracker_(
           std::make_unique<network::TestNetworkQualityTracker>()),
-      params_(params) {}
+      params_(params) {
+  if (url_loader_factory) {
+    test_shared_url_loader_factory_ = std::move(url_loader_factory);
+    return;
+  }
+
+  // To be used for EnableDataReductionProxyWithSecureProxyCheckSuccess.
+  test_url_loader_factory_ = std::make_unique<network::TestURLLoaderFactory>();
+  test_shared_url_loader_factory_ =
+      base::MakeRefCounted<network::WeakWrapperSharedURLLoaderFactory>(
+          test_url_loader_factory_.get());
+}
 
 DataReductionProxyTestContext::~DataReductionProxyTestContext() {
   DestroySettings();
@@ -641,12 +649,12 @@ DataReductionProxyTestContext::CreateDataReductionProxyServiceInternal(
     return std::make_unique<MockDataReductionProxyService>(
         settings, test_network_quality_tracker_.get(),
         simple_pref_service_.get(), request_context_getter_.get(),
-        url_loader_factory_, task_runner_);
+        test_shared_url_loader_factory_, task_runner_);
   }
   return std::make_unique<DataReductionProxyService>(
       settings, simple_pref_service_.get(), request_context_getter_.get(),
-      url_loader_factory_, base::WrapUnique(new TestDataStore()), nullptr,
-      test_network_quality_tracker_.get(), task_runner_, task_runner_,
+      test_shared_url_loader_factory_, base::WrapUnique(new TestDataStore()),
+      nullptr, test_network_quality_tracker_.get(), task_runner_, task_runner_,
       task_runner_, base::TimeDelta());
 }
 
@@ -676,16 +684,14 @@ void DataReductionProxyTestContext::
   // check.
   DCHECK(data_reduction_proxy_service());
 
+  // This method should be called in case DataReductionProxyTestContext is
+  // constructed without a valid SharedURLLoaderFactory instance.
+  DCHECK(test_url_loader_factory_);
+
   // Enable the Data Reduction Proxy, simulating a successful secure proxy
   // check.
-  net::MockRead mock_reads[] = {
-      net::MockRead("HTTP/1.1 200 OK\r\n\r\n"),
-      net::MockRead("OK"),
-      net::MockRead(net::SYNCHRONOUS, net::OK),
-  };
-  net::StaticSocketDataProvider socket_data_provider(
-      mock_reads, base::span<net::MockWrite>());
-  mock_socket_factory_->AddSocketDataProvider(&socket_data_provider);
+  test_url_loader_factory_->AddResponse(params::GetSecureProxyCheckURL().spec(),
+                                        "OK");
 
   // Set the pref to cause the secure proxy check to be issued.
   pref_service()->SetBoolean(kDataReductionProxyEnabled, true);

components/data_reduction_proxy/core/browser/data_reduction_proxy_test_utils.h

@@ -45,6 +45,7 @@ class URLRequestContextStorage;
 namespace network {
 class SharedURLLoaderFactory;
 class TestNetworkQualityTracker;
+class TestURLLoaderFactory;
 }
 
 namespace data_reduction_proxy {
@@ -471,7 +472,7 @@ class DataReductionProxyTestContext {
   }
 
   scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory() const {
-    return url_loader_factory_;
+    return test_shared_url_loader_factory_;
   }
 
   DataReductionProxyBypassStats* bypass_stats() const {
@@ -538,7 +539,9 @@ class DataReductionProxyTestContext {
   scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
   std::unique_ptr<TestingPrefServiceSimple> simple_pref_service_;
   scoped_refptr<net::URLRequestContextGetter> request_context_getter_;
-  scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory_;
+  scoped_refptr<network::SharedURLLoaderFactory>
+      test_shared_url_loader_factory_;
+  std::unique_ptr<network::TestURLLoaderFactory> test_url_loader_factory_;
   // Non-owned pointer. Will be NULL if |this| was built without specifying a
   // |net::MockClientSocketFactory|.
   net::MockClientSocketFactory* mock_socket_factory_;

components/data_reduction_proxy/core/browser/secure_proxy_checker.cc

@@ -11,6 +11,8 @@
 #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/shared_url_loader_factory.h"
+#include "services/network/public/cpp/simple_url_loader.h"
 
 namespace {
 
@@ -23,16 +25,27 @@ const char kUMAProxySecureProxyCheckLatency[] =
 namespace data_reduction_proxy {
 
 SecureProxyChecker::SecureProxyChecker(
-    const scoped_refptr<net::URLRequestContextGetter>&
-        url_request_context_getter)
-    : url_request_context_getter_(url_request_context_getter) {}
-
-void SecureProxyChecker::OnURLFetchComplete(const net::URLFetcher* source) {
-  DCHECK_EQ(source, fetcher_.get());
-  net::URLRequestStatus status = source->GetStatus();
+    scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory)
+    : url_loader_factory_(std::move(url_loader_factory)) {}
 
+void SecureProxyChecker::OnURLLoadComplete(
+    std::unique_ptr<std::string> response_body) {
   std::string response;
-  source->GetResponseAsString(&response);
+  if (response_body)
+    response = std::move(*response_body);
+
+  int response_code = -1;
+  if (url_loader_->ResponseInfo() && url_loader_->ResponseInfo()->headers)
+    response_code = url_loader_->ResponseInfo()->headers->response_code();
+
+  OnURLLoadCompleteOrRedirect(response, url_loader_->NetError(), response_code);
+}
+
+void SecureProxyChecker::OnURLLoadCompleteOrRedirect(
+    const std::string& response,
+    int net_error,
+    int response_code) {
+  url_loader_.reset();
 
   base::TimeDelta secure_proxy_check_latency =
       base::Time::Now() - secure_proxy_check_start_time_;
@@ -41,7 +54,15 @@ void SecureProxyChecker::OnURLFetchComplete(const net::URLFetcher* source) {
                                secure_proxy_check_latency);
   }
 
-  fetcher_callback_.Run(response, status, source->GetResponseCode());
+  fetcher_callback_.Run(response, net_error, response_code);
+}
+
+void SecureProxyChecker::OnURLLoaderRedirect(
+    const net::RedirectInfo& redirect_info,
+    const network::ResourceResponseHead& response_head,
+    std::vector<std::string>* to_be_removed_headers) {
+  OnURLLoadCompleteOrRedirect(std::string(), net::ERR_ABORTED,
+                              redirect_info.status_code);
 }
 
 void SecureProxyChecker::CheckIfSecureProxyIsAllowed(
@@ -70,32 +91,36 @@ void SecureProxyChecker::CheckIfSecureProxyIsAllowed(
                 "it is enabled by installing the Data Saver extension."
               policy_exception_justification: "Not implemented."
             })");
-  fetcher_ =
-      net::URLFetcher::Create(params::GetSecureProxyCheckURL(),
-                              net::URLFetcher::GET, this, traffic_annotation);
-  data_use_measurement::DataUseUserData::AttachToFetcher(
-      fetcher_.get(),
-      data_use_measurement::DataUseUserData::DATA_REDUCTION_PROXY);
-  fetcher_->SetLoadFlags(net::LOAD_DISABLE_CACHE | net::LOAD_BYPASS_PROXY |
-                         net::LOAD_DO_NOT_SEND_COOKIES |
-                         net::LOAD_DO_NOT_SAVE_COOKIES);
-  fetcher_->SetRequestContext(url_request_context_getter_.get());
+  auto resource_request = std::make_unique<network::ResourceRequest>();
+  resource_request->url = params::GetSecureProxyCheckURL();
+  resource_request->load_flags =
+      net::LOAD_DISABLE_CACHE | net::LOAD_BYPASS_PROXY |
+      net::LOAD_DO_NOT_SEND_COOKIES | net::LOAD_DO_NOT_SAVE_COOKIES;
+  url_loader_ = network::SimpleURLLoader::Create(std::move(resource_request),
+                                                 traffic_annotation);
+
   // Configure max retries to be at most kMaxRetries times for 5xx errors.
   static const int kMaxRetries = 5;
-  fetcher_->SetMaxRetriesOn5xx(kMaxRetries);
-  fetcher_->SetAutomaticallyRetryOnNetworkChanges(kMaxRetries);
+  url_loader_->SetRetryOptions(
+      kMaxRetries, network::SimpleURLLoader::RETRY_ON_NETWORK_CHANGE |
+                       network::SimpleURLLoader::RETRY_ON_5XX);
+  // Hook the redirect callback so we can cancel the request.
   // The secure proxy check should not be redirected. Since the secure proxy
   // check will inevitably fail if it gets redirected somewhere else (e.g. by
   // a captive portal), short circuit that by giving up on the secure proxy
   // check if it gets redirected.
-  fetcher_->SetStopOnRedirect(true);
+  url_loader_->SetOnRedirectCallback(base::BindRepeating(
+      &SecureProxyChecker::OnURLLoaderRedirect, base::Unretained(this)));
 
   fetcher_callback_ = fetcher_callback;
-
   secure_proxy_check_start_time_ = base::Time::Now();
-  fetcher_->Start();
+
+  url_loader_->DownloadToStringOfUnboundedSizeUntilCrashAndDie(
+      url_loader_factory_.get(),
+      base::BindOnce(&SecureProxyChecker::OnURLLoadComplete,
+                     base::Unretained(this)));
 }
 
 SecureProxyChecker::~SecureProxyChecker() {}
 
-}  // namespace data_reduction_proxy
\ No newline at end of file
+}  // namespace data_reduction_proxy

components/data_reduction_proxy/core/browser/secure_proxy_checker.h

@@ -11,39 +11,47 @@
 #include "base/callback.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
-#include "net/url_request/url_fetcher_delegate.h"
 #include "net/url_request/url_request_status.h"
 
 namespace net {
-
-class URLFetcher;
-class URLRequestContextGetter;
-
+struct RedirectInfo;
 }  // namespace net
 
+namespace network {
+class SharedURLLoaderFactory;
+class SimpleURLLoader;
+struct ResourceResponseHead;
+}  // namespace network
+
 namespace data_reduction_proxy {
 
-typedef base::Callback<
-    void(const std::string&, const net::URLRequestStatus&, int)>
+typedef base::RepeatingCallback<void(const std::string&, int, int)>
     SecureProxyCheckerCallback;
 
 // Checks if the secure proxy is allowed by the carrier by sending a probe.
-class SecureProxyChecker : public net::URLFetcherDelegate {
+class SecureProxyChecker {
  public:
-  explicit SecureProxyChecker(const scoped_refptr<net::URLRequestContextGetter>&
-                                  url_request_context_getter);
+  explicit SecureProxyChecker(
+      scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory);
 
-  ~SecureProxyChecker() override;
+  virtual ~SecureProxyChecker();
 
   void CheckIfSecureProxyIsAllowed(SecureProxyCheckerCallback fetcher_callback);
 
  private:
-  void OnURLFetchComplete(const net::URLFetcher* source) override;
+  void OnURLLoadComplete(std::unique_ptr<std::string> response_body);
+  void OnURLLoadCompleteOrRedirect(const std::string& response,
+                                   int net_error,
+                                   int response_code);
+
+  void OnURLLoaderRedirect(const net::RedirectInfo& redirect_info,
+                           const network::ResourceResponseHead& response_head,
+                           std::vector<std::string>* to_be_removed_headers);
 
-  scoped_refptr<net::URLRequestContextGetter> url_request_context_getter_;
+  scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory_;
 
-  // The URLFetcher being used for the secure proxy check.
-  std::unique_ptr<net::URLFetcher> fetcher_;
+  // The URLLoader being used for the secure proxy check.
+  std::unique_ptr<network::SimpleURLLoader> url_loader_;
   SecureProxyCheckerCallback fetcher_callback_;
 
   // Used to determine the latency in performing the Data Reduction Proxy secure
@@ -55,4 +63,4 @@ class SecureProxyChecker : public net::URLFetcherDelegate {
 
 }  // namespace data_reduction_proxy
 
-#endif  // COMPONENTS_DATA_REDUCTION_PROXY_CORE_BROWSER_SECURE_PROXY_CHECKER_H_
\ No newline at end of file
+#endif  // COMPONENTS_DATA_REDUCTION_PROXY_CORE_BROWSER_SECURE_PROXY_CHECKER_H_