Migrate AutofillDownloadManager to SimpleURLLoader

This CL migrates AutofillDownloadManager away from URLFetcher, to
SimpleURLLoader.

As a notorious remark, the CL introduces a public method to
SimpleURLLoader, LoadedFromCache. It allows to verify whether
some specific content are served from the cache, as expected.

BUG=773295,844929

Cq-Include-Trybots: luci.chromium.try:linux_mojo
Change-Id: I92a36bc032b7c0e182ae83f367eefd605ad888ca
Reviewed-on: https://chromium-review.googlesource.com/1114998Reviewed-by: Matt Menke <mmenke@chromium.org>
Reviewed-by: Roger McFarlane <rogerm@chromium.org>
Commit-Queue: Antonio Gomes <tonikitoo@igalia.com>
Cr-Commit-Position: refs/heads/master@{#575269}

chrome/browser/autofill/autofill_server_browsertest.cc

@@ -20,7 +20,8 @@
 #include "components/autofill/core/browser/personal_data_manager_observer.h"
 #include "content/public/test/browser_test_utils.h"
 #include "content/public/test/test_utils.h"
-#include "net/url_request/test_url_fetcher_factory.h"
+#include "content/public/test/url_loader_interceptor.h"
+#include "services/network/test/test_utils.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "url/third_party/mozilla/url_parse.h"
 
@@ -52,27 +53,28 @@ class WindowedPersonalDataManagerObserver : public PersonalDataManagerObserver {
   scoped_refptr<content::MessageLoopRunner> message_loop_runner_;
 };
 
-class WindowedNetworkObserver : public net::TestURLFetcher::DelegateForTests {
+class WindowedNetworkObserver {
  public:
   explicit WindowedNetworkObserver(const std::string& expected_upload_data)
-      : factory_(new net::TestURLFetcherFactory),
-        expected_upload_data_(expected_upload_data),
+      : expected_upload_data_(expected_upload_data),
         message_loop_runner_(new content::MessageLoopRunner) {
-    factory_->SetDelegateForTests(this);
+    interceptor_ =
+        std::make_unique<content::URLLoaderInterceptor>(base::BindRepeating(
+            &WindowedNetworkObserver::OnIntercept, base::Unretained(this)));
   }
   ~WindowedNetworkObserver() {}
 
   // Waits for a network request with the |expected_upload_data_|.
   void Wait() {
     message_loop_runner_->Run();
-    factory_.reset();
+    interceptor_.reset();
   }
 
+ private:
   // Helper to extract the value of a query param. Returns "*** not found ***"
   // if the requested query param is not in the query string.
-  std::string GetQueryParam(const net::TestURLFetcher& fetcher,
+  std::string GetQueryParam(const std::string& query_str,
                             const std::string& param_name) {
-    std::string query_str = fetcher.GetOriginalURL().query();
     url::Component query(0, query_str.length());
     url::Component key, value;
     while (url::ExtractQueryKeyValue(query_str.c_str(), &query, &key, &value)) {
@@ -89,26 +91,32 @@ class WindowedNetworkObserver : public net::TestURLFetcher::DelegateForTests {
     return "*** not found ***";
   }
 
-  // net::TestURLFetcher::DelegateForTests:
-  void OnRequestStart(int fetcher_id) override {
-    net::TestURLFetcher* fetcher = factory_->GetFetcherByID(fetcher_id);
-    if (fetcher->upload_data() == expected_upload_data_ ||
-        GetQueryParam(*fetcher, "q") == expected_upload_data_) {
+  bool OnIntercept(content::URLLoaderInterceptor::RequestParams* params) {
+    // NOTE: This constant matches the one defined in
+    // components/autofill/core/browser/autofill_download_manager.cc
+    static const char kDefaultAutofillServerURL[] =
+        "https://clients1.google.com/tbproxy/af/";
+    DCHECK(params);
+    network::ResourceRequest resource_request = params->url_request;
+    if (resource_request.url.spec().find(kDefaultAutofillServerURL) ==
+        std::string::npos) {
+      return false;
+    }
+
+    if (network::GetUploadData(resource_request) == expected_upload_data_ ||
+        GetQueryParam(resource_request.url.query(), "q") ==
+            expected_upload_data_) {
       message_loop_runner_->Quit();
     }
-    // Not interested in any further status updates from this fetcher.
-    fetcher->SetDelegateForTests(nullptr);
+    return false;
   }
-  void OnChunkUpload(int fetcher_id) override {}
-  void OnRequestEnd(int fetcher_id) override {}
 
  private:
-  // Mocks out network requests.
-  std::unique_ptr<net::TestURLFetcherFactory> factory_;
-
   const std::string expected_upload_data_;
   scoped_refptr<content::MessageLoopRunner> message_loop_runner_;
 
+  std::unique_ptr<content::URLLoaderInterceptor> interceptor_;
+
   DISALLOW_COPY_AND_ASSIGN(WindowedNetworkObserver);
 };
 

components/autofill/core/browser/DEPS

@@ -36,6 +36,9 @@ include_rules = [
 ]
 
 specific_include_rules = {
+  "autofill_download_manager_unittest\.cc": [
+    "+services/network/test",
+  ],
   "autofill_manager_unittest\.cc": [
     "+components/ukm",
   ],

components/autofill/core/browser/autofill_download_manager.cc

@@ -34,6 +34,8 @@
 #include "net/http/http_status_code.h"
 #include "net/http/http_util.h"
 #include "net/traffic_annotation/network_traffic_annotation.h"
+#include "services/network/public/cpp/resource_request.h"
+#include "services/network/public/cpp/shared_url_loader_factory.h"
 
 namespace autofill {
 
@@ -285,8 +287,7 @@ AutofillDownloadManager::AutofillDownloadManager(AutofillDriver* driver,
       observer_(observer),
       autofill_server_url_(GetAutofillServerURL()),
       max_form_cache_size_(kMaxFormCacheSize),
-      fetcher_backoff_(&kAutofillBackoffPolicy),
-      fetcher_id_for_unittest_(0),
+      loader_backoff_(&kAutofillBackoffPolicy),
       weak_factory_(this) {
   DCHECK(observer_);
 }
@@ -362,24 +363,22 @@ bool AutofillDownloadManager::StartUploadRequest(
   return StartRequest(std::move(request_data));
 }
 
-std::tuple<GURL, net::URLFetcher::RequestType>
-AutofillDownloadManager::GetRequestURLAndMethod(
+std::tuple<GURL, std::string> AutofillDownloadManager::GetRequestURLAndMethod(
     const FormRequestData& request_data) const {
-  net::URLFetcher::RequestType method = net::URLFetcher::POST;
+  std::string method("POST");
   std::string query_str;
 
   if (request_data.request_type == AutofillDownloadManager::REQUEST_QUERY) {
     if (request_data.payload.length() <= kMaxQueryGetSize &&
         base::FeatureList::IsEnabled(features::kAutofillCacheQueryResponses)) {
-      method = net::URLFetcher::GET;
+      method = "GET";
       std::string base64_payload;
       base::Base64UrlEncode(request_data.payload,
                             base::Base64UrlEncodePolicy::INCLUDE_PADDING,
                             &base64_payload);
       base::StrAppend(&query_str, {"q=", base64_payload});
     }
-    UMA_HISTOGRAM_BOOLEAN("Autofill.Query.Method",
-                          (method == net::URLFetcher::GET) ? 0 : 1);
+    UMA_HISTOGRAM_BOOLEAN("Autofill.Query.Method", (method == "GET") ? 0 : 1);
   }
 
   GURL::Replacements replacements;
@@ -388,52 +387,49 @@ AutofillDownloadManager::GetRequestURLAndMethod(
   GURL url = autofill_server_url_
                  .Resolve(RequestTypeToString(request_data.request_type))
                  .ReplaceComponents(replacements);
-  return std::make_tuple(std::move(url), method);
+  return std::make_tuple(std::move(url), std::move(method));
 }
 
 bool AutofillDownloadManager::StartRequest(FormRequestData request_data) {
-  net::URLRequestContextGetter* request_context =
-      driver_->GetURLRequestContext();
-  DCHECK(request_context);
+  scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory =
+      driver_->GetURLLoaderFactory();
+  DCHECK(url_loader_factory);
 
   // Get the URL and method to use for this request.
-  net::URLFetcher::RequestType method;
+  std::string method;
   GURL request_url;
   std::tie(request_url, method) = GetRequestURLAndMethod(request_data);
 
-  // Id is ignored for regular chrome, in unit test id's for fake fetcher
-  // factory will be 0, 1, 2, ...
-  std::unique_ptr<net::URLFetcher> fetcher = net::URLFetcher::Create(
-      fetcher_id_for_unittest_++, request_url, method, this,
-      GetNetworkTrafficAnnotation(request_data.request_type));
-
-  data_use_measurement::DataUseUserData::AttachToFetcher(
-      fetcher.get(), data_use_measurement::DataUseUserData::AUTOFILL);
-  fetcher->SetAutomaticallyRetryOn5xx(false);
-  fetcher->SetRequestContext(request_context);
-  fetcher->SetLoadFlags(net::LOAD_DO_NOT_SAVE_COOKIES |
-                        net::LOAD_DO_NOT_SEND_COOKIES);
-  if (method == net::URLFetcher::POST) {
-    fetcher->SetUploadData("text/proto", request_data.payload);
-  }
+  auto resource_request = std::make_unique<network::ResourceRequest>();
+  resource_request->url = request_url;
+  resource_request->load_flags =
+      net::LOAD_DO_NOT_SEND_COOKIES | net::LOAD_DO_NOT_SAVE_COOKIES;
+  resource_request->method = method;
 
   // Add Chrome experiment state to the request headers.
-  net::HttpRequestHeaders headers;
   variations::AppendVariationHeadersUnknownSignedIn(
-      fetcher->GetOriginalURL(),
+      request_url,
       driver_->IsIncognito() ? variations::InIncognito::kYes
                              : variations::InIncognito::kNo,
-      &headers);
-  fetcher->SetExtraRequestHeaders(headers.ToString());
+      &resource_request->headers);
 
-  // Transfer ownership of the fetcher into url_fetchers_. Temporarily hang
-  // onto the raw pointer to use it as a key and to kick off the request;
-  // transferring ownership (std::move) invalidates the |fetcher| variable.
-  auto* raw_fetcher = fetcher.get();
-  url_fetchers_[raw_fetcher] =
-      std::make_pair(std::move(fetcher), std::move(request_data));
-  raw_fetcher->Start();
+  auto simple_loader = network::SimpleURLLoader::Create(
+      std::move(resource_request),
+      GetNetworkTrafficAnnotation(request_data.request_type));
+  if (method == "POST")
+    simple_loader->AttachStringForUpload(request_data.payload, "text/proto");
 
+  // Transfer ownership of the loader into url_loaders_. Temporarily hang
+  // onto the raw pointer to use it as a key and to kick off the request;
+  // transferring ownership (std::move) invalidates the |simple_loader|
+  // variable.
+  auto* raw_simple_loader = simple_loader.get();
+  url_loaders_.push_back(std::move(simple_loader));
+  raw_simple_loader->DownloadToStringOfUnboundedSizeUntilCrashAndDie(
+      url_loader_factory.get(),
+      base::BindOnce(&AutofillDownloadManager::OnSimpleLoaderComplete,
+                     base::Unretained(this), std::move(--url_loaders_.end()),
+                     std::move(request_data)));
   return true;
 }
 
@@ -489,24 +485,22 @@ std::string AutofillDownloadManager::GetCombinedSignature(
   return signature;
 }
 
-void AutofillDownloadManager::OnURLFetchComplete(
-    const net::URLFetcher* source) {
-  auto it = url_fetchers_.find(const_cast<net::URLFetcher*>(source));
-  if (it == url_fetchers_.end()) {
-    // Looks like crash on Mac is possibly caused with callback entering here
-    // with unknown fetcher when network is refreshed.
-    return;
-  }
-
-  // Move the fetcher and request out of the active fetchers list.
-  std::unique_ptr<net::URLFetcher> fetcher = std::move(it->second.first);
-  FormRequestData request_data = std::move(it->second.second);
-  url_fetchers_.erase(it);
+void AutofillDownloadManager::OnSimpleLoaderComplete(
+    std::list<std::unique_ptr<network::SimpleURLLoader>>::iterator it,
+    FormRequestData request_data,
+    std::unique_ptr<std::string> response_body) {
+  // Move the loader out of the active loaders list.
+  std::unique_ptr<network::SimpleURLLoader> simple_loader = std::move(*it);
+  url_loaders_.erase(it);
 
   CHECK(request_data.form_signatures.size());
-  const int response_code = fetcher->GetResponseCode();
-  const bool success = (response_code == net::HTTP_OK);
-  fetcher_backoff_.InformOfRequest(success);
+  int response_code = -1;
+  if (simple_loader->ResponseInfo() && simple_loader->ResponseInfo()->headers) {
+    response_code = simple_loader->ResponseInfo()->headers->response_code();
+  }
+
+  const bool success = !!response_body;
+  loader_backoff_.InformOfRequest(success);
 
   LogHttpResponseCode(request_data.request_type, response_code);
 
@@ -532,16 +526,15 @@ void AutofillDownloadManager::OnURLFetchComplete(
         base::BindOnce(
             base::IgnoreResult(&AutofillDownloadManager::StartRequest),
             weak_factory_.GetWeakPtr(), std::move(request_data)),
-        fetcher_backoff_.GetTimeUntilRelease());
+        loader_backoff_.GetTimeUntilRelease());
     return;
   }
 
   if (request_data.request_type == AutofillDownloadManager::REQUEST_QUERY) {
-    std::string response_body;
-    fetcher->GetResponseAsString(&response_body);
-    CacheQueryRequest(request_data.form_signatures, response_body);
-    UMA_HISTOGRAM_BOOLEAN("Autofill.Query.WasInCache", fetcher->WasCached());
-    observer_->OnLoadedServerPredictions(std::move(response_body),
+    CacheQueryRequest(request_data.form_signatures, *response_body);
+    UMA_HISTOGRAM_BOOLEAN("Autofill.Query.WasInCache",
+                          simple_loader->LoadedFromCache());
+    observer_->OnLoadedServerPredictions(std::move(*response_body),
                                          request_data.form_signatures);
     return;
   }

components/autofill/core/browser/autofill_download_manager.h

@@ -20,8 +20,7 @@
 #include "base/time/time.h"
 #include "components/autofill/core/browser/autofill_type.h"
 #include "net/base/backoff_entry.h"
-#include "net/url_request/url_fetcher.h"
-#include "net/url_request/url_fetcher_delegate.h"
+#include "services/network/public/cpp/simple_url_loader.h"
 #include "url/gurl.h"
 
 namespace autofill {
@@ -30,7 +29,7 @@ class AutofillDriver;
 class FormStructure;
 
 // Handles getting and updating Autofill heuristics.
-class AutofillDownloadManager : public net::URLFetcherDelegate {
+class AutofillDownloadManager {
  public:
   enum RequestType { REQUEST_QUERY, REQUEST_UPLOAD, };
 
@@ -64,7 +63,7 @@ class AutofillDownloadManager : public net::URLFetcherDelegate {
   // |observer| - observer to notify on successful completion or error.
   AutofillDownloadManager(AutofillDriver* driver,
                           Observer* observer);
-  ~AutofillDownloadManager() override;
+  virtual ~AutofillDownloadManager();
 
   // Starts a query request to Autofill servers. The observer is called with the
   // list of the fields of all requested forms.
@@ -102,7 +101,7 @@ class AutofillDownloadManager : public net::URLFetcherDelegate {
   // described by |request_data|. If the returned method is GET, the URL
   // fully encompasses the request, do not include request_data.payload when
   // transmitting the request.
-  std::tuple<GURL, net::URLFetcher::RequestType> GetRequestURLAndMethod(
+  std::tuple<GURL, std::string> GetRequestURLAndMethod(
       const FormRequestData& request_data) const;
 
   // Initiates request to Autofill servers to download/upload type predictions.
@@ -130,8 +129,10 @@ class AutofillDownloadManager : public net::URLFetcherDelegate {
   std::string GetCombinedSignature(
       const std::vector<std::string>& forms_in_query) const;
 
-  // net::URLFetcherDelegate implementation:
-  void OnURLFetchComplete(const net::URLFetcher* source) override;
+  void OnSimpleLoaderComplete(
+      std::list<std::unique_ptr<network::SimpleURLLoader>>::iterator it,
+      FormRequestData request_data,
+      std::unique_ptr<std::string> response_body);
 
   // The AutofillDriver that this instance will use. Must not be null, and must
   // outlive this instance.
@@ -145,23 +146,15 @@ class AutofillDownloadManager : public net::URLFetcherDelegate {
   // final path component for the request and the query params.
   GURL autofill_server_url_;
 
-  // For each requested form for both query and upload we create a separate
-  // request and save its info. As url fetcher is identified by its address
-  // we use a map between fetchers and info. The value type is a pair of an
-  // owning pointer to the key and the actual FormRequestData.
-  std::map<net::URLFetcher*,
-           std::pair<std::unique_ptr<net::URLFetcher>, FormRequestData>>
-      url_fetchers_;
+  // Loaders used for the processing the requests. Invalidated after completion.
+  std::list<std::unique_ptr<network::SimpleURLLoader>> url_loaders_;
 
   // Cached QUERY requests.
   QueryRequestCache cached_forms_;
   size_t max_form_cache_size_;
 
   // Used for exponential backoff of requests.
-  net::BackoffEntry fetcher_backoff_;
-
-  // Needed for unit-test.
-  int fetcher_id_for_unittest_;
+  net::BackoffEntry loader_backoff_;
 
   base::WeakPtrFactory<AutofillDownloadManager> weak_factory_;
 };

components/autofill/core/browser/test_autofill_driver.cc

@@ -95,4 +95,9 @@ void TestAutofillDriver::SetURLRequestContext(
   url_request_context_ = url_request_context;
 }
 
+void TestAutofillDriver::SetSharedURLLoaderFactory(
+    scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory) {
+  test_shared_loader_factory_ = url_loader_factory;
+}
+
 }  // namespace autofill

components/autofill/core/browser/test_autofill_driver.h

@@ -57,6 +57,8 @@ class TestAutofillDriver : public AutofillDriver {
   // Sets the URL request context for this instance. |url_request_context|
   // should outlive this instance.
   void SetURLRequestContext(net::URLRequestContextGetter* url_request_context);
+  void SetSharedURLLoaderFactory(
+      scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory);
 
  private:
   net::URLRequestContextGetter* url_request_context_;

services/network/public/cpp/simple_url_loader.cc

@@ -214,6 +214,7 @@ class SimpleURLLoaderImpl : public SimpleURLLoader,
   int NetError() const override;
   const ResourceResponseHead* ResponseInfo() const override;
   const GURL& GetFinalURL() const override;
+  bool LoadedFromCache() const override;
 
   // Called by BodyHandler when the BodyHandler body handler is done. If |error|
   // is not net::OK, some error occurred reading or consuming the body. If it is
@@ -253,6 +254,8 @@ class SimpleURLLoaderImpl : public SimpleURLLoader,
     // Result of the request.
     int net_error = net::ERR_IO_PENDING;
 
+    bool loaded_from_cache = false;
+
     std::unique_ptr<ResourceResponseHead> response_info;
   };
 
@@ -1194,6 +1197,12 @@ const GURL& SimpleURLLoaderImpl::GetFinalURL() const {
   return final_url_;
 }
 
+bool SimpleURLLoaderImpl::LoadedFromCache() const {
+  // Should only be called once the request is compelete.
+  DCHECK(request_state_->finished);
+  return request_state_->loaded_from_cache;
+}
+
 const ResourceResponseHead* SimpleURLLoaderImpl::ResponseInfo() const {
   // Should only be called once the request is compelete.
   DCHECK(request_state_->finished);
@@ -1410,6 +1419,7 @@ void SimpleURLLoaderImpl::OnComplete(const URLLoaderCompletionStatus& status) {
   request_state_->request_completed = true;
   request_state_->expected_body_size = status.decoded_body_length;
   request_state_->net_error = status.error_code;
+  request_state_->loaded_from_cache = status.exists_in_cache;
   // If |status| indicates success, but the body pipe was never received, the
   // URLLoader is violating the API contract.
   if (request_state_->net_error == net::OK && !request_state_->body_started)

services/network/public/cpp/simple_url_loader.h

@@ -270,6 +270,11 @@ class COMPONENT_EXPORT(NETWORK_CPP) SimpleURLLoader {
   // loader has informed the caller of completion.
   virtual const GURL& GetFinalURL() const = 0;
 
+  // Indicates the request that this loader is processing was loaded from the
+  // HTTP cache. May only be called once the loader has informed the caller of
+  // completion.
+  virtual bool LoadedFromCache() const = 0;
+
  protected:
   SimpleURLLoader();