predictors: Always wait for dns job before starting a new one

PreconnectManager is designed with restriction on the maximum number of
simultaneous dns requests. There are instances of violation of this
constraint.

Each dns job has an alternative proxy resolve job that can finish first,
cancel pending dns job and schedule a new one. However, this cancelling
is asynchronous and wouldn't have an effect if a dns job was already
scheduled in the network stack.

This CL makes the PreconnectManager wait for dns job being cancelled
before scheduling a new one.

Bug: 883806
Change-Id: I8e90ad0dce55b569a4c77b202435f70a6b776231
Reviewed-on: https://chromium-review.googlesource.com/c/1292880Reviewed-by: Benoit L <lizeb@chromium.org>
Commit-Queue: Alexandr Ilin <alexilin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#602409}

chrome/browser/predictors/preconnect_manager.cc

@@ -49,7 +49,8 @@ PreresolveJob::PreresolveJob(const GURL& url,
     : url(url),
       num_sockets(num_sockets),
       allow_credentials(allow_credentials),
-      info(info) {
+      info(info),
+      success(false) {
   DCHECK_GE(num_sockets, 0);
 }
 
@@ -243,7 +244,8 @@ void PreconnectManager::OnPreresolveFinished(PreresolveJobId job_id,
     observer_->OnPreresolveFinished(job->url, success);
 
   job->resolve_host_client = nullptr;
-  FinishPreresolveJob(job_id, success);
+  job->success = job->success || success;
+  FinishPreresolveJob(job_id);
 }
 
 void PreconnectManager::OnProxyLookupFinished(PreresolveJobId job_id,
@@ -256,21 +258,27 @@ void PreconnectManager::OnProxyLookupFinished(PreresolveJobId job_id,
     observer_->OnProxyLookupFinished(job->url, success);
 
   job->proxy_lookup_client = nullptr;
-  FinishPreresolveJob(job_id, success);
+  job->success = job->success || success;
+  if (job->success && job->resolve_host_client)
+    job->resolve_host_client->Cancel();
+  FinishPreresolveJob(job_id);
 }
 
-void PreconnectManager::FinishPreresolveJob(PreresolveJobId job_id,
-                                            bool success) {
+void PreconnectManager::FinishPreresolveJob(PreresolveJobId job_id) {
   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))
+  // Always wait for the host resolution to be complete.
+  if (job->resolve_host_client)
     return;
 
-  bool need_preconnect = success && job->need_preconnect();
+  // Proxy lookup still may return success, wait for it before finishing the
+  // job.
+  if (!job->success && job->proxy_lookup_client)
+    return;
+
+  bool need_preconnect = job->success && job->need_preconnect();
   if (need_preconnect)
     PreconnectUrl(job->url, job->num_sockets, job->allow_credentials);
 

chrome/browser/predictors/preconnect_manager.h

@@ -92,6 +92,7 @@ struct PreresolveJob {
   PreresolveInfo* info;
   std::unique_ptr<ResolveHostClientImpl> resolve_host_client;
   std::unique_ptr<ProxyLookupClientImpl> proxy_lookup_client;
+  bool success;
 
   DISALLOW_COPY_AND_ASSIGN(PreresolveJob);
 };
@@ -179,7 +180,7 @@ class PreconnectManager {
   void TryToLaunchPreresolveJobs();
   void OnPreresolveFinished(PreresolveJobId job_id, bool success);
   void OnProxyLookupFinished(PreresolveJobId job_id, bool success);
-  void FinishPreresolveJob(PreresolveJobId job_id, bool success);
+  void FinishPreresolveJob(PreresolveJobId job_id);
   void AllPreresolvesForUrlFinished(PreresolveInfo* info);
   network::mojom::NetworkContext* GetNetworkContext() const;
 

chrome/browser/predictors/preconnect_manager_unittest.cc

@@ -20,6 +20,7 @@
 #include "content/public/test/test_browser_thread_bundle.h"
 #include "net/base/load_flags.h"
 #include "net/traffic_annotation/network_traffic_annotation_test_helper.h"
+#include "services/network/public/mojom/host_resolver.mojom.h"
 #include "services/network/test/test_network_context.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -60,6 +61,30 @@ class MockPreconnectManagerDelegate
   MOCK_METHOD1(PreconnectFinishedProxy, void(const GURL& url));
 };
 
+class TestResolveHostClient : public network::mojom::ResolveHostHandle {
+ public:
+  using CancelCallback = base::OnceCallback<void(int result)>;
+
+  TestResolveHostClient(
+      network::mojom::ResolveHostClientPtr client,
+      network::mojom::ResolveHostHandleRequest control_handle_request,
+      CancelCallback callback)
+      : callback_(std::move(callback)), client_(std::move(client)) {
+    if (control_handle_request)
+      control_handle_binding_.Bind(std::move(control_handle_request));
+  }
+
+  void Cancel(int result) override { std::move(callback_).Run(result); }
+
+  void OnComplete(int result) { client_->OnComplete(result, base::nullopt); }
+
+ private:
+  CancelCallback callback_;
+  network::mojom::ResolveHostClientPtr client_;
+  mojo::Binding<network::mojom::ResolveHostHandle> control_handle_binding_{
+      this};
+};
+
 class MockNetworkContext : public network::TestNetworkContext {
  public:
   MockNetworkContext() = default;
@@ -73,8 +98,16 @@ class MockNetworkContext : public network::TestNetworkContext {
       network::mojom::ResolveHostParametersPtr optional_parameters,
       network::mojom::ResolveHostClientPtr response_client) override {
     const std::string& host = host_port.host();
-    EXPECT_TRUE(
-        resolve_host_clients_.emplace(host, std::move(response_client)).second);
+    network::mojom::ResolveHostHandleRequest control_handle_request;
+    if (optional_parameters)
+      control_handle_request = std::move(optional_parameters->control_handle);
+    auto test_client = std::make_unique<TestResolveHostClient>(
+        std::move(response_client), std::move(control_handle_request),
+        base::BindOnce(&MockNetworkContext::CompleteHostLookup,
+                       base::Unretained(this), host));
+    EXPECT_TRUE(resolve_host_clients_
+                    .insert(std::make_pair(host, std::move(test_client)))
+                    .second);
     ResolveHostProxy(host);
   }
 
@@ -92,10 +125,10 @@ class MockNetworkContext : public network::TestNetworkContext {
       ADD_FAILURE() << host << " wasn't found";
       return;
     }
-    it->second->OnComplete(result, base::nullopt);
-    resolve_host_clients_.erase(it);
+    it->second->OnComplete(result);
     // Wait for OnComplete() to be executed on the UI thread.
     base::RunLoop().RunUntilIdle();
+    resolve_host_clients_.erase(it);
   }
 
   void CompleteProxyLookup(const GURL& url,
@@ -119,7 +152,7 @@ class MockNetworkContext : public network::TestNetworkContext {
                     bool privacy_mode_enabled));
 
  private:
-  std::map<std::string, network::mojom::ResolveHostClientPtr>
+  std::map<std::string, std::unique_ptr<TestResolveHostClient>>
       resolve_host_clients_;
   std::map<GURL, network::mojom::ProxyLookupClientPtr> proxy_lookup_clients_;
 };
@@ -379,8 +412,6 @@ TEST_F(PreconnectManagerTest, TestSuccessfulProxyLookup) {
                                              GetIndirectProxyInfo());
   // We should preconnect socket before the host lookup is complete.
   Mock::VerifyAndClearExpectations(mock_network_context_.get());
-
-  mock_network_context_->CompleteHostLookup(url_to_preconnect.host(), net::OK);
 }
 
 TEST_F(PreconnectManagerTest, TestSuccessfulProxyLookupAfterPreresolveFailure) {

chrome/browser/predictors/resolve_host_client_impl.cc

@@ -27,6 +27,7 @@ ResolveHostClientImpl::ResolveHostClientImpl(
       network::mojom::ResolveHostParameters::New();
   parameters->initial_priority = net::RequestPriority::IDLE;
   parameters->is_speculative = true;
+  parameters->control_handle = mojo::MakeRequest(&control_handle_);
   network_context->ResolveHost(net::HostPortPair::FromURL(url),
                                std::move(parameters),
                                std::move(resolve_host_client_ptr));
@@ -36,6 +37,10 @@ ResolveHostClientImpl::ResolveHostClientImpl(
 
 ResolveHostClientImpl::~ResolveHostClientImpl() = default;
 
+void ResolveHostClientImpl::Cancel() {
+  control_handle_->Cancel(net::ERR_ABORTED);
+}
+
 void ResolveHostClientImpl::OnComplete(
     int result,
     const base::Optional<net::AddressList>& resolved_addresses) {

chrome/browser/predictors/resolve_host_client_impl.h

@@ -37,6 +37,8 @@ class ResolveHostClientImpl : public network::mojom::ResolveHostClient {
   // Cancels the request if it hasn't been completed yet.
   ~ResolveHostClientImpl() override;
 
+  void Cancel();
+
   // network::mojom::ResolveHostClient:
   void OnComplete(
       int result,
@@ -46,6 +48,7 @@ class ResolveHostClientImpl : public network::mojom::ResolveHostClient {
 
  private:
   mojo::Binding<network::mojom::ResolveHostClient> binding_;
+  network::mojom::ResolveHostHandlePtr control_handle_;
   ResolveHostCallback callback_;
 
   DISALLOW_COPY_AND_ASSIGN(ResolveHostClientImpl);