Reland "Convert dns_api.cc to mojo host resolver"

This is a reland of dccf844a

This was reverted because NetworkContextConfigurationBrowserTests
were failing on Mac due to crbug.com/843324. This is because those
tests call AddSimulatedFailure() on the host resolver, which now
sends those rules to NetworkServiceTest (which doesn't work on Mac).
I disabled the NetworkContextConfigurationBrowserTests on Mac that
run with network service enabled.

Original change's description:
> Convert dns_api.cc to mojo host resolver
>
> Bug: 874658
> Cq-Include-Trybots: luci.chromium.try:linux_mojo
> Change-Id: I576d2877ec71df8e489d349916f758ba6fe74263
> Reviewed-on: https://chromium-review.googlesource.com/1176725
> Reviewed-by: Will Harris <wfh@chromium.org>
> Reviewed-by: John Abd-El-Malek <jam@chromium.org>
> Commit-Queue: Clark DuVall <cduvall@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#583878}

TBR=wfh@chromium.org,jam@chromium.org

Bug: 874658
Change-Id: Ib26c612a450a48818a10f817bd2d309292d4c9ea
Cq-Include-Trybots: luci.chromium.try:linux_mojo
Reviewed-on: https://chromium-review.googlesource.com/1179922Reviewed-by: Clark DuVall <cduvall@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Commit-Queue: Clark DuVall <cduvall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#584122}

chrome/browser/net/network_context_configuration_browsertest.cc

@@ -404,9 +404,7 @@ class NetworkContextConfigurationBrowserTest
       ASSERT_TRUE(simple_loader_helper.response_body());
       EXPECT_EQ(*simple_loader_helper.response_body(), "Echo");
     } else {
-      // TestHostResolver returns net::ERR_NOT_IMPLEMENTED for non-local host
-      // URLs.
-      EXPECT_EQ(net::ERR_NOT_IMPLEMENTED, simple_loader->NetError());
+      EXPECT_EQ(net::ERR_NAME_NOT_RESOLVED, simple_loader->NetError());
       ASSERT_FALSE(simple_loader_helper.response_body());
     }
   }
@@ -1213,16 +1211,7 @@ IN_PROC_BROWSER_TEST_P(NetworkContextConfigurationBrowserTest,
   EXPECT_FALSE(GetCookies(embedded_test_server()->base_url()).empty());
 }
 
-#if defined(OS_MACOSX)
-// Disable the test on Mac OSX since it fails on the bot.
-// (https://crbug.com/847555)
-#define MAYBE_CookiesEnabled DISABLED_CookiesEnabled
-#else
-#define MAYBE_CookiesEnabled CookiesEnabled
-#endif
-
-IN_PROC_BROWSER_TEST_P(NetworkContextConfigurationBrowserTest,
-                       MAYBE_CookiesEnabled) {
+IN_PROC_BROWSER_TEST_P(NetworkContextConfigurationBrowserTest, CookiesEnabled) {
   // Check that the cookie from the first stage of the test was / was not
   // preserved between browser restarts, as expected.
   bool has_cookies = !GetCookies(embedded_test_server()->base_url()).empty();
@@ -1681,7 +1670,7 @@ IN_PROC_BROWSER_TEST_P(NetworkContextConfigurationHttpsStrippingPacBrowserTest,
 }
 
 // |NetworkServiceTestHelper| doesn't work on browser_tests on OSX.
-// See https://crbug.com/757088
+// See https://crbug.com/843324
 #if defined(OS_MACOSX)
 
 #if BUILDFLAG(ENABLE_EXTENSIONS)
@@ -1689,23 +1678,17 @@ IN_PROC_BROWSER_TEST_P(NetworkContextConfigurationHttpsStrippingPacBrowserTest,
   INSTANTIATE_TEST_CASE_P(                                                \
       OnDiskApp, TestFixture,                                             \
       ::testing::Values(TestCase({NetworkServiceState::kDisabled,         \
-                                  NetworkContextType::kOnDiskApp}),       \
-                        TestCase({NetworkServiceState::kEnabled,          \
                                   NetworkContextType::kOnDiskApp})));     \
                                                                           \
   INSTANTIATE_TEST_CASE_P(                                                \
       InMemoryApp, TestFixture,                                           \
       ::testing::Values(TestCase({NetworkServiceState::kDisabled,         \
-                                  NetworkContextType::kInMemoryApp}),     \
-                        TestCase({NetworkServiceState::kEnabled,          \
                                   NetworkContextType::kInMemoryApp})));   \
                                                                           \
   INSTANTIATE_TEST_CASE_P(                                                \
       OnDiskAppWithIncognitoProfile, TestFixture,                         \
       ::testing::Values(                                                  \
           TestCase({NetworkServiceState::kDisabled,                       \
-                    NetworkContextType::kOnDiskAppWithIncognitoProfile}), \
-          TestCase({NetworkServiceState::kEnabled,                        \
                     NetworkContextType::kOnDiskAppWithIncognitoProfile})));
 #else  // !BUILDFLAG(ENABLE_EXTENSIONS)
 #define INSTANTIATE_EXTENSION_TESTS(TestFixture)
@@ -1719,29 +1702,21 @@ IN_PROC_BROWSER_TEST_P(NetworkContextConfigurationHttpsStrippingPacBrowserTest,
   INSTANTIATE_TEST_CASE_P(                                                 \
       SystemNetworkContext, TestFixture,                                   \
       ::testing::Values(TestCase({NetworkServiceState::kDisabled,          \
-                                  NetworkContextType::kSystem}),           \
-                        TestCase({NetworkServiceState::kEnabled,           \
                                   NetworkContextType::kSystem})));         \
                                                                            \
   INSTANTIATE_TEST_CASE_P(                                                 \
       SafeBrowsingNetworkContext, TestFixture,                             \
       ::testing::Values(TestCase({NetworkServiceState::kDisabled,          \
-                                  NetworkContextType::kSafeBrowsing}),     \
-                        TestCase({NetworkServiceState::kEnabled,           \
                                   NetworkContextType::kSystem})));         \
                                                                            \
   INSTANTIATE_TEST_CASE_P(                                                 \
       ProfileMainNetworkContext, TestFixture,                              \
       ::testing::Values(TestCase({NetworkServiceState::kDisabled,          \
-                                  NetworkContextType::kProfile}),          \
-                        TestCase({NetworkServiceState::kEnabled,           \
                                   NetworkContextType::kProfile})));        \
                                                                            \
   INSTANTIATE_TEST_CASE_P(                                                 \
       IncognitoProfileMainNetworkContext, TestFixture,                     \
       ::testing::Values(TestCase({NetworkServiceState::kDisabled,          \
-                                  NetworkContextType::kIncognitoProfile}), \
-                        TestCase({NetworkServiceState::kEnabled,           \
                                   NetworkContextType::kIncognitoProfile})))
 #else  // !defined(OS_MACOSX)
 // Instiates tests with a prefix indicating which NetworkContext is being

content/public/test/browser_test_base.cc

@@ -492,6 +492,20 @@ void BrowserTestBase::InitializeNetworkProcess() {
     // For now, this covers all the rules used in content's tests.
     // TODO(jam: expand this when we try to make browser_tests and
     // components_browsertests work.
+    if (rule.resolver_type ==
+        net::RuleBasedHostResolverProc::Rule::kResolverTypeFail) {
+      // The host "wpad" is added automatically in TestHostResolver, so we don't
+      // need to send it to NetworkServiceTest.
+      if (rule.host_pattern != "wpad") {
+        network::mojom::RulePtr mojo_rule = network::mojom::Rule::New();
+        mojo_rule->resolver_type =
+            network::mojom::ResolverType::kResolverTypeFail;
+        mojo_rule->host_pattern = rule.host_pattern;
+        mojo_rules.push_back(std::move(mojo_rule));
+      }
+      continue;
+    }
+
     if ((rule.resolver_type !=
              net::RuleBasedHostResolverProc::Rule::kResolverTypeSystem &&
          rule.resolver_type !=
@@ -500,6 +514,14 @@ void BrowserTestBase::InitializeNetworkProcess() {
         !!rule.latency_ms || rule.replacement.empty())
       continue;
     network::mojom::RulePtr mojo_rule = network::mojom::Rule::New();
+    if (rule.resolver_type ==
+        net::RuleBasedHostResolverProc::Rule::kResolverTypeSystem) {
+      mojo_rule->resolver_type =
+          network::mojom::ResolverType::kResolverTypeSystem;
+    } else {
+      mojo_rule->resolver_type =
+          network::mojom::ResolverType::kResolverTypeIPLiteral;
+    }
     mojo_rule->host_pattern = rule.host_pattern;
     mojo_rule->replacement = rule.replacement;
     mojo_rules.push_back(std::move(mojo_rule));

content/public/test/network_service_test_helper.cc

@@ -40,6 +40,19 @@
 
 namespace content {
 namespace {
+
+#define STATIC_ASSERT_ENUM(a, b)                            \
+  static_assert(static_cast<int>(a) == static_cast<int>(b), \
+                "mismatching enums: " #a)
+
+STATIC_ASSERT_ENUM(network::mojom::ResolverType::kResolverTypeFail,
+                   net::RuleBasedHostResolverProc::Rule::kResolverTypeFail);
+STATIC_ASSERT_ENUM(network::mojom::ResolverType::kResolverTypeSystem,
+                   net::RuleBasedHostResolverProc::Rule::kResolverTypeSystem);
+STATIC_ASSERT_ENUM(
+    network::mojom::ResolverType::kResolverTypeIPLiteral,
+    net::RuleBasedHostResolverProc::Rule::kResolverTypeIPLiteral);
+
 void CrashResolveHost(const std::string& host_to_crash,
                       const std::string& host) {
   if (host_to_crash == host)
@@ -68,8 +81,14 @@ class NetworkServiceTestHelper::NetworkServiceTestImpl
   void AddRules(std::vector<network::mojom::RulePtr> rules,
                 AddRulesCallback callback) override {
     for (const auto& rule : rules) {
-      test_host_resolver_.host_resolver()->AddRule(rule->host_pattern,
-                                                   rule->replacement);
+      if (rule->resolver_type ==
+          network::mojom::ResolverType::kResolverTypeFail) {
+        test_host_resolver_.host_resolver()->AddSimulatedFailure(
+            rule->host_pattern);
+      } else {
+        test_host_resolver_.host_resolver()->AddRule(rule->host_pattern,
+                                                     rule->replacement);
+      }
     }
     std::move(callback).Run();
   }

extensions/browser/api/dns/dns_api.cc

@@ -13,9 +13,6 @@
 #include "extensions/common/api/dns.h"
 #include "net/base/host_port_pair.h"
 #include "net/base/net_errors.h"
-#include "net/log/net_log_with_source.h"
-#include "net/url_request/url_request_context.h"
-#include "net/url_request/url_request_context_getter.h"
 
 using content::BrowserThread;
 using extensions::api::dns::ResolveCallbackResolveInfo;
@@ -24,79 +21,51 @@ namespace Resolve = extensions::api::dns::Resolve;
 
 namespace extensions {
 
-DnsResolveFunction::DnsResolveFunction()
-    : response_(false), addresses_(new net::AddressList) {}
+DnsResolveFunction::DnsResolveFunction() : binding_(this) {}
 
 DnsResolveFunction::~DnsResolveFunction() {}
 
 ExtensionFunction::ResponseAction DnsResolveFunction::Run() {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
   std::unique_ptr<Resolve::Params> params(Resolve::Params::Create(*args_));
   EXTENSION_FUNCTION_VALIDATE(params.get());
 
-  hostname_ = params->hostname;
-  url_request_context_getter_ =
-      content::BrowserContext::GetDefaultStoragePartition(browser_context())
-          ->GetURLRequestContext();
-
-  bool result = BrowserThread::PostTask(
-      BrowserThread::IO,
-      FROM_HERE,
-      base::Bind(&DnsResolveFunction::WorkOnIOThread, this));
-  DCHECK(result);
-  return RespondLater();
-}
-
-void DnsResolveFunction::WorkOnIOThread() {
-  DCHECK_CURRENTLY_ON(BrowserThread::IO);
-
-  net::HostResolver* host_resolver =
-      HostResolverWrapper::GetInstance()->GetHostResolver(
-          url_request_context_getter_.get());
-  DCHECK(host_resolver);
-
+  network::mojom::ResolveHostClientPtr client_ptr;
+  binding_.Bind(mojo::MakeRequest(&client_ptr));
+  binding_.set_connection_error_handler(
+      base::BindOnce(&DnsResolveFunction::OnComplete, base::Unretained(this),
+                     net::ERR_FAILED, base::nullopt));
   // Yes, we are passing zero as the port. There are some interesting but not
   // presently relevant reasons why HostResolver asks for the port of the
   // hostname you'd like to resolve, even though it doesn't use that value in
   // determining its answer.
-  net::HostPortPair host_port_pair(hostname_, 0);
-
-  net::HostResolver::RequestInfo request_info(host_port_pair);
-  int resolve_result = host_resolver->Resolve(
-      request_info, net::DEFAULT_PRIORITY, addresses_.get(),
-      base::Bind(&DnsResolveFunction::OnLookupFinished, this), &request_,
-      net::NetLogWithSource());
+  net::HostPortPair host_port_pair(params->hostname, 0);
+  content::BrowserContext::GetDefaultStoragePartition(browser_context())
+      ->GetNetworkContext()
+      ->ResolveHost(host_port_pair, nullptr, std::move(client_ptr));
 
-  // Balanced in OnLookupFinished.
+  // Balanced in OnComplete().
   AddRef();
-
-  if (resolve_result != net::ERR_IO_PENDING)
-    OnLookupFinished(resolve_result);
+  return RespondLater();
 }
 
-void DnsResolveFunction::RespondOnUIThread(
-    std::unique_ptr<base::ListValue> results) {
+void DnsResolveFunction::OnComplete(
+    int result,
+    const base::Optional<net::AddressList>& resolved_addresses) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  Respond(response_ ? ArgumentList(std::move(results))
-                    : Error(kUnknownErrorDoNotUse));
-}
-
-void DnsResolveFunction::OnLookupFinished(int resolve_result) {
-  auto resolve_info = std::make_unique<ResolveCallbackResolveInfo>();
-  resolve_info->result_code = resolve_result;
-  if (resolve_result == net::OK) {
-    DCHECK(!addresses_->empty());
-    resolve_info->address.reset(
-        new std::string(addresses_->front().ToStringWithoutPort()));
+  binding_.Close();
+
+  ResolveCallbackResolveInfo resolve_info;
+  resolve_info.result_code = result;
+  if (result == net::OK) {
+    DCHECK(resolved_addresses.has_value() && !resolved_addresses->empty());
+    resolve_info.address = std::make_unique<std::string>(
+        resolved_addresses->front().ToStringWithoutPort());
   }
-  response_ = true;
 
-  bool post_task_result = BrowserThread::PostTask(
-      BrowserThread::UI, FROM_HERE,
-      base::BindOnce(&DnsResolveFunction::RespondOnUIThread, this,
-                     Resolve::Results::Create(*resolve_info)));
-  DCHECK(post_task_result);
+  Respond(ArgumentList(Resolve::Results::Create(resolve_info)));
 
-  Release();  // Added in WorkOnIOThread().
+  Release();  // Added in Run().
 }
 
 }  // namespace extensions

extensions/browser/api/dns/dns_api.h

@@ -8,17 +8,16 @@
 #include <string>
 
 #include "extensions/browser/extension_function.h"
+#include "mojo/public/cpp/bindings/binding.h"
 #include "net/base/address_list.h"
 #include "net/base/completion_callback.h"
 #include "net/dns/host_resolver.h"
-
-namespace net {
-class URLRequestContextGetter;
-}
+#include "services/network/public/mojom/network_context.mojom.h"
 
 namespace extensions {
 
-class DnsResolveFunction : public UIThreadExtensionFunction {
+class DnsResolveFunction : public UIThreadExtensionFunction,
+                           public network::mojom::ResolveHostClient {
  public:
   DECLARE_EXTENSION_FUNCTION("dns.resolve", DNS_RESOLVE)
 
@@ -31,19 +30,14 @@ class DnsResolveFunction : public UIThreadExtensionFunction {
   ResponseAction Run() override;
 
  private:
-  void WorkOnIOThread();
-  void RespondOnUIThread(std::unique_ptr<base::ListValue> results);
-
-  void OnLookupFinished(int result);
-
-  std::string hostname_;
-
-  scoped_refptr<net::URLRequestContextGetter> url_request_context_getter_;
-
-  bool response_;  // The value sent in SendResponse().
-
-  std::unique_ptr<net::HostResolver::Request> request_;
-  std::unique_ptr<net::AddressList> addresses_;
+  // network::mojom::ResolveHostClient implementation:
+  void OnComplete(
+      int result,
+      const base::Optional<net::AddressList>& resolved_addresses) override;
+
+  // A reference to |this| must be taken while the request is being made on this
+  // binding so the object is alive when the request completes.
+  mojo::Binding<network::mojom::ResolveHostClient> binding_;
 };
 
 }  // namespace extensions

extensions/browser/api/dns/dns_apitest.cc

@@ -9,40 +9,29 @@
 #include "content/public/browser/notification_service.h"
 #include "content/public/test/test_utils.h"
 #include "extensions/browser/api/dns/dns_api.h"
-#include "extensions/browser/api/dns/host_resolver_wrapper.h"
-#include "extensions/browser/api/dns/mock_host_resolver_creator.h"
 #include "extensions/browser/api_test_utils.h"
 #include "extensions/browser/notification_types.h"
 #include "extensions/common/extension.h"
 #include "extensions/common/extension_builder.h"
 #include "extensions/shell/test/shell_apitest.h"
 #include "net/base/net_errors.h"
+#include "net/dns/mock_host_resolver.h"
 
+namespace extensions {
+namespace {
 using extensions::api_test_utils::RunFunctionAndReturnSingleResult;
 
-namespace extensions {
+constexpr char kHostname[] = "www.sowbug.com";
+constexpr char kAddress[] = "9.8.7.6";
+}  // namespace
 
 class DnsApiTest : public ShellApiTest {
- public:
-  DnsApiTest() : resolver_creator_(new MockHostResolverCreator()) {}
-
  private:
   void SetUpOnMainThread() override {
     ShellApiTest::SetUpOnMainThread();
-    HostResolverWrapper::GetInstance()->SetHostResolverForTesting(
-        resolver_creator_->CreateMockHostResolver());
+    host_resolver()->AddRule(kHostname, kAddress);
+    host_resolver()->AddSimulatedFailure("this.hostname.is.bogus");
   }
-
-  void TearDownOnMainThread() override {
-    HostResolverWrapper::GetInstance()->SetHostResolverForTesting(NULL);
-    resolver_creator_->DeleteMockHostResolver();
-    ShellApiTest::TearDownOnMainThread();
-  }
-
-  // The MockHostResolver asserts that it's used on the same thread on which
-  // it's created, which is actually a stronger rule than its real counterpart.
-  // But that's fine; it's good practice.
-  scoped_refptr<MockHostResolverCreator> resolver_creator_;
 };
 
 IN_PROC_BROWSER_TEST_F(DnsApiTest, DnsResolveIPLiteral) {
@@ -74,7 +63,7 @@ IN_PROC_BROWSER_TEST_F(DnsApiTest, DnsResolveHostname) {
   resolve_function->set_has_callback(true);
 
   std::string function_arguments("[\"");
-  function_arguments += MockHostResolverCreator::kHostname;
+  function_arguments += kHostname;
   function_arguments += "\"]";
   std::unique_ptr<base::Value> result(RunFunctionAndReturnSingleResult(
       resolve_function.get(), function_arguments, browser_context()));
@@ -87,7 +76,7 @@ IN_PROC_BROWSER_TEST_F(DnsApiTest, DnsResolveHostname) {
 
   std::string address;
   EXPECT_TRUE(dict->GetString("address", &address));
-  EXPECT_EQ(MockHostResolverCreator::kAddress, address);
+  EXPECT_EQ(kAddress, address);
 }
 
 IN_PROC_BROWSER_TEST_F(DnsApiTest, DnsExtension) {

services/network/public/mojom/network_service_test.mojom

@@ -8,7 +8,15 @@ import "services/network/public/mojom/network_change_manager.mojom";
 import "services/network/public/mojom/network_param.mojom";
 import "services/network/public/mojom/network_types.mojom";
 
+// Maps to net::RuleBasedHostResolverProc::Rule::ResolverType.
+enum ResolverType {
+  kResolverTypeFail,
+  kResolverTypeSystem,
+  kResolverTypeIPLiteral,
+};
+
 struct Rule {
+  ResolverType resolver_type;
   string host_pattern;
   string replacement;
 };