predictors: Issue a dns request only after a proxy lookup is complete

Active cancellation of dns requests may lead to creation of too many background
threads performing getaddrinfo. It happens because we can't cancel getaddrinfo,
and the task runner will allocate a new thread even though a thread with the
cancelled getaddrinfo is still alive.

This reverts commit 512b3dda and applies new
patches on top of this revert.

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

chrome/browser/predictors/preconnect_manager.cc

@@ -49,8 +49,7 @@ PreresolveJob::PreresolveJob(const GURL& url,
     : url(url),
       num_sockets(num_sockets),
       allow_credentials(allow_credentials),
-      info(info),
-      success(false) {
+      info(info) {
   DCHECK_GE(num_sockets, 0);
 }
 
@@ -191,8 +190,7 @@ std::unique_ptr<ProxyLookupClientImpl> PreconnectManager::LookupProxyForUrl(
 
   auto* network_context = GetNetworkContext();
   if (!network_context) {
-    // It's okay to not invoke the callback here because PreresolveUrl()
-    // callback will be invoked.
+    std::move(callback).Run(false);
     return nullptr;
   }
 
@@ -218,10 +216,6 @@ void PreconnectManager::TryToLaunchPreresolveJobs() {
       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));
       if (info)
         ++info->inflight_count;
       ++inflight_preresolves_count_;
@@ -244,8 +238,7 @@ void PreconnectManager::OnPreresolveFinished(PreresolveJobId job_id,
     observer_->OnPreresolveFinished(job->url, success);
 
   job->resolve_host_client = nullptr;
-  job->success = job->success || success;
-  FinishPreresolveJob(job_id);
+  FinishPreresolveJob(job_id, success);
 }
 
 void PreconnectManager::OnProxyLookupFinished(PreresolveJobId job_id,
@@ -258,27 +251,22 @@ void PreconnectManager::OnProxyLookupFinished(PreresolveJobId job_id,
     observer_->OnProxyLookupFinished(job->url, success);
 
   job->proxy_lookup_client = nullptr;
-  job->success = job->success || success;
-  if (job->success && job->resolve_host_client)
-    job->resolve_host_client->Cancel();
-  FinishPreresolveJob(job_id);
+  if (success) {
+    FinishPreresolveJob(job_id, success);
+  } else {
+    job->resolve_host_client = PreresolveUrl(
+        job->url, base::BindOnce(&PreconnectManager::OnPreresolveFinished,
+                                 weak_factory_.GetWeakPtr(), job_id));
+  }
 }
 
-void PreconnectManager::FinishPreresolveJob(PreresolveJobId job_id) {
+void PreconnectManager::FinishPreresolveJob(PreresolveJobId job_id,
+                                            bool success) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   PreresolveJob* job = preresolve_jobs_.Lookup(job_id);
   DCHECK(job);
 
-  // Always wait for the host resolution to be complete.
-  if (job->resolve_host_client)
-    return;
-
-  // 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();
+  bool need_preconnect = success && job->need_preconnect();
   if (need_preconnect)
     PreconnectUrl(job->url, job->num_sockets, job->allow_credentials);
 

chrome/browser/predictors/preconnect_manager.h

@@ -92,7 +92,6 @@ 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);
 };
@@ -180,7 +179,7 @@ class PreconnectManager {
   void TryToLaunchPreresolveJobs();
   void OnPreresolveFinished(PreresolveJobId job_id, bool success);
   void OnProxyLookupFinished(PreresolveJobId job_id, bool success);
-  void FinishPreresolveJob(PreresolveJobId job_id);
+  void FinishPreresolveJob(PreresolveJobId job_id, bool success);
   void AllPreresolvesForUrlFinished(PreresolveInfo* info);
   network::mojom::NetworkContext* GetNetworkContext() const;
 

chrome/browser/predictors/preconnect_manager_unittest.cc

@@ -20,7 +20,6 @@
 #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"
@@ -61,30 +60,6 @@ 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;
@@ -98,16 +73,8 @@ class MockNetworkContext : public network::TestNetworkContext {
       network::mojom::ResolveHostParametersPtr optional_parameters,
       network::mojom::ResolveHostClientPtr response_client) override {
     const std::string& host = host_port.host();
-    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);
+    EXPECT_TRUE(
+        resolve_host_clients_.emplace(host, std::move(response_client)).second);
     ResolveHostProxy(host);
   }
 
@@ -117,6 +84,10 @@ class MockNetworkContext : public network::TestNetworkContext {
     EXPECT_TRUE(
         proxy_lookup_clients_.emplace(url, std::move(proxy_lookup_client))
             .second);
+    if (!enabled_proxy_testing_) {
+      // We don't want to test proxy, return that the proxy is disabled.
+      CompleteProxyLookup(url, base::nullopt);
+    }
   }
 
   void CompleteHostLookup(const std::string& host, int result) {
@@ -125,10 +96,10 @@ class MockNetworkContext : public network::TestNetworkContext {
       ADD_FAILURE() << host << " wasn't found";
       return;
     }
-    it->second->OnComplete(result);
+    it->second->OnComplete(result, base::nullopt);
+    resolve_host_clients_.erase(it);
     // Wait for OnComplete() to be executed on the UI thread.
     base::RunLoop().RunUntilIdle();
-    resolve_host_clients_.erase(it);
   }
 
   void CompleteProxyLookup(const GURL& url,
@@ -144,6 +115,8 @@ class MockNetworkContext : public network::TestNetworkContext {
     base::RunLoop().RunUntilIdle();
   }
 
+  void EnableProxyTesting() { enabled_proxy_testing_ = true; }
+
   MOCK_METHOD1(ResolveHostProxy, void(const std::string& host));
   MOCK_METHOD4(PreconnectSockets,
                void(uint32_t num_streams,
@@ -152,9 +125,10 @@ class MockNetworkContext : public network::TestNetworkContext {
                     bool privacy_mode_enabled));
 
  private:
-  std::map<std::string, std::unique_ptr<TestResolveHostClient>>
+  std::map<std::string, network::mojom::ResolveHostClientPtr>
       resolve_host_clients_;
   std::map<GURL, network::mojom::ProxyLookupClientPtr> proxy_lookup_clients_;
+  bool enabled_proxy_testing_ = false;
 };
 
 class PreconnectManagerTest : public testing::Test {
@@ -397,35 +371,13 @@ TEST_F(PreconnectManagerTest, TestDetachedRequestHasHigherPriority) {
 }
 
 TEST_F(PreconnectManagerTest, TestSuccessfulProxyLookup) {
+  mock_network_context_->EnableProxyTesting();
   GURL main_frame_url("http://google.com");
   GURL url_to_preconnect("http://cdn.google.com");
 
-  EXPECT_CALL(*mock_network_context_,
-              ResolveHostProxy(url_to_preconnect.host()));
   preconnect_manager_->Start(main_frame_url,
                              {PreconnectRequest(url_to_preconnect, 1)});
 
-  EXPECT_CALL(*mock_network_context_,
-              PreconnectSockets(1, url_to_preconnect, kNormalLoadFlags, false));
-  EXPECT_CALL(*mock_delegate_, PreconnectFinishedProxy(main_frame_url));
-  mock_network_context_->CompleteProxyLookup(url_to_preconnect,
-                                             GetIndirectProxyInfo());
-  // We should preconnect socket before the host lookup is complete.
-  Mock::VerifyAndClearExpectations(mock_network_context_.get());
-}
-
-TEST_F(PreconnectManagerTest, TestSuccessfulProxyLookupAfterPreresolveFailure) {
-  GURL main_frame_url("http://google.com");
-  GURL url_to_preconnect("http://cdn.google.com");
-
-  EXPECT_CALL(*mock_network_context_,
-              ResolveHostProxy(url_to_preconnect.host()));
-  preconnect_manager_->Start(main_frame_url,
-                             {PreconnectRequest(url_to_preconnect, 1)});
-  mock_network_context_->CompleteHostLookup(url_to_preconnect.host(),
-                                            net::ERR_FAILED);
-  Mock::VerifyAndClearExpectations(mock_network_context_.get());
-
   EXPECT_CALL(*mock_network_context_,
               PreconnectSockets(1, url_to_preconnect, kNormalLoadFlags, false));
   EXPECT_CALL(*mock_delegate_, PreconnectFinishedProxy(main_frame_url));
@@ -434,17 +386,18 @@ TEST_F(PreconnectManagerTest, TestSuccessfulProxyLookupAfterPreresolveFailure) {
 }
 
 TEST_F(PreconnectManagerTest, TestSuccessfulHostLookupAfterProxyLookupFailure) {
+  mock_network_context_->EnableProxyTesting();
   GURL main_frame_url("http://google.com");
   GURL url_to_preconnect("http://cdn.google.com");
   GURL url_to_preconnect2("http://ads.google.com");
 
+  preconnect_manager_->Start(main_frame_url,
+                             {PreconnectRequest(url_to_preconnect, 1),
+                              PreconnectRequest(url_to_preconnect2, 1)});
   EXPECT_CALL(*mock_network_context_,
               ResolveHostProxy(url_to_preconnect.host()));
   EXPECT_CALL(*mock_network_context_,
               ResolveHostProxy(url_to_preconnect2.host()));
-  preconnect_manager_->Start(main_frame_url,
-                             {PreconnectRequest(url_to_preconnect, 1),
-                              PreconnectRequest(url_to_preconnect2, 1)});
   // First URL uses direct connection.
   mock_network_context_->CompleteProxyLookup(url_to_preconnect,
                                              GetDirectProxyInfo());
@@ -463,29 +416,21 @@ TEST_F(PreconnectManagerTest, TestSuccessfulHostLookupAfterProxyLookupFailure) {
 }
 
 TEST_F(PreconnectManagerTest, TestBothProxyAndHostLookupFailed) {
+  mock_network_context_->EnableProxyTesting();
   GURL main_frame_url("http://google.com");
   GURL url_to_preconnect("http://cdn.google.com");
-  GURL url_to_preconnect2("http://ads.google.com");
+
+  preconnect_manager_->Start(main_frame_url,
+                             {PreconnectRequest(url_to_preconnect, 1)});
 
   EXPECT_CALL(*mock_network_context_,
               ResolveHostProxy(url_to_preconnect.host()));
-  EXPECT_CALL(*mock_network_context_,
-              ResolveHostProxy(url_to_preconnect2.host()));
-  preconnect_manager_->Start(main_frame_url,
-                             {PreconnectRequest(url_to_preconnect, 1),
-                              PreconnectRequest(url_to_preconnect2, 1)});
-  // Test two failures in different order:
-  // proxy -> host for |url_to_preconnect|
-  // host -> proxy for |url_to_preconnect2|
   mock_network_context_->CompleteProxyLookup(url_to_preconnect, base::nullopt);
-  mock_network_context_->CompleteHostLookup(url_to_preconnect2.host(),
-                                            net::ERR_FAILED);
   Mock::VerifyAndClearExpectations(mock_network_context_.get());
 
   EXPECT_CALL(*mock_delegate_, PreconnectFinishedProxy(main_frame_url));
   mock_network_context_->CompleteHostLookup(url_to_preconnect.host(),
                                             net::ERR_FAILED);
-  mock_network_context_->CompleteProxyLookup(url_to_preconnect2, base::nullopt);
 }
 
 }  // namespace predictors

chrome/browser/predictors/resolve_host_client_impl.cc

@@ -27,7 +27,6 @@ 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));
@@ -37,10 +36,6 @@ 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,8 +37,6 @@ 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,
@@ -48,7 +46,6 @@ 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);