S13n: Port ChromeOmniboxNavigationObserver to SimpleURLLoader

URLFetcher will stop working with advent of Network Service, and
SimpleURLLoader is the replacement API for most clients.


Bug: 840378
Cq-Include-Trybots: luci.chromium.try:linux_mojo
Change-Id: Ie318335f2d3a21cac1042862fd6a49e62cc931a4
Reviewed-on: https://chromium-review.googlesource.com/1124771
Commit-Queue: Maks Orlovich <morlovich@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#575900}

chrome/browser/ui/omnibox/chrome_omnibox_navigation_observer.cc

@@ -25,8 +25,9 @@
 #include "net/base/load_flags.h"
 #include "net/base/registry_controlled_domains/registry_controlled_domain.h"
 #include "net/traffic_annotation/network_traffic_annotation.h"
-#include "net/url_request/url_fetcher.h"
-#include "net/url_request/url_request.h"
+#include "services/network/public/cpp/resource_request.h"
+#include "services/network/public/cpp/shared_url_loader_factory.h"
+#include "services/network/public/cpp/simple_url_loader.h"
 #include "url/gurl.h"
 #include "url/url_constants.h"
 
@@ -81,11 +82,10 @@ ChromeOmniboxNavigationObserver::ChromeOmniboxNavigationObserver(
       alternate_nav_match_(alternate_nav_match),
       template_url_service_(TemplateURLServiceFactory::GetForProfile(profile)),
       shortcuts_backend_(ShortcutsBackendFactory::GetForProfile(profile)),
-      request_context_(nullptr),
       load_state_(LOAD_NOT_SEEN),
       fetch_state_(FETCH_NOT_COMPLETE) {
   if (alternate_nav_match_.destination_url.is_valid())
-    CreateFetcher(alternate_nav_match_.destination_url);
+    CreateLoader(alternate_nav_match_.destination_url);
 
   // We need to start by listening to AllSources, since we don't know which tab
   // the navigation might occur in.
@@ -117,6 +117,11 @@ void ChromeOmniboxNavigationObserver::On404() {
   template_url_service_->Remove(template_url);
 }
 
+void ChromeOmniboxNavigationObserver::SetURLLoaderFactoryForTesting(
+    scoped_refptr<network::SharedURLLoaderFactory> testing_loader_factory) {
+  loader_factory_for_testing_ = std::move(testing_loader_factory);
+}
+
 void ChromeOmniboxNavigationObserver::CreateAlternateNavInfoBar() {
   AlternateNavInfoBarDelegate::Create(
       web_contents(), text_, alternate_nav_match_, match_.destination_url);
@@ -169,13 +174,22 @@ void ChromeOmniboxNavigationObserver::Observe(
   load_state_ = LOAD_PENDING;
   WebContentsObserver::Observe(web_contents);
 
-  // Start the alternate nav fetcher if need be.
-  if (fetcher_) {
-    request_context_ = content::BrowserContext::GetDefaultStoragePartition(
+  // Start the alternate nav loader if need be.
+  if (loader_) {
+    network::mojom::URLLoaderFactory* loader_factory = nullptr;
+    if (loader_factory_for_testing_) {
+      loader_factory = loader_factory_for_testing_.get();
+    } else {
+      loader_factory = content::BrowserContext::GetDefaultStoragePartition(
                            controller->GetBrowserContext())
-                           ->GetURLRequestContext();
-    fetcher_->SetRequestContext(request_context_);
-    fetcher_->Start();
+                           ->GetURLLoaderFactoryForBrowserProcess()
+                           .get();
+    }
+    loader_->DownloadToString(
+        loader_factory,
+        base::BindOnce(&ChromeOmniboxNavigationObserver::OnURLLoadComplete,
+                       base::Unretained(this)),
+        1u /* max_body_size */);
   }
 }
 
@@ -196,7 +210,7 @@ void ChromeOmniboxNavigationObserver::NavigationEntryCommitted(
     OnSuccessfulNavigation();
   if (load_details.http_status_code == 404)
     On404();
-  if (!fetcher_ || (fetch_state_ != FETCH_NOT_COMPLETE))
+  if (!loader_ || (fetch_state_ != FETCH_NOT_COMPLETE))
     OnAllLoadingFinished();  // deletes |this|!
 }
 
@@ -204,15 +218,12 @@ void ChromeOmniboxNavigationObserver::WebContentsDestroyed() {
   delete this;
 }
 
-void ChromeOmniboxNavigationObserver::OnURLFetchComplete(
-    const net::URLFetcher* source) {
-  DCHECK_EQ(fetcher_.get(), source);
-  const net::URLRequestStatus& status = source->GetStatus();
-  int response_code = source->GetResponseCode();
-  bool valid_redirect =
-      (status.status() == net::URLRequestStatus::CANCELED) &&
-      ((response_code / 100) == 3) &&
-      IsValidNavigation(alternate_nav_match_.destination_url, source->GetURL());
+void ChromeOmniboxNavigationObserver::OnURLRedirect(
+    const net::RedirectInfo& redirect_info,
+    const network::ResourceResponseHead& response_head,
+    std::vector<std::string>* to_be_removed_headers) {
+  bool valid_redirect = IsValidNavigation(alternate_nav_match_.destination_url,
+                                          redirect_info.new_url);
   // If this is a valid redirect (not hijacked), and the redirect is from
   // http->https (no other changes), then follow it instead of assuming the
   // destination is valid.  This fixes several cases when the infobar appears
@@ -229,17 +240,41 @@ void ChromeOmniboxNavigationObserver::OnURLFetchComplete(
   //   requests for local sites.
   if (valid_redirect &&
       OnlyChangeIsFromHTTPToHTTPS(alternate_nav_match_.destination_url,
-                                  source->GetURL())) {
-    CreateFetcher(source->GetURL());
-    fetcher_->SetRequestContext(request_context_);
-    fetcher_->Start();
+                                  redirect_info.new_url)) {
     return;
   }
-  fetch_state_ =
-      ((status.is_success() && ResponseCodeIndicatesSuccess(response_code)) ||
-       valid_redirect)
-          ? FETCH_SUCCEEDED
-          : FETCH_FAILED;
+
+  // Otherwise report results based on whether the redirect itself is valid.
+  // OnDoneWithURL() will also stop the redirect from being followed since it
+  // destroys |*loader_|.
+  //
+  // We stop-on-redirect here for a couple of reasons:
+  // * Sites with lots of redirects, especially through slow machines, take time
+  //   to follow the redirects.  This delays the appearance of the infobar,
+  //   sometimes by several seconds, which feels really broken.
+  // * Some servers behind redirects respond to HEAD with an error and GET with
+  //   a valid response, in violation of the HTTP spec.  Stop-on-redirects
+  //   reduces the number of cases where this error makes us believe there was
+  //   no server.
+  OnDoneWithURL(valid_redirect);
+}
+
+void ChromeOmniboxNavigationObserver::OnURLLoadComplete(
+    std::unique_ptr<std::string> body) {
+  int response_code = -1;
+  if (loader_->ResponseInfo() && loader_->ResponseInfo()->headers)
+    response_code = loader_->ResponseInfo()->headers->response_code();
+  // We may see ERR_INSUFFICIENT_RESOURCES here even if everything is workable
+  // if the server includes a body in response to a HEAD, as a size limit was
+  // set while fetching.
+  bool fetch_likely_ok = loader_->NetError() == net::OK ||
+                         loader_->NetError() == net::ERR_INSUFFICIENT_RESOURCES;
+  OnDoneWithURL(fetch_likely_ok && ResponseCodeIndicatesSuccess(response_code));
+}
+
+void ChromeOmniboxNavigationObserver::OnDoneWithURL(bool success) {
+  loader_ = nullptr;
+  fetch_state_ = success ? FETCH_SUCCEEDED : FETCH_FAILED;
   if (load_state_ == LOAD_COMMITTED)
     OnAllLoadingFinished();  // deletes |this|!
 }
@@ -250,7 +285,7 @@ void ChromeOmniboxNavigationObserver::OnAllLoadingFinished() {
   delete this;
 }
 
-void ChromeOmniboxNavigationObserver::CreateFetcher(
+void ChromeOmniboxNavigationObserver::CreateLoader(
     const GURL& destination_url) {
   net::NetworkTrafficAnnotationTag traffic_annotation =
       net::DefineNetworkTrafficAnnotation("omnibox_navigation_observer", R"(
@@ -282,17 +317,13 @@ void ChromeOmniboxNavigationObserver::CreateFetcher(
             "this. More fine-grained policies are requested to be "
             "implemented (crbug.com/81226)."
         })");
-  fetcher_ = net::URLFetcher::Create(destination_url, net::URLFetcher::HEAD,
-                                     this, traffic_annotation);
-  fetcher_->SetLoadFlags(net::LOAD_DO_NOT_SAVE_COOKIES);
-  // We stop-on-redirect for a couple of reasons:
-  // * Sites with lots of redirects, especially through slow machines, take time
-  //   to follow the redirects.  This delays the appearance of the infobar,
-  //   sometimes by several seconds, which feels really broken.
-  // * Some servers behind redirects respond to HEAD with an error and GET with
-  //   a valid response, in violation of the HTTP spec.  Stop-on-redirects
-  //   reduces the number of cases where this error makes us believe there was
-  //   no server.
-  // We do allow a certain kind of redirect; see code in OnURLFetchComplete().
-  fetcher_->SetStopOnRedirect(true);
+  auto request = std::make_unique<network::ResourceRequest>();
+  request->url = destination_url;
+  request->method = "HEAD";
+  request->load_flags = net::LOAD_DO_NOT_SAVE_COOKIES;
+  loader_ =
+      network::SimpleURLLoader::Create(std::move(request), traffic_annotation);
+  loader_->SetAllowHttpErrorResults(true);
+  loader_->SetOnRedirectCallback(base::BindRepeating(
+      &ChromeOmniboxNavigationObserver::OnURLRedirect, base::Unretained(this)));
 }

chrome/browser/ui/omnibox/chrome_omnibox_navigation_observer.h

@@ -23,9 +23,9 @@ class Profile;
 class ShortcutsBackend;
 class TemplateURLService;
 
-namespace net {
-class URLFetcher;
-class URLRequestContextGetter;
+namespace network {
+class SharedURLLoaderFactory;
+class SimpleURLLoader;
 }
 
 // Monitors omnibox navigations in order to trigger behaviors that depend on
@@ -46,8 +46,7 @@ class URLRequestContextGetter;
 // about the memory management of this object.
 class ChromeOmniboxNavigationObserver : public OmniboxNavigationObserver,
                                         public content::NotificationObserver,
-                                        public content::WebContentsObserver,
-                                        public net::URLFetcherDelegate {
+                                        public content::WebContentsObserver {
  public:
   enum LoadState {
     LOAD_NOT_SEEN,
@@ -74,6 +73,11 @@ class ChromeOmniboxNavigationObserver : public OmniboxNavigationObserver,
   // the broken engine again.
   void On404();
 
+  // Test-only method to override how loading happens. Normally this is
+  // extracted from the profile passed to the constructor.
+  void SetURLLoaderFactoryForTesting(
+      scoped_refptr<network::SharedURLLoaderFactory> testing_loader_factory);
+
  protected:
   // Creates/displays the alternate nav infobar.  Overridden in tests.
   virtual void CreateAlternateNavInfoBar();
@@ -105,27 +109,32 @@ class ChromeOmniboxNavigationObserver : public OmniboxNavigationObserver,
       const content::LoadCommittedDetails& load_details) override;
   void WebContentsDestroyed() override;
 
-  // net::URLFetcherDelegate:
-  void OnURLFetchComplete(const net::URLFetcher* source) override;
+  // Used as callbacks from |loader_|.
+  void OnURLLoadComplete(std::unique_ptr<std::string> body);
+
+  // See SimpleURLLoader::OnRedirectCallback for info on the signature.
+  void OnURLRedirect(const net::RedirectInfo& redirect_info,
+                     const network::ResourceResponseHead& response_head,
+                     std::vector<std::string>* to_be_removed_headers);
+
+  // Called from either OnURLLoadComplete or OnURLRedirect.
+  void OnDoneWithURL(bool success);
 
   // Once the load has committed and any URL fetch has completed, this displays
   // the alternate nav infobar if necessary, and deletes |this|.
   void OnAllLoadingFinished();
 
-  // Creates a URL fetcher for |destination_url| and stores it in |fetcher_|.
-  // Does not start the fetcher.
-  void CreateFetcher(const GURL& destination_url);
+  // Creates a URL loader for |destination_url| and stores it in |loader_|.
+  // Does not start the loader.
+  void CreateLoader(const GURL& destination_url);
 
   const base::string16 text_;
   const AutocompleteMatch match_;
   const AutocompleteMatch alternate_nav_match_;
   TemplateURLService* template_url_service_;
   scoped_refptr<ShortcutsBackend> shortcuts_backend_;  // NULL in incognito.
-  std::unique_ptr<net::URLFetcher> fetcher_;
-  // The request context used by |fetcher_|.  It's necessary to keep a reference
-  // to this because sometimes we need to create a follow-up fetcher with the
-  // same context and URLFetcher does not have a way to get the context out.
-  net::URLRequestContextGetter* request_context_;
+  std::unique_ptr<network::SimpleURLLoader> loader_;
+  scoped_refptr<network::SharedURLLoaderFactory> loader_factory_for_testing_;
   LoadState load_state_;
   FetchState fetch_state_;
 

services/network/test/test_url_loader_factory.cc

@@ -34,13 +34,15 @@ void TestURLLoaderFactory::AddResponse(const GURL& url,
                                        const ResourceResponseHead& head,
                                        const std::string& content,
                                        const URLLoaderCompletionStatus& status,
-                                       const Redirects& redirects) {
+                                       const Redirects& redirects,
+                                       ResponseProduceFlags flags) {
   Response response;
   response.url = url;
   response.redirects = redirects;
   response.head = head;
   response.content = content;
   response.status = status;
+  response.flags = flags;
   responses_[url] = response;
 
   for (auto it = pending_requests_.begin(); it != pending_requests_.end();) {
@@ -125,7 +127,7 @@ bool TestURLLoaderFactory::CreateLoaderAndStartInternal(
     return false;
 
   SimulateResponse(client, it->second.redirects, it->second.head,
-                   it->second.content, it->second.status);
+                   it->second.content, it->second.status, it->second.flags);
   return true;
 }
 
@@ -134,7 +136,7 @@ bool TestURLLoaderFactory::SimulateResponseForPendingRequest(
     const network::URLLoaderCompletionStatus& completion_status,
     const ResourceResponseHead& response_head,
     const std::string& content,
-    SimulateResponseFlags flags) {
+    ResponseMatchFlags flags) {
   if (pending_requests_.empty())
     return false;
 
@@ -172,7 +174,7 @@ bool TestURLLoaderFactory::SimulateResponseForPendingRequest(
   status.decoded_body_length = content.size();
 
   SimulateResponse(request.client.get(), TestURLLoaderFactory::Redirects(),
-                   response_head, content, status);
+                   response_head, content, status, kResponseDefault);
   base::RunLoop().RunUntilIdle();
 
   return true;
@@ -186,7 +188,7 @@ void TestURLLoaderFactory::SimulateResponseWithoutRemovingFromPendingList(
   URLLoaderCompletionStatus status(completion_status);
   status.decoded_body_length = content.size();
   SimulateResponse(request->client.get(), TestURLLoaderFactory::Redirects(),
-                   head, content, status);
+                   head, content, status, kResponseDefault);
   base::RunLoop().RunUntilIdle();
 }
 
@@ -205,10 +207,14 @@ void TestURLLoaderFactory::SimulateResponse(
     TestURLLoaderFactory::Redirects redirects,
     ResourceResponseHead head,
     std::string content,
-    URLLoaderCompletionStatus status) {
+    URLLoaderCompletionStatus status,
+    ResponseProduceFlags response_flags) {
   for (const auto& redirect : redirects)
     client->OnReceiveRedirect(redirect.first, redirect.second);
 
+  if (response_flags & kResponseOnlyRedirectsNoDestination)
+    return;
+
   if (status.error_code == net::OK) {
     client->OnReceiveResponse(head);
     mojo::DataPipe data_pipe(content.size());

services/network/test/test_url_loader_factory.h

@@ -32,15 +32,22 @@ class TestURLLoaderFactory : public mojom::URLLoaderFactory {
     ResourceRequest request;
   };
 
-  // Bitfield that is used with |SimulateResponseForPendingRequest()|.
-  enum SimulateResponseFlags : uint32_t {
-    kDefault = 0x0,
+  // Bitfield that is used with |SimulateResponseForPendingRequest()| to
+  // control which request is selected.
+  enum ResponseMatchFlags : uint32_t {
+    kMatchDefault = 0x0,
     kUrlMatchPrefix = 0x1,   // Whether URLs are a match if they start with the
                              // URL passed in to
                              // SimulateResponseForPendingRequest
     kMostRecentMatch = 0x2,  // Start with the most recent requests.
   };
 
+  // Flags used with |AddResponse| to control how it produces a response.
+  enum ResponseProduceFlags : uint32_t {
+    kResponseDefault = 0,
+    kResponseOnlyRedirectsNoDestination = 0x1,
+  };
+
   TestURLLoaderFactory();
   ~TestURLLoaderFactory() override;
 
@@ -56,7 +63,8 @@ class TestURLLoaderFactory : public mojom::URLLoaderFactory {
                    const ResourceResponseHead& head,
                    const std::string& content,
                    const URLLoaderCompletionStatus& status,
-                   const Redirects& redirects = Redirects());
+                   const Redirects& redirects = Redirects(),
+                   ResponseProduceFlags rp_flags = kResponseDefault);
 
   // Simpler version of above for the common case of success or error page.
   void AddResponse(const std::string& url,
@@ -96,7 +104,7 @@ class TestURLLoaderFactory : public mojom::URLLoaderFactory {
       const network::URLLoaderCompletionStatus& completion_status,
       const ResourceResponseHead& response_head,
       const std::string& content,
-      SimulateResponseFlags flags = kDefault);
+      ResponseMatchFlags flags = kMatchDefault);
 
   // Sends a response for the given request |request|.
   //
@@ -133,7 +141,8 @@ class TestURLLoaderFactory : public mojom::URLLoaderFactory {
                                Redirects redirects,
                                ResourceResponseHead head,
                                std::string content,
-                               URLLoaderCompletionStatus status);
+                               URLLoaderCompletionStatus status,
+                               ResponseProduceFlags response_flags);
 
   struct Response {
     Response();
@@ -144,6 +153,7 @@ class TestURLLoaderFactory : public mojom::URLLoaderFactory {
     ResourceResponseHead head;
     std::string content;
     URLLoaderCompletionStatus status;
+    ResponseProduceFlags flags;
   };
   std::map<GURL, Response> responses_;
 

services/network/test/test_url_loader_factory_unittest.cc

@@ -289,7 +289,7 @@ TEST_F(TestURLLoaderFactoryTest, SimulateResponseUrlMatch) {
       GURL(base_url), ok_status, response_head, /*content=*/""));
   EXPECT_FALSE(factory()->SimulateResponseForPendingRequest(
       GURL(base_url), ok_status, response_head, /*content=*/"",
-      TestURLLoaderFactory::kDefault));
+      TestURLLoaderFactory::kMatchDefault));
 
   EXPECT_TRUE(factory()->SimulateResponseForPendingRequest(
       GURL(base_url), ok_status, response_head, /*content=*/"",