Modernize host resolution in proxy mojo resolve

Updated the resolve implementation in mojo_host_resolver_impl.cc to use
the newer HostResolver::CreateRequest() API.

Updated the mojo API to no longer use a serialized version of the
deprecated RequestInfo struct. As this mojo API is only used by one
special-purpose caller (HostResolverMojo), reduced parameters down to
just those parameters (hostname and an operation enum) used by that one
caller.

Also reduced the output of the mojo API down to just an array of
IPAddress as that is all that is used by the one caller.

Bug: 922699
Change-Id: Idec52470363981a34c8741a2012cb6eab218ffdb
Reviewed-on: https://chromium-review.googlesource.com/c/1446780
Commit-Queue: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Reviewed-by: Maks Orlovich <morlovich@chromium.org>
Auto-Submit: Eric Orth <ericorth@chromium.org>
Cr-Commit-Position: refs/heads/master@{#634377}

chrome/browser/net/chrome_mojo_proxy_resolver_factory_browsertest.cc

@@ -145,7 +145,8 @@ class DumbProxyResolverFactoryRequestClient
   void Alert(const std::string& error) override {}
   void OnError(int32_t line_number, const std::string& error) override {}
   void ResolveDns(
-      std::unique_ptr<net::HostResolver::RequestInfo> request_info,
+      const std::string& hostname,
+      net::ProxyResolveDnsOperation operation,
       proxy_resolver::mojom::HostResolverRequestClientPtr client) override {}
 
   proxy_resolver::mojom::ProxyResolverPtr resolver_;

content/test/proxy_service_mojo_unittest.cc

@@ -111,20 +111,6 @@ void CheckCapturedNetLogEntries(const net::TestNetLogEntry::List& entries) {
   EXPECT_EQ(3, line_number);
 }
 
-class LoggingMockHostResolver : public net::MockHostResolver {
- public:
-  int Resolve(const RequestInfo& info,
-              net::RequestPriority priority,
-              net::AddressList* addresses,
-              net::CompletionOnceCallback callback,
-              std::unique_ptr<Request>* out_req,
-              const net::NetLogWithSource& net_log) override {
-    net_log.AddEvent(net::NetLogEventType::HOST_RESOLVER_IMPL_JOB);
-    return net::MockHostResolver::Resolve(
-        info, priority, addresses, std::move(callback), out_req, net_log);
-  }
-};
-
 }  // namespace
 
 class ProxyServiceMojoTest : public testing::Test {
@@ -148,7 +134,7 @@ class ProxyServiceMojoTest : public testing::Test {
   base::test::ScopedTaskEnvironment task_environment_;
   content::TestMojoProxyResolverFactory test_mojo_proxy_resolver_factory_;
   TestNetworkDelegate network_delegate_;
-  LoggingMockHostResolver mock_host_resolver_;
+  net::MockHostResolver mock_host_resolver_;
   // Owned by |proxy_resolution_service_|.
   net::MockPacFileFetcher* fetcher_;
   net::TestNetLog net_log_;
@@ -179,12 +165,11 @@ TEST_F(ProxyServiceMojoTest, Basic) {
 TEST_F(ProxyServiceMojoTest, DnsResolution) {
   net::ProxyInfo info;
   net::TestCompletionCallback callback;
-  net::BoundTestNetLog test_net_log;
   std::unique_ptr<net::ProxyResolutionService::Request> request;
   EXPECT_EQ(net::ERR_IO_PENDING,
             proxy_resolution_service_->ResolveProxy(
                 GURL("http://foo"), std::string(), &info, callback.callback(),
-                &request, test_net_log.bound()));
+                &request, net::NetLogWithSource()));
 
   // PAC file fetcher should have a fetch triggered by the first
   // |ResolveProxy()| request.
@@ -197,16 +182,6 @@ TEST_F(ProxyServiceMojoTest, DnsResolution) {
   EXPECT_EQ("QUIC bar:4321", info.ToPacString());
   EXPECT_EQ(1u, mock_host_resolver_.num_resolve());
   proxy_resolution_service_.reset();
-
-  net::TestNetLogEntry::List entries;
-  test_net_log.GetEntries(&entries);
-  // There should be one entry with type TYPE_HOST_RESOLVER_IMPL_JOB.
-  EXPECT_EQ(1,
-            std::count_if(entries.begin(), entries.end(),
-                          [](const net::TestNetLogEntry& entry) {
-                            return entry.type ==
-                                   net::NetLogEventType::HOST_RESOLVER_IMPL_JOB;
-                          }));
 }
 
 TEST_F(ProxyServiceMojoTest, Error) {

services/network/mojo_host_resolver_impl.cc

@@ -5,12 +5,15 @@
 #include "services/network/mojo_host_resolver_impl.h"
 
 #include <utility>
+#include <vector>
 
 #include "base/bind.h"
-#include "net/base/address_list.h"
+#include "base/logging.h"
+#include "net/base/host_port_pair.h"
+#include "net/base/ip_address.h"
 #include "net/base/net_errors.h"
-#include "net/base/network_interfaces.h"
 #include "net/dns/host_resolver.h"
+#include "net/dns/public/dns_query_type.h"
 
 namespace network {
 
@@ -21,7 +24,8 @@ class MojoHostResolverImpl::Job {
  public:
   Job(MojoHostResolverImpl* resolver_service,
       net::HostResolver* resolver,
-      const net::HostResolver::RequestInfo& request_info,
+      const std::string& hostname,
+      bool is_ex,
       const net::NetLogWithSource& net_log,
       proxy_resolver::mojom::HostResolverRequestClientPtr client);
   ~Job();
@@ -41,12 +45,9 @@ class MojoHostResolverImpl::Job {
   // This Job's iterator in |resolver_service_|, so the Job may be removed on
   // completion.
   std::list<Job>::iterator iter_;
-  net::HostResolver* resolver_;
-  net::HostResolver::RequestInfo request_info_;
-  const net::NetLogWithSource net_log_;
   proxy_resolver::mojom::HostResolverRequestClientPtr client_;
-  std::unique_ptr<net::HostResolver::Request> request_;
-  net::AddressList result_;
+  const std::string hostname_;
+  std::unique_ptr<net::HostResolver::ResolveHostRequest> request_;
   base::ThreadChecker thread_checker_;
 };
 
@@ -59,17 +60,12 @@ MojoHostResolverImpl::~MojoHostResolverImpl() {
 }
 
 void MojoHostResolverImpl::Resolve(
-    std::unique_ptr<net::HostResolver::RequestInfo> request_info,
+    const std::string& hostname,
+    bool is_ex,
     proxy_resolver::mojom::HostResolverRequestClientPtr client) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  if (request_info->is_my_ip_address()) {
-    // The proxy resolver running inside a sandbox may not be able to get the
-    // correct host name. Instead, fill it ourself if the request is for our own
-    // IP address.
-    request_info->set_host_port_pair(net::HostPortPair(net::GetHostName(), 80));
-  }
 
-  pending_jobs_.emplace_front(this, resolver_, *request_info, net_log_,
+  pending_jobs_.emplace_front(this, resolver_, hostname, is_ex, net_log_,
                               std::move(client));
   auto job = pending_jobs_.begin();
   job->set_iter(job);
@@ -84,28 +80,30 @@ void MojoHostResolverImpl::DeleteJob(std::list<Job>::iterator job) {
 MojoHostResolverImpl::Job::Job(
     MojoHostResolverImpl* resolver_service,
     net::HostResolver* resolver,
-    const net::HostResolver::RequestInfo& request_info,
+    const std::string& hostname,
+    bool is_ex,
     const net::NetLogWithSource& net_log,
     proxy_resolver::mojom::HostResolverRequestClientPtr client)
     : resolver_service_(resolver_service),
-      resolver_(resolver),
-      request_info_(request_info),
-      net_log_(net_log),
-      client_(std::move(client)) {
+      client_(std::move(client)),
+      hostname_(hostname) {
   client_.set_connection_error_handler(base::Bind(
       &MojoHostResolverImpl::Job::OnConnectionError, base::Unretained(this)));
+
+  net::HostResolver::ResolveHostParameters parameters;
+  if (!is_ex)
+    parameters.dns_query_type = net::DnsQueryType::A;
+  request_ = resolver->CreateRequest(net::HostPortPair(hostname_, 0), net_log,
+                                     parameters);
 }
 
 void MojoHostResolverImpl::Job::Start() {
   // The caller is responsible for setting up |iter_|.
   DCHECK_EQ(this, &*iter_);
 
-  DVLOG(1) << "Resolve " << request_info_.host_port_pair().ToString();
-  int result =
-      resolver_->Resolve(request_info_, net::DEFAULT_PRIORITY, &result_,
-                         base::Bind(&MojoHostResolverImpl::Job::OnResolveDone,
-                                    base::Unretained(this)),
-                         &request_, net_log_);
+  DVLOG(1) << "Resolve " << hostname_;
+  int result = request_->Start(base::BindOnce(
+      &MojoHostResolverImpl::Job::OnResolveDone, base::Unretained(this)));
 
   if (result != net::ERR_IO_PENDING)
     OnResolveDone(result);
@@ -115,23 +113,31 @@ MojoHostResolverImpl::Job::~Job() = default;
 
 void MojoHostResolverImpl::Job::OnResolveDone(int result) {
   DCHECK(thread_checker_.CalledOnValidThread());
+
+  std::vector<net::IPAddress> result_addresses;
+  if (request_->GetAddressResults()) {
+    for (const auto& endpoint :
+         request_->GetAddressResults().value().endpoints()) {
+      result_addresses.push_back(endpoint.address());
+    }
+  }
+
   request_.reset();
-  DVLOG(1) << "Resolved " << request_info_.host_port_pair().ToString()
-           << " with error " << result << " and " << result_.size()
-           << " results!";
-  for (const auto& address : result_) {
+  DVLOG(1) << "Resolved " << hostname_ << " with error " << result << " and "
+           << result_addresses.size() << " results!";
+  for (const auto& address : result_addresses) {
     DVLOG(1) << address.ToString();
   }
-  client_->ReportResult(result, result_);
+  client_->ReportResult(result, result_addresses);
+
   resolver_service_->DeleteJob(iter_);
 }
 
 void MojoHostResolverImpl::Job::OnConnectionError() {
   DCHECK(thread_checker_.CalledOnValidThread());
   // |resolver_service_| should always outlive us.
   DCHECK(resolver_service_);
-  DVLOG(1) << "Connection error on request for "
-           << request_info_.host_port_pair().ToString();
+  DVLOG(1) << "Connection error on request for " << hostname_;
   resolver_service_->DeleteJob(iter_);
 }
 

services/network/mojo_host_resolver_impl.h

@@ -7,6 +7,7 @@
 
 #include <list>
 #include <memory>
+#include <string>
 
 #include "base/component_export.h"
 #include "base/macros.h"
@@ -36,7 +37,8 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) MojoHostResolverImpl {
                        const net::NetLogWithSource& net_log);
   ~MojoHostResolverImpl();
 
-  void Resolve(std::unique_ptr<net::HostResolver::RequestInfo> request_info,
+  void Resolve(const std::string& hostname,
+               bool is_ex,
                proxy_resolver::mojom::HostResolverRequestClientPtr client);
 
   bool request_in_progress() { return !pending_jobs_.empty(); }