Ensure we pass the scheme and port for dns prefetches.

Currently when a dns-prefetch was encountered either in an HTTP header, an anchor tag or on hover the browser would do a DNS lookup based on a hard coded scheme of "http". Since net::HostCache uses a key that changes based on whether scheme was used or not, along with the port, this meant that additional DNS requests were happening. When the system resolver is used the overhead is probably low since the system would cache it, but it would still slow things down because the synchronous path wasn't taken. When DoH is used this would be more expensive. And in both cases the cache fills up faster which could then take out records that are more likely to be used than the ones with incorrect scheme/port. (cherry picked from commit 069bdb0) Bug: 1357161 Change-Id: I8e25bf141ffc0e344a08d865b29ac9a288427edd Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4151321 Reviewed-by: Nate Chapin <japhet@chromium.org> Reviewed-by: Ryan Sturm <ryansturm@chromium.org> Commit-Queue: John Abd-El-Malek <jam@chromium.org> Reviewed-by: Nasko Oskov <nasko@chromium.org> Reviewed-by: Adam Rice <ricea@chromium.org> Cr-Original-Commit-Position: refs/heads/main@{#1091446} Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4182143 Commit-Queue: Nate Chapin <japhet@chromium.org> Reviewed-by: Alex Gough <ajgo@chromium.org> Auto-Submit: John Abd-El-Malek <jam@chromium.org> Cr-Commit-Position: refs/branch-heads/5481@{#503} Cr-Branched-From: 130f3e4-refs/heads/main@{#1084008}
chromium · Jan 20, 2023 · fe642ce · fe642ce
1 parent e24b3e6
commit fe642ce
Show file tree

Hide file tree

Showing 22 changed files with 78 additions and 47 deletions.
diff --git a/...oid_webview/java/src/org/chromium/android_webview/common/ProductionSupportedFlagList.java b/...oid_webview/java/src/org/chromium/android_webview/common/ProductionSupportedFlagList.java
@@ -344,6 +344,7 @@ private ProductionSupportedFlagList() {}
             Flag.baseFeature(BlinkFeatures.THREADED_BODY_LOADER,
                     "If enabled, reads and decodes navigation body data off the main thread."),
             Flag.baseFeature("PreconnectOnRedirect"),
+            Flag.baseFeature("PreconnectInNetworkService"), Flag.baseFeature("PrefetchDNSWithURL"),
             Flag.baseFeature(BlinkFeatures.SEND_MOUSE_EVENTS_DISABLED_FORM_CONTROLS,
                     "This changes event propagation for disabled form controls."),
             Flag.baseFeature(ContentFeatures.SURFACE_SYNC_FULLSCREEN_KILLSWITCH,

diff --git a/chrome/browser/predictors/loading_predictor_unittest.cc b/chrome/browser/predictors/loading_predictor_unittest.cc
@@ -54,7 +54,7 @@ class MockPreconnectManager : public PreconnectManager {
            const net::NetworkAnonymizationKey& network_anonymization_key));
   MOCK_METHOD2(
       StartPreresolveHosts,
-      void(const std::vector<std::string>& hostnames,
+      void(const std::vector<GURL>& urls,
            const net::NetworkAnonymizationKey& network_anonymization_key));
   MOCK_METHOD3(StartPreconnectUrl,
                void(const GURL& url,

diff --git a/chrome/browser/predictors/network_hints_handler_impl.cc b/chrome/browser/predictors/network_hints_handler_impl.cc
@@ -43,8 +43,7 @@ void NetworkHintsHandlerImpl::Create(
       std::move(receiver));
 }
 
-void NetworkHintsHandlerImpl::PrefetchDNS(
-    const std::vector<std::string>& names) {
+void NetworkHintsHandlerImpl::PrefetchDNS(const std::vector<GURL>& urls) {
   if (!preconnect_manager_)
     return;
 
@@ -54,7 +53,7 @@ void NetworkHintsHandlerImpl::PrefetchDNS(
     return;
 
   preconnect_manager_->StartPreresolveHosts(
-      names, GetPendingNetworkAnonymizationKey(render_frame_host));
+      urls, GetPendingNetworkAnonymizationKey(render_frame_host));
 }
 
 void NetworkHintsHandlerImpl::Preconnect(const GURL& url,

diff --git a/chrome/browser/predictors/network_hints_handler_impl.h b/chrome/browser/predictors/network_hints_handler_impl.h
@@ -29,7 +29,7 @@ class NetworkHintsHandlerImpl
           receiver);
 
   // network_hints::mojom::NetworkHintsHandler methods:
-  void PrefetchDNS(const std::vector<std::string>& names) override;
+  void PrefetchDNS(const std::vector<GURL>& urls) override;
   void Preconnect(const GURL& url, bool allow_credentials) override;
 
  private:

diff --git a/chrome/browser/predictors/preconnect_manager.cc b/chrome/browser/predictors/preconnect_manager.cc
@@ -132,15 +132,15 @@ void PreconnectManager::StartPreresolveHost(
 }
 
 void PreconnectManager::StartPreresolveHosts(
-    const std::vector<std::string>& hostnames,
+    const std::vector<GURL>& urls,
     const net::NetworkAnonymizationKey& network_anonymization_key) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   if (!IsEnabled())
     return;
   // Push jobs in front of the queue due to higher priority.
-  for (const std::string& hostname : base::Reversed(hostnames)) {
+  for (const GURL& url : base::Reversed(urls)) {
     PreresolveJobId job_id = preresolve_jobs_.Add(
-        std::make_unique<PreresolveJob>(GURL("http://" + hostname), 0,
+        std::make_unique<PreresolveJob>(url.DeprecatedGetOriginAsURL(), 0,
                                         kAllowCredentialsOnPreconnectByDefault,
                                         network_anonymization_key, nullptr));
     queued_jobs_.push_front(job_id);

diff --git a/chrome/browser/predictors/preconnect_manager.h b/chrome/browser/predictors/preconnect_manager.h
@@ -182,7 +182,7 @@ class PreconnectManager {
       const GURL& url,
       const net::NetworkAnonymizationKey& network_anonymization_key);
   virtual void StartPreresolveHosts(
-      const std::vector<std::string>& hostnames,
+      const std::vector<GURL>& urls,
       const net::NetworkAnonymizationKey& network_anonymization_key);
   virtual void StartPreconnectUrl(
       const GURL& url,

diff --git a/chrome/browser/predictors/preconnect_manager_unittest.cc b/chrome/browser/predictors/preconnect_manager_unittest.cc
@@ -861,7 +861,7 @@ TEST_F(PreconnectManagerTest, TestStartPreresolveHosts) {
 
   EXPECT_CALL(*mock_network_context_, ResolveHostProxy(cdn.host()));
   EXPECT_CALL(*mock_network_context_, ResolveHostProxy(fonts.host()));
-  preconnect_manager_->StartPreresolveHosts({cdn.host(), fonts.host()},
+  preconnect_manager_->StartPreresolveHosts({cdn, fonts},
                                             network_anonymization_key);
   mock_network_context_->CompleteHostLookup(cdn.host(),
                                             network_anonymization_key, net::OK);
@@ -879,7 +879,7 @@ TEST_F(PreconnectManagerTest, TestStartPreresolveHostsDisabledViaUI) {
 
   // mock_network_context_.ResolveHostProxy shouldn't be called. The StrictMock
   // will raise an error if it happens.
-  preconnect_manager_->StartPreresolveHosts({cdn.host(), fonts.host()},
+  preconnect_manager_->StartPreresolveHosts({cdn, fonts},
                                             network_anonymization_key);
 }
 

diff --git a/components/network_hints/browser/simple_network_hints_handler_impl.cc b/components/network_hints/browser/simple_network_hints_handler_impl.cc
@@ -41,19 +41,15 @@ net::NetworkAnonymizationKey GetPendingNetworkAnonymizationKey(
       .network_anonymization_key();
 }
 
-const int kDefaultPort = 80;
-
 // This class contains a std::unique_ptr of itself, it is passed in through
 // Start() method, and will be freed by the OnComplete() method when resolving
 // has completed or mojo connection error has happened.
 class DnsLookupRequest : public network::ResolveHostClientBase {
  public:
-  DnsLookupRequest(int render_process_id,
-                   int render_frame_id,
-                   const std::string& hostname)
+  DnsLookupRequest(int render_process_id, int render_frame_id, const GURL& url)
       : render_process_id_(render_process_id),
         render_frame_id_(render_frame_id),
-        hostname_(hostname) {}
+        url_(url) {}
 
   DnsLookupRequest(const DnsLookupRequest&) = delete;
   DnsLookupRequest& operator=(const DnsLookupRequest&) = delete;
@@ -76,7 +72,7 @@ class DnsLookupRequest : public network::ResolveHostClientBase {
     }
 
     DCHECK(!receiver_.is_bound());
-    net::HostPortPair host_port_pair(hostname_, kDefaultPort);
+    url::SchemeHostPort scheme_host_port(url_);
     network::mojom::ResolveHostParametersPtr resolve_host_parameters =
         network::mojom::ResolveHostParameters::New();
     // Lets the host resolver know it can be de-prioritized.
@@ -91,8 +87,8 @@ class DnsLookupRequest : public network::ResolveHostClientBase {
     render_frame_host->GetProcess()
         ->GetStoragePartition()
         ->GetNetworkContext()
-        ->ResolveHost(network::mojom::HostResolverHost::NewHostPortPair(
-                          std::move(host_port_pair)),
+        ->ResolveHost(network::mojom::HostResolverHost::NewSchemeHostPort(
+                          std::move(scheme_host_port)),
                       GetPendingNetworkAnonymizationKey(render_frame_host),
                       std::move(resolve_host_parameters),
                       receiver_.BindNewPipeAndPassRemote());
@@ -110,15 +106,15 @@ class DnsLookupRequest : public network::ResolveHostClientBase {
                   const absl::optional<net::AddressList>& resolved_addresses,
                   const absl::optional<net::HostResolverEndpointResults>&
                       endpoint_results_with_metadata) override {
-    VLOG(2) << __FUNCTION__ << ": " << hostname_
+    VLOG(2) << __FUNCTION__ << ": " << url_.spec()
             << ", result=" << resolve_error_info.error;
     request_.reset();
   }
 
   mojo::Receiver<network::mojom::ResolveHostClient> receiver_{this};
   const int render_process_id_;
   const int render_frame_id_;
-  const std::string hostname_;
+  const GURL url_;
   std::unique_ptr<DnsLookupRequest> request_;
 };
 
@@ -144,12 +140,11 @@ void SimpleNetworkHintsHandlerImpl::Create(
       std::move(receiver));
 }
 
-void SimpleNetworkHintsHandlerImpl::PrefetchDNS(
-    const std::vector<std::string>& names) {
-  for (const std::string& hostname : names) {
+void SimpleNetworkHintsHandlerImpl::PrefetchDNS(const std::vector<GURL>& urls) {
+  for (const GURL& url : urls) {
     std::unique_ptr<DnsLookupRequest> request =
         std::make_unique<DnsLookupRequest>(render_process_id_, render_frame_id_,
-                                           hostname);
+                                           url);
     DnsLookupRequest* request_ptr = request.get();
     request_ptr->Start(std::move(request));
   }

diff --git a/components/network_hints/browser/simple_network_hints_handler_impl.h b/components/network_hints/browser/simple_network_hints_handler_impl.h
@@ -30,7 +30,7 @@ class SimpleNetworkHintsHandlerImpl : public mojom::NetworkHintsHandler {
       mojo::PendingReceiver<mojom::NetworkHintsHandler> receiver);
 
   // mojom::NetworkHintsHandler methods:
-  void PrefetchDNS(const std::vector<std::string>& names) override;
+  void PrefetchDNS(const std::vector<GURL>& urls) override;
   void Preconnect(const GURL& url, bool allow_credentials) override;
 
  private:

diff --git a/components/network_hints/common/network_hints.mojom b/components/network_hints/common/network_hints.mojom
@@ -8,9 +8,9 @@ import "url/mojom/url.mojom";
 
 // This interface is used by the renderer to provide hints to the browser.
 interface NetworkHintsHandler {
-  // This method is called periodically with a hint to prefetch a batch set of
-  // hostnames.
-  PrefetchDNS(array<string> hostname_list);
+  // This method is called periodically with a hint to perform DNS lookups for
+  // a batch set of urls.
+  PrefetchDNS(array<url.mojom.Url> url_list);
 
   // This method is called periodically with a hint to preconnect to the origin
   // of the specified url.

diff --git a/components/network_hints/renderer/DEPS b/components/network_hints/renderer/DEPS
@@ -3,4 +3,5 @@ include_rules = [
   "+content/public/renderer",
   "+mojo",
   "+third_party/blink/public",
+  "+services/network/public/cpp",
 ]
diff --git a/components/network_hints/renderer/web_prescient_networking_impl.cc b/components/network_hints/renderer/web_prescient_networking_impl.cc
@@ -6,14 +6,19 @@
 
 #include "base/logging.h"
 #include "content/public/renderer/render_frame.h"
+#include "services/network/public/cpp/features.h"
 #include "third_party/blink/public/common/browser_interface_broker_proxy.h"
 
 namespace network_hints {
 namespace {
 
 void ForwardToHandler(mojo::Remote<mojom::NetworkHintsHandler>* handler,
                       const std::vector<std::string>& names) {
-  handler->get()->PrefetchDNS(names);
+  std::vector<GURL> urls;
+  for (const auto& name : names) {
+    urls.emplace_back("http://" + name);
+  }
+  handler->get()->PrefetchDNS(urls);
 }
 
 }  // namespace
@@ -28,13 +33,24 @@ WebPrescientNetworkingImpl::WebPrescientNetworkingImpl(
 
 WebPrescientNetworkingImpl::~WebPrescientNetworkingImpl() {}
 
-void WebPrescientNetworkingImpl::PrefetchDNS(const blink::WebString& hostname) {
-  DVLOG(2) << "Prefetch DNS: " << hostname.Utf8();
-  if (hostname.IsEmpty())
+void WebPrescientNetworkingImpl::PrefetchDNS(const blink::WebURL& url) {
+  DVLOG(2) << "Prefetch DNS: " << url.GetString().Utf8();
+  GURL gurl(url);
+  if (!gurl.is_valid() || !gurl.has_host()) {
     return;
+  }
 
-  std::string hostname_utf8 = hostname.Utf8();
-  dns_prefetch_.Resolve(hostname_utf8.data(), hostname_utf8.length());
+  if (base::FeatureList::IsEnabled(network::features::kPrefetchDNSWithURL)) {
+    std::vector<GURL> urls;
+    urls.push_back(std::move(gurl));
+    handler_->PrefetchDNS(urls);
+    // TODO(jam): If this launches remove DnsQueue and RendererDnsPrefetch
+    // which are no longer needed. They were from a feature which existed
+    // at launch but not anymore that prefetched DNS for every link on a page.
+  } else {
+    auto host_piece = gurl.host_piece();
+    dns_prefetch_.Resolve(host_piece.data(), host_piece.length());
+  }
 }
 
 void WebPrescientNetworkingImpl::Preconnect(

diff --git a/components/network_hints/renderer/web_prescient_networking_impl.h b/components/network_hints/renderer/web_prescient_networking_impl.h
@@ -29,7 +29,7 @@ class WebPrescientNetworkingImpl : public blink::WebPrescientNetworking {
   ~WebPrescientNetworkingImpl() override;
 
   // blink::WebPrescientNetworking methods:
-  void PrefetchDNS(const blink::WebString& hostname) override;
+  void PrefetchDNS(const blink::WebURL& url) override;
   void Preconnect(const blink::WebURL& url, bool allow_credentials) override;
 
  private:

diff --git a/services/network/public/cpp/features.cc b/services/network/public/cpp/features.cc
@@ -307,6 +307,16 @@ BASE_FEATURE(kPreconnectInNetworkService,
              "PreconnectInNetworkService",
              base::FEATURE_DISABLED_BY_DEFAULT);
 
+// When prefetching a DNS record ensures that the scheme and port are taken
+// into account so that the cache (which is keyed by scheme and port) works
+// for subsequent queries.
+BASE_FEATURE(kPrefetchDNSWithURL,
+             "PrefetchDNSWithURL",
+             base::FEATURE_DISABLED_BY_DEFAULT);
+
+constexpr base::FeatureParam<bool> kPrefetchDNSWithURLAllAnchorElements{
+    &kPrefetchDNSWithURL, "prefetch_dns_all_anchor_elements", true};
+
 // Preconnect to a new origin right when a redirect starts.
 BASE_FEATURE(kPreconnectOnRedirect,
              "PreconnectOnRedirect",

diff --git a/services/network/public/cpp/features.h b/services/network/public/cpp/features.h
@@ -97,6 +97,11 @@ BASE_DECLARE_FEATURE(kPrivateNetworkAccessPreflightShortTimeout);
 
 COMPONENT_EXPORT(NETWORK_CPP) BASE_DECLARE_FEATURE(kPreconnectInNetworkService);
 
+COMPONENT_EXPORT(NETWORK_CPP) BASE_DECLARE_FEATURE(kPrefetchDNSWithURL);
+
+COMPONENT_EXPORT(NETWORK_CPP)
+extern const base::FeatureParam<bool> kPrefetchDNSWithURLAllAnchorElements;
+
 COMPONENT_EXPORT(NETWORK_CPP) BASE_DECLARE_FEATURE(kPreconnectOnRedirect);
 
 COMPONENT_EXPORT(NETWORK_CPP)

diff --git a/third_party/blink/public/platform/web_prescient_networking.h b/third_party/blink/public/platform/web_prescient_networking.h
@@ -42,7 +42,7 @@ class WebPrescientNetworking {
 
   // When a page navigation is speculated, DNS prefetch is triggered to hide
   // the host resolution latency.
-  virtual void PrefetchDNS(const WebString& hostname) {}
+  virtual void PrefetchDNS(const WebURL& url) {}
 
   virtual void Preconnect(const WebURL& url, bool allow_credentials) {}
 };

diff --git a/third_party/blink/renderer/core/html/html_anchor_element.cc b/third_party/blink/renderer/core/html/html_anchor_element.cc
@@ -26,6 +26,7 @@
 
 #include "base/metrics/histogram_macros.h"
 #include "base/time/time.h"
+#include "services/network/public/cpp/features.h"
 #include "third_party/blink/public/common/features.h"
 #include "third_party/blink/public/mojom/conversions/attribution_reporting.mojom-blink.h"
 #include "third_party/blink/public/mojom/fetch/fetch_api_request.mojom-blink.h"
@@ -252,14 +253,19 @@ void HTMLAnchorElement::ParseAttribute(
       // GetDocument().GetFrame() could be null if this method is called from
       // DOMParser::parseFromString(), which internally creates a document
       // and eventually calls this.
-      if (GetDocument().IsDNSPrefetchEnabled() && GetDocument().GetFrame()) {
+      static bool enable =
+          !base::FeatureList::IsEnabled(
+              network::features::kPrefetchDNSWithURL) ||
+          network::features::kPrefetchDNSWithURLAllAnchorElements.Get();
+      if (GetDocument().IsDNSPrefetchEnabled() && GetDocument().GetFrame() &&
+          enable) {
         if (ProtocolIs(parsed_url, "http") || ProtocolIs(parsed_url, "https") ||
             parsed_url.StartsWith("//")) {
           WebPrescientNetworking* web_prescient_networking =
               GetDocument().GetFrame()->PrescientNetworking();
           if (web_prescient_networking) {
             web_prescient_networking->PrefetchDNS(
-                GetDocument().CompleteURL(parsed_url).Host());
+                GetDocument().CompleteURL(parsed_url));
           }
         }
       }

diff --git a/third_party/blink/renderer/core/html/parser/html_preload_scanner_document_test.cc b/third_party/blink/renderer/core/html/parser/html_preload_scanner_document_test.cc
@@ -28,7 +28,7 @@ class MockPrescientNetworking : public WebPrescientNetworking {
   bool DidPreconnect() const { return did_preconnect_; }
 
  private:
-  void PrefetchDNS(const WebString&) override { did_dns_prefetch_ = true; }
+  void PrefetchDNS(const WebURL&) override { did_dns_prefetch_ = true; }
   void Preconnect(const WebURL&, bool) override { did_preconnect_ = true; }
 
   bool did_dns_prefetch_ = false;

diff --git a/third_party/blink/renderer/core/html/parser/html_resource_preloader_test.cc b/third_party/blink/renderer/core/html/parser/html_resource_preloader_test.cc
@@ -25,7 +25,7 @@ class PreloaderNetworkHintsMock : public WebPrescientNetworking {
  public:
   PreloaderNetworkHintsMock() : did_preconnect_(false) {}
 
-  void PrefetchDNS(const WebString& hostname) override {}
+  void PrefetchDNS(const WebURL& url) override {}
   void Preconnect(const WebURL& url, bool allow_credentials) override {
     did_preconnect_ = true;
     is_https_ = url.ProtocolIs("https");

diff --git a/third_party/blink/renderer/core/loader/link_loader_test.cc b/third_party/blink/renderer/core/loader/link_loader_test.cc
@@ -61,9 +61,7 @@ class NetworkHintsMock : public WebPrescientNetworking {
  public:
   NetworkHintsMock() = default;
 
-  void PrefetchDNS(const WebString& hostname) override {
-    did_dns_prefetch_ = true;
-  }
+  void PrefetchDNS(const WebURL& url) override { did_dns_prefetch_ = true; }
   void Preconnect(const WebURL& url, bool allow_credentials) override {
     did_preconnect_ = true;
     is_https_ = url.ProtocolIs("https");

diff --git a/third_party/blink/renderer/core/loader/preload_helper.cc b/third_party/blink/renderer/core/loader/preload_helper.cc
@@ -177,7 +177,7 @@ void PreloadHelper::DnsPrefetchIfNeeded(
       WebPrescientNetworking* web_prescient_networking =
           frame ? frame->PrescientNetworking() : nullptr;
       if (web_prescient_networking) {
-        web_prescient_networking->PrefetchDNS(params.href.Host());
+        web_prescient_networking->PrefetchDNS(params.href);
       }
     }
   }

diff --git a/third_party/blink/renderer/core/page/chrome_client.cc b/third_party/blink/renderer/core/page/chrome_client.cc
@@ -161,7 +161,7 @@ void ChromeClient::MouseDidMoveOverElement(LocalFrame& frame,
     WebPrescientNetworking* web_prescient_networking =
         frame.PrescientNetworking();
     if (web_prescient_networking) {
-      web_prescient_networking->PrefetchDNS(result.AbsoluteLinkURL().Host());
+      web_prescient_networking->PrefetchDNS(result.AbsoluteLinkURL());
     }
   }