predictors: Request a proxy lookup in parallel with host resolve request

This CL adds an asynchronous proxy lookup request to the PreconnectManager
after the synchronous request was removed in https://crrev.com/c/1165550.
The idea is that we don't need to wait for a host to be resolved before a
preconnect if a proxy is enabled.

Since we don't have a synchronous API for proxy lookup anymore, we issue both
host and proxy requests in parallel. Then we wait until either one of the
requests completes with success or both requests fail.

Bug: 838763
Change-Id: Ib3fa47884ba82d1fc12c62f7fd24dd21c279e9c5
Reviewed-on: https://chromium-review.googlesource.com/1202222
Commit-Queue: Alexandr Ilin <alexilin@chromium.org>
Reviewed-by: Benoit L <lizeb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#589877}

chrome/browser/BUILD.gn

@@ -1132,6 +1132,8 @@ jumbo_split_static_library("browser") {
     "predictors/predictor_database_factory.h",
     "predictors/predictor_table_base.cc",
     "predictors/predictor_table_base.h",
+    "predictors/proxy_lookup_client_impl.cc",
+    "predictors/proxy_lookup_client_impl.h",
     "predictors/resolve_host_client_impl.cc",
     "predictors/resolve_host_client_impl.h",
     "predictors/resource_prefetch_common.cc",

chrome/browser/predictors/loading_stats_collector_unittest.cc

@@ -174,11 +174,11 @@ TEST_F(LoadingStatsCollectorTest, TestPreconnectHistograms) {
     // Initialize PreconnectStats.
 
     // These two are hits.
-    PreconnectedRequestStats origin1(GURL(gen(1)).GetOrigin(), false, true);
-    PreconnectedRequestStats origin2(GURL(gen(2)).GetOrigin(), true, false);
+    PreconnectedRequestStats origin1(GURL(gen(1)).GetOrigin(), true);
+    PreconnectedRequestStats origin2(GURL(gen(2)).GetOrigin(), false);
     // And these two are misses.
-    PreconnectedRequestStats origin3(GURL(gen(3)).GetOrigin(), false, false);
-    PreconnectedRequestStats origin4(GURL(gen(4)).GetOrigin(), true, true);
+    PreconnectedRequestStats origin3(GURL(gen(3)).GetOrigin(), false);
+    PreconnectedRequestStats origin4(GURL(gen(4)).GetOrigin(), true);
 
     auto stats = std::make_unique<PreconnectStats>(GURL(main_frame_url));
     stats->requests_stats = {origin1, origin2, origin3, origin4};
@@ -257,11 +257,11 @@ TEST_F(LoadingStatsCollectorTest, TestPreconnectHistogramsPreresolvesOnly) {
     // Initialize PreconnectStats.
 
     // These two are hits.
-    PreconnectedRequestStats origin1(GURL(gen(1)).GetOrigin(), false, false);
-    PreconnectedRequestStats origin2(GURL(gen(2)).GetOrigin(), true, false);
+    PreconnectedRequestStats origin1(GURL(gen(1)).GetOrigin(), false);
+    PreconnectedRequestStats origin2(GURL(gen(2)).GetOrigin(), false);
     // And these two are misses.
-    PreconnectedRequestStats origin3(GURL(gen(3)).GetOrigin(), false, false);
-    PreconnectedRequestStats origin4(GURL(gen(4)).GetOrigin(), true, false);
+    PreconnectedRequestStats origin3(GURL(gen(3)).GetOrigin(), false);
+    PreconnectedRequestStats origin4(GURL(gen(4)).GetOrigin(), false);
 
     auto stats = std::make_unique<PreconnectStats>(GURL(main_frame_url));
     stats->requests_stats = {origin1, origin2, origin3, origin4};

chrome/browser/predictors/preconnect_manager.cc

@@ -19,10 +19,8 @@ namespace predictors {
 const bool kAllowCredentialsOnPreconnectByDefault = true;
 
 PreconnectedRequestStats::PreconnectedRequestStats(const GURL& origin,
-                                                   bool was_preresolve_cached,
                                                    bool was_preconnected)
     : origin(origin),
-      was_preresolve_cached(was_preresolve_cached),
       was_preconnected(was_preconnected) {}
 
 PreconnectedRequestStats::PreconnectedRequestStats(
@@ -141,7 +139,6 @@ void PreconnectManager::Stop(const GURL& url) {
 }
 
 void PreconnectManager::PreconnectUrl(const GURL& url,
-                                      const GURL& site_for_cookies,
                                       int num_sockets,
                                       bool allow_credentials) const {
   DCHECK(url.GetOrigin() == url);
@@ -171,8 +168,6 @@ std::unique_ptr<ResolveHostClientImpl> PreconnectManager::PreresolveUrl(
     ResolveHostCallback callback) const {
   DCHECK(url.GetOrigin() == url);
   DCHECK(url.SchemeIsHTTPOrHTTPS());
-  if (observer_)
-    observer_->OnPreresolveUrl(url);
 
   auto* network_context = GetNetworkContext();
   if (!network_context) {
@@ -185,7 +180,24 @@ std::unique_ptr<ResolveHostClientImpl> PreconnectManager::PreresolveUrl(
   }
 
   return std::make_unique<ResolveHostClientImpl>(url, std::move(callback),
-                                                 GetNetworkContext());
+                                                 network_context);
+}
+
+std::unique_ptr<ProxyLookupClientImpl> PreconnectManager::LookupProxyForUrl(
+    const GURL& url,
+    ProxyLookupCallback callback) const {
+  DCHECK(url.GetOrigin() == url);
+  DCHECK(url.SchemeIsHTTPOrHTTPS());
+
+  auto* network_context = GetNetworkContext();
+  if (!network_context) {
+    // It's okay to not invoke the callback here because PreresolveUrl()
+    // callback will be invoked.
+    return nullptr;
+  }
+
+  return std::make_unique<ProxyLookupClientImpl>(url, std::move(callback),
+                                                 network_context);
 }
 
 void PreconnectManager::TryToLaunchPreresolveJobs() {
@@ -200,7 +212,13 @@ void PreconnectManager::TryToLaunchPreresolveJobs() {
     PreresolveInfo* info = job->info;
 
     if (!(info && info->was_canceled)) {
-      // TODO(crbug.com/838763): Preconnect to url if proxy is enabled.
+      // This is used to avoid issuing DNS requests when a fixed proxy
+      // configuration is in place, which improves efficiency, and is also
+      // important if the unproxied DNS may contain incorrect entries.
+      job->proxy_lookup_client = LookupProxyForUrl(
+          job->url, base::BindOnce(&PreconnectManager::OnProxyLookupFinished,
+                                   weak_factory_.GetWeakPtr(), job_id));
+
       job->resolve_host_client = PreresolveUrl(
           job->url, base::BindOnce(&PreconnectManager::OnPreresolveFinished,
                                    weak_factory_.GetWeakPtr(), job_id));
@@ -221,37 +239,52 @@ void PreconnectManager::OnPreresolveFinished(PreresolveJobId job_id,
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   PreresolveJob* job = preresolve_jobs_.Lookup(job_id);
   DCHECK(job);
-  PreresolveInfo* info = job->info;
 
   if (observer_)
     observer_->OnPreresolveFinished(job->url, success);
 
-  FinishPreresolve(job_id, success, false);
-  --inflight_preresolves_count_;
-  if (info)
-    --info->inflight_count;
-  if (info && info->is_done())
-    AllPreresolvesForUrlFinished(info);
-  TryToLaunchPreresolveJobs();
+  job->resolve_host_client = nullptr;
+  FinishPreresolveJob(job_id, success);
 }
 
-void PreconnectManager::FinishPreresolve(PreresolveJobId job_id,
-                                         bool found,
-                                         bool cached) {
+void PreconnectManager::OnProxyLookupFinished(PreresolveJobId job_id,
+                                              bool success) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   PreresolveJob* job = preresolve_jobs_.Lookup(job_id);
   DCHECK(job);
-  PreresolveInfo* info = job->info;
 
-  bool need_preconnect =
-      found && job->need_preconnect() && (!info || !info->was_canceled);
-  if (need_preconnect) {
-    PreconnectUrl(job->url, info ? info->url : GURL(), job->num_sockets,
-                  job->allow_credentials);
-  }
-  if (info && found)
-    info->stats->requests_stats.emplace_back(job->url, cached, need_preconnect);
+  if (observer_)
+    observer_->OnProxyLookupFinished(job->url, success);
+
+  job->proxy_lookup_client = nullptr;
+  FinishPreresolveJob(job_id, success);
+}
+
+void PreconnectManager::FinishPreresolveJob(PreresolveJobId job_id,
+                                            bool success) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  PreresolveJob* job = preresolve_jobs_.Lookup(job_id);
+  DCHECK(job);
+
+  // In case one of the clients failed, wait for the second one, because it may
+  // be successful.
+  if (!success && (job->resolve_host_client || job->proxy_lookup_client))
+    return;
+
+  bool need_preconnect = success && job->need_preconnect();
+  if (need_preconnect)
+    PreconnectUrl(job->url, job->num_sockets, job->allow_credentials);
+
+  PreresolveInfo* info = job->info;
+  if (info)
+    info->stats->requests_stats.emplace_back(job->url, need_preconnect);
   preresolve_jobs_.Remove(job_id);
+  --inflight_preresolves_count_;
+  if (info)
+    --info->inflight_count;
+  if (info && info->is_done())
+    AllPreresolvesForUrlFinished(info);
+  TryToLaunchPreresolveJobs();
 }
 
 void PreconnectManager::AllPreresolvesForUrlFinished(PreresolveInfo* info) {

chrome/browser/predictors/preconnect_manager.h

@@ -14,6 +14,7 @@
 #include "base/containers/id_map.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/weak_ptr.h"
+#include "chrome/browser/predictors/proxy_lookup_client_impl.h"
 #include "chrome/browser/predictors/resolve_host_client_impl.h"
 #include "chrome/browser/predictors/resource_prefetch_predictor.h"
 #include "url/gurl.h"
@@ -32,13 +33,11 @@ struct PreconnectRequest;
 
 struct PreconnectedRequestStats {
   PreconnectedRequestStats(const GURL& origin,
-                           bool was_preresolve_cached,
                            bool was_preconnected);
   PreconnectedRequestStats(const PreconnectedRequestStats& other);
   ~PreconnectedRequestStats();
 
   GURL origin;
-  bool was_preresolve_cached;
   bool was_preconnected;
 };
 
@@ -79,7 +78,9 @@ struct PreresolveJob {
                 PreresolveInfo* info);
   PreresolveJob(PreresolveJob&& other);
   ~PreresolveJob();
-  bool need_preconnect() const { return num_sockets > 0; }
+  bool need_preconnect() const {
+    return num_sockets > 0 && !(info && info->was_canceled);
+  }
 
   GURL url;
   int num_sockets;
@@ -90,6 +91,7 @@ struct PreresolveJob {
   // May be equal to nullptr in case of detached job.
   PreresolveInfo* info;
   std::unique_ptr<ResolveHostClientImpl> resolve_host_client;
+  std::unique_ptr<ProxyLookupClientImpl> proxy_lookup_client;
 
   DISALLOW_COPY_AND_ASSIGN(PreresolveJob);
 };
@@ -125,9 +127,9 @@ class PreconnectManager {
     virtual void OnPreconnectUrl(const GURL& url,
                                  int num_sockets,
                                  bool allow_credentials) {}
-    virtual void OnPreresolveUrl(const GURL& url) {}
 
     virtual void OnPreresolveFinished(const GURL& url, bool success) {}
+    virtual void OnProxyLookupFinished(const GURL& url, bool success) {}
   };
 
   static const size_t kMaxInflightPreresolves = 3;
@@ -165,16 +167,19 @@ class PreconnectManager {
   friend class PreconnectManagerTest;
 
   void PreconnectUrl(const GURL& url,
-                     const GURL& site_for_cookies,
                      int num_sockets,
                      bool allow_credentials) const;
   std::unique_ptr<ResolveHostClientImpl> PreresolveUrl(
       const GURL& url,
       ResolveHostCallback callback) const;
+  std::unique_ptr<ProxyLookupClientImpl> LookupProxyForUrl(
+      const GURL& url,
+      ProxyLookupCallback callback) const;
 
   void TryToLaunchPreresolveJobs();
   void OnPreresolveFinished(PreresolveJobId job_id, bool success);
-  void FinishPreresolve(PreresolveJobId job_id, bool found, bool cached);
+  void OnProxyLookupFinished(PreresolveJobId job_id, bool success);
+  void FinishPreresolveJob(PreresolveJobId job_id, bool success);
   void AllPreresolvesForUrlFinished(PreresolveInfo* info);
   network::mojom::NetworkContext* GetNetworkContext() const;
 

chrome/browser/predictors/proxy_lookup_client_impl.cc

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/predictors/proxy_lookup_client_impl.h"
+
+#include <utility>
+
+#include "base/bind.h"
+#include "mojo/public/cpp/bindings/interface_request.h"
+#include "net/proxy_resolution/proxy_info.h"
+#include "services/network/public/mojom/network_context.mojom.h"
+#include "url/gurl.h"
+
+namespace predictors {
+
+ProxyLookupClientImpl::ProxyLookupClientImpl(
+    const GURL& url,
+    ProxyLookupCallback callback,
+    network::mojom::NetworkContext* network_context)
+    : binding_(this), callback_(std::move(callback)) {
+  network::mojom::ProxyLookupClientPtr proxy_lookup_client_ptr;
+  binding_.Bind(mojo::MakeRequest(&proxy_lookup_client_ptr));
+  network_context->LookUpProxyForURL(url, std::move(proxy_lookup_client_ptr));
+  binding_.set_connection_error_handler(
+      base::BindOnce(&ProxyLookupClientImpl::OnProxyLookupComplete,
+                     base::Unretained(this), base::nullopt));
+}
+
+ProxyLookupClientImpl::~ProxyLookupClientImpl() = default;
+
+void ProxyLookupClientImpl::OnProxyLookupComplete(
+    const base::Optional<net::ProxyInfo>& proxy_info) {
+  bool success = proxy_info.has_value() && !proxy_info->is_direct();
+  std::move(callback_).Run(success);
+}
+
+}  // namespace predictors

chrome/browser/predictors/proxy_lookup_client_impl.h

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROME_BROWSER_PREDICTORS_PROXY_LOOKUP_CLIENT_IMPL_H_
+#define CHROME_BROWSER_PREDICTORS_PROXY_LOOKUP_CLIENT_IMPL_H_
+
+#include "base/bind.h"
+#include "base/macros.h"
+#include "base/optional.h"
+#include "mojo/public/cpp/bindings/binding.h"
+#include "services/network/public/mojom/proxy_lookup_client.mojom.h"
+
+class GURL;
+
+namespace network {
+namespace mojom {
+class NetworkContext;
+}
+}  // namespace network
+
+namespace predictors {
+
+using ProxyLookupCallback = base::OnceCallback<void(bool success)>;
+
+// This class helps perform the proxy lookup using the NetworkContext.
+// An instance of this class must be deleted after the callback is invoked.
+class ProxyLookupClientImpl : public network::mojom::ProxyLookupClient {
+ public:
+  // Starts the proxy lookup for |url|. |callback| is called when the proxy
+  // lookup is completed or when an error occurs.
+  ProxyLookupClientImpl(const GURL& url,
+                        ProxyLookupCallback callback,
+                        network::mojom::NetworkContext* network_context);
+  // Cancels the request if it hasn't been completed yet.
+  ~ProxyLookupClientImpl() override;
+
+  // network::mojom::ProxyLookupClient:
+  void OnProxyLookupComplete(
+      const base::Optional<net::ProxyInfo>& proxy_info) override;
+
+ private:
+  mojo::Binding<network::mojom::ProxyLookupClient> binding_;
+  ProxyLookupCallback callback_;
+
+  DISALLOW_COPY_AND_ASSIGN(ProxyLookupClientImpl);
+};
+
+}  // namespace predictors
+
+#endif  // CHROME_BROWSER_PREDICTORS_PROXY_LOOKUP_CLIENT_IMPL_H_

chrome/test/data/predictors/proxy.pac

+function FindProxyForURL(url, host)
+{
+  if (shExpMatch(host, "test.com|foo.com|baz.com|bar.com"))
+  {
+    return "PROXY 127.0.0.1:REPLACE_WITH_PORT";
+  }
+
+  // All other requests don't need a proxy:
+  return "DIRECT";
+}