Revert "Convert dns_api.cc to mojo host resolver"

This reverts commit dccf844a.

Reason for revert: Broke some browser_tests on Mac.

https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Mac10.13%20Tests%20%28dbg%29/4738

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=jam@chromium.org,wfh@chromium.org,ericorth@chromium.org,cduvall@chromium.org

Change-Id: Id845006ad303fca63c7c681fd259d3db379dfddd
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 874658
Cq-Include-Trybots: luci.chromium.try:linux_mojo
Reviewed-on: https://chromium-review.googlesource.com/1179383Reviewed-by: Kentaro Hara <haraken@chromium.org>
Commit-Queue: Kentaro Hara <haraken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#583955}

chrome/browser/net/network_context_configuration_browsertest.cc

@@ -404,7 +404,9 @@ class NetworkContextConfigurationBrowserTest
       ASSERT_TRUE(simple_loader_helper.response_body());
       EXPECT_EQ(*simple_loader_helper.response_body(), "Echo");
     } else {
-      EXPECT_EQ(net::ERR_NAME_NOT_RESOLVED, simple_loader->NetError());
+      // TestHostResolver returns net::ERR_NOT_IMPLEMENTED for non-local host
+      // URLs.
+      EXPECT_EQ(net::ERR_NOT_IMPLEMENTED, simple_loader->NetError());
       ASSERT_FALSE(simple_loader_helper.response_body());
     }
   }

content/public/test/browser_test_base.cc

@@ -492,20 +492,6 @@ 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 !=
@@ -514,14 +500,6 @@ 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,19 +40,6 @@
 
 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)
@@ -81,15 +68,9 @@ class NetworkServiceTestHelper::NetworkServiceTestImpl
   void AddRules(std::vector<network::mojom::RulePtr> rules,
                 AddRulesCallback callback) override {
     for (const auto& rule : rules) {
-      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,6 +13,9 @@
 #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;
@@ -21,51 +24,79 @@ namespace Resolve = extensions::api::dns::Resolve;
 
 namespace extensions {
 
-DnsResolveFunction::DnsResolveFunction() : binding_(this) {}
+DnsResolveFunction::DnsResolveFunction()
+    : response_(false), addresses_(new net::AddressList) {}
 
 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());
 
-  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));
+  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);
+
   // 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(params->hostname, 0);
-  content::BrowserContext::GetDefaultStoragePartition(browser_context())
-      ->GetNetworkContext()
-      ->ResolveHost(host_port_pair, nullptr, std::move(client_ptr));
+  net::HostPortPair host_port_pair(hostname_, 0);
 
-  // Balanced in OnComplete().
+  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());
+
+  // Balanced in OnLookupFinished.
   AddRef();
-  return RespondLater();
+
+  if (resolve_result != net::ERR_IO_PENDING)
+    OnLookupFinished(resolve_result);
 }
 
-void DnsResolveFunction::OnComplete(
-    int result,
-    const base::Optional<net::AddressList>& resolved_addresses) {
+void DnsResolveFunction::RespondOnUIThread(
+    std::unique_ptr<base::ListValue> results) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  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());
+  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()));
   }
+  response_ = true;
 
-  Respond(ArgumentList(Resolve::Results::Create(resolve_info)));
+  bool post_task_result = BrowserThread::PostTask(
+      BrowserThread::UI, FROM_HERE,
+      base::BindOnce(&DnsResolveFunction::RespondOnUIThread, this,
+                     Resolve::Results::Create(*resolve_info)));
+  DCHECK(post_task_result);
 
-  Release();  // Added in Run().
+  Release();  // Added in WorkOnIOThread().
 }
 
 }  // namespace extensions

extensions/browser/api/dns/dns_api.h

@@ -8,16 +8,17 @@
 #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"
-#include "services/network/public/mojom/network_context.mojom.h"
+
+namespace net {
+class URLRequestContextGetter;
+}
 
 namespace extensions {
 
-class DnsResolveFunction : public UIThreadExtensionFunction,
-                           public network::mojom::ResolveHostClient {
+class DnsResolveFunction : public UIThreadExtensionFunction {
  public:
   DECLARE_EXTENSION_FUNCTION("dns.resolve", DNS_RESOLVE)
 
@@ -30,14 +31,19 @@ class DnsResolveFunction : public UIThreadExtensionFunction,
   ResponseAction Run() override;
 
  private:
-  // 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_;
+  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_;
 };
 
 }  // namespace extensions

extensions/browser/api/dns/dns_apitest.cc

@@ -9,29 +9,40 @@
 #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;
 
-constexpr char kHostname[] = "www.sowbug.com";
-constexpr char kAddress[] = "9.8.7.6";
-}  // namespace
+namespace extensions {
 
 class DnsApiTest : public ShellApiTest {
+ public:
+  DnsApiTest() : resolver_creator_(new MockHostResolverCreator()) {}
+
  private:
   void SetUpOnMainThread() override {
     ShellApiTest::SetUpOnMainThread();
-    host_resolver()->AddRule(kHostname, kAddress);
-    host_resolver()->AddSimulatedFailure("this.hostname.is.bogus");
+    HostResolverWrapper::GetInstance()->SetHostResolverForTesting(
+        resolver_creator_->CreateMockHostResolver());
   }
+
+  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) {
@@ -63,7 +74,7 @@ IN_PROC_BROWSER_TEST_F(DnsApiTest, DnsResolveHostname) {
   resolve_function->set_has_callback(true);
 
   std::string function_arguments("[\"");
-  function_arguments += kHostname;
+  function_arguments += MockHostResolverCreator::kHostname;
   function_arguments += "\"]";
   std::unique_ptr<base::Value> result(RunFunctionAndReturnSingleResult(
       resolve_function.get(), function_arguments, browser_context()));
@@ -76,7 +87,7 @@ IN_PROC_BROWSER_TEST_F(DnsApiTest, DnsResolveHostname) {
 
   std::string address;
   EXPECT_TRUE(dict->GetString("address", &address));
-  EXPECT_EQ(kAddress, address);
+  EXPECT_EQ(MockHostResolverCreator::kAddress, address);
 }
 
 IN_PROC_BROWSER_TEST_F(DnsApiTest, DnsExtension) {

services/network/public/mojom/network_service_test.mojom

@@ -8,15 +8,7 @@ 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;
 };