Convert PepperHostResolverMessageFilter to use the mojo host resolver.

Bug: 821021
Change-Id: I420e1c9493b4eea33632e4b8b20c18b0a3abcb01
Reviewed-on: https://chromium-review.googlesource.com/1182009Reviewed-by: Doug Turner <dougt@chromium.org>
Commit-Queue: John Abd-El-Malek <jam@chromium.org>
Cr-Commit-Position: refs/heads/master@{#584780}

chrome/test/ppapi/ppapi_browsertest.cc

@@ -395,6 +395,21 @@ TEST_PPAPI_NACL_DISALLOWED_SOCKETS(UDPSocketPrivateDisallowed)
       LIST_TEST(HostResolver_ResolveIPv4) \
   )
 
+IN_PROC_BROWSER_TEST_F(OutOfProcessPPAPITest, HostResolverCrash_Basic) {
+  if (!base::FeatureList::IsEnabled(network::features::kNetworkService) ||
+      content::IsNetworkServiceRunningInProcess())
+    return;
+
+  network::mojom::NetworkServiceTestPtr network_service_test;
+  content::ServiceManagerConnection::GetForProcess()
+      ->GetConnector()
+      ->BindInterface(content::mojom::kNetworkServiceName,
+                      &network_service_test);
+  network_service_test->CrashOnResolveHost("crash.com");
+
+  RunTestViaHTTP(STRIP_PREFIXES(HostResolverCrash_Basic));
+}
+
 IN_PROC_BROWSER_TEST_F(OutOfProcessPPAPITest, HostResolver) {
   RUN_HOST_RESOLVER_SUBTESTS;
 }

content/browser/BUILD.gn

@@ -2010,7 +2010,6 @@ jumbo_source_set("browser") {
       "renderer_host/pepper/pepper_host_resolver_message_filter.h",
       "renderer_host/pepper/pepper_internal_file_ref_backend.cc",
       "renderer_host/pepper/pepper_internal_file_ref_backend.h",
-      "renderer_host/pepper/pepper_lookup_request.h",
       "renderer_host/pepper/pepper_message_filter.cc",
       "renderer_host/pepper/pepper_message_filter.h",
       "renderer_host/pepper/pepper_network_monitor_host.cc",

content/browser/renderer_host/pepper/pepper_host_resolver_message_filter.cc

@@ -10,7 +10,6 @@
 
 #include "base/logging.h"
 #include "content/browser/renderer_host/pepper/browser_ppapi_host_impl.h"
-#include "content/browser/renderer_host/pepper/pepper_lookup_request.h"
 #include "content/browser/renderer_host/pepper/pepper_socket_utils.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/browser_thread.h"
@@ -18,9 +17,6 @@
 #include "content/public/browser/storage_partition.h"
 #include "content/public/common/socket_permission_request.h"
 #include "net/base/address_list.h"
-#include "net/dns/host_resolver.h"
-#include "net/url_request/url_request_context.h"
-#include "net/url_request/url_request_context_getter.h"
 #include "ppapi/c/pp_errors.h"
 #include "ppapi/c/private/ppb_host_resolver_private.h"
 #include "ppapi/c/private/ppb_net_address_private.h"
@@ -38,28 +34,22 @@ namespace content {
 namespace {
 
 void PrepareRequestInfo(const PP_HostResolver_Private_Hint& hint,
-                        net::HostResolver::RequestInfo* request_info) {
-  DCHECK(request_info);
-
-  net::AddressFamily address_family;
+                        network::mojom::ResolveHostParameters* params) {
   switch (hint.family) {
     case PP_NETADDRESSFAMILY_PRIVATE_IPV4:
-      address_family = net::ADDRESS_FAMILY_IPV4;
+      params->dns_query_type = net::HostResolver::DnsQueryType::A;
       break;
     case PP_NETADDRESSFAMILY_PRIVATE_IPV6:
-      address_family = net::ADDRESS_FAMILY_IPV6;
+      params->dns_query_type = net::HostResolver::DnsQueryType::AAAA;
       break;
     default:
-      address_family = net::ADDRESS_FAMILY_UNSPECIFIED;
+      params->dns_query_type = net::HostResolver::DnsQueryType::UNSPECIFIED;
   }
-  request_info->set_address_family(address_family);
 
-  net::HostResolverFlags host_resolver_flags = 0;
   if (hint.flags & PP_HOST_RESOLVER_PRIVATE_FLAGS_CANONNAME)
-    host_resolver_flags |= net::HOST_RESOLVER_CANONNAME;
+    params->include_canonical_name = true;
   if (hint.flags & PP_HOST_RESOLVER_PRIVATE_FLAGS_LOOPBACK_ONLY)
-    host_resolver_flags |= net::HOST_RESOLVER_LOOPBACK_ONLY;
-  request_info->set_host_resolver_flags(host_resolver_flags);
+    params->loopback_only = true;
 }
 
 void CreateNetAddressListFromAddressList(
@@ -90,7 +80,8 @@ PepperHostResolverMessageFilter::PepperHostResolverMessageFilter(
     : external_plugin_(host->external_plugin()),
       private_api_(private_api),
       render_process_id_(0),
-      render_frame_id_(0) {
+      render_frame_id_(0),
+      binding_(this) {
   DCHECK(host);
 
   if (!host->GetRenderFrameIDsForInstance(
@@ -142,63 +133,56 @@ int32_t PepperHostResolverMessageFilter::OnMsgResolve(
     return PP_ERROR_FAILED;
   auto* storage_partition = render_process_host->GetStoragePartition();
 
-  BrowserThread::PostTask(
-      BrowserThread::IO, FROM_HERE,
-      base::BindOnce(
-          &PepperHostResolverMessageFilter::DoResolve, this,
-          context->MakeReplyMessageContext(), host_port, hint,
-          base::WrapRefCounted(storage_partition->GetURLRequestContext())));
+  // Grab a reference to this class to ensure that it's fully alive if a
+  // connection error occurs (i.e. ref count is higher than 0 and there's no
+  // task from ResourceMessageFilterDeleteTraits to delete this object on the IO
+  // thread pending). Balanced in OnComplete();
+  AddRef();
+
+  network::mojom::ResolveHostClientPtr client_ptr;
+  binding_.Bind(mojo::MakeRequest(&client_ptr));
+  binding_.set_connection_error_handler(
+      base::BindOnce(&PepperHostResolverMessageFilter::OnComplete,
+                     base::Unretained(this), net::ERR_FAILED, base::nullopt));
+
+  network::mojom::ResolveHostParametersPtr parameters =
+      network::mojom::ResolveHostParameters::New();
+  PrepareRequestInfo(hint, parameters.get());
+
+  storage_partition->GetNetworkContext()->ResolveHost(
+      net::HostPortPair(host_port.host, host_port.port), std::move(parameters),
+      std::move(client_ptr));
+  host_resolve_context_ = context->MakeReplyMessageContext();
+
   return PP_OK_COMPLETIONPENDING;
 }
 
-void PepperHostResolverMessageFilter::DoResolve(
-    const ReplyMessageContext& context,
-    const ppapi::HostPortPair& host_port,
-    const PP_HostResolver_Private_Hint& hint,
-    scoped_refptr<net::URLRequestContextGetter> url_request_context_getter) {
-  DCHECK_CURRENTLY_ON(BrowserThread::IO);
-
-  auto* url_request_context =
-      url_request_context_getter->GetURLRequestContext();
-  if (!url_request_context) {
-    SendResolveError(PP_ERROR_FAILED, context);
-    return;
-  }
+void PepperHostResolverMessageFilter::OnComplete(
+    int result,
+    const base::Optional<net::AddressList>& resolved_addresses) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  binding_.Close();
 
-  net::HostResolver* host_resolver = url_request_context->host_resolver();
-  if (!host_resolver) {
-    SendResolveError(PP_ERROR_FAILED, context);
-    return;
-  }
+  BrowserThread::PostTask(
+      BrowserThread::IO, FROM_HERE,
+      base::BindOnce(&PepperHostResolverMessageFilter::OnLookupFinished, this,
+                     result, std::move(resolved_addresses),
+                     host_resolve_context_));
+  host_resolve_context_ = ppapi::host::ReplyMessageContext();
 
-  net::HostResolver::RequestInfo request_info(
-      net::HostPortPair(host_port.host, host_port.port));
-  PrepareRequestInfo(hint, &request_info);
-
-  std::unique_ptr<ReplyMessageContext> bound_info(
-      new ReplyMessageContext(context));
-
-  // The lookup request will delete itself on completion.
-  PepperLookupRequest<ReplyMessageContext>* lookup_request =
-      new PepperLookupRequest<ReplyMessageContext>(
-          host_resolver,
-          request_info,
-          net::DEFAULT_PRIORITY,
-          bound_info.release(),
-          base::Bind(&PepperHostResolverMessageFilter::OnLookupFinished, this));
-  lookup_request->Start();
+  Release();  // Balances AddRef in OnMsgResolve.
 }
 
 void PepperHostResolverMessageFilter::OnLookupFinished(
     int net_result,
-    const net::AddressList& addresses,
+    const base::Optional<net::AddressList>& addresses,
     const ReplyMessageContext& context) {
   if (net_result != net::OK) {
     SendResolveError(NetErrorToPepperError(net_result), context);
   } else {
-    const std::string& canonical_name = addresses.canonical_name();
+    const std::string& canonical_name = addresses.value().canonical_name();
     NetAddressList net_address_list;
-    CreateNetAddressListFromAddressList(addresses, &net_address_list);
+    CreateNetAddressListFromAddressList(addresses.value(), &net_address_list);
     if (net_address_list.empty())
       SendResolveError(PP_ERROR_FAILED, context);
     else

content/browser/renderer_host/pepper/pepper_host_resolver_message_filter.h

@@ -14,15 +14,16 @@
 #include "base/macros.h"
 #include "content/common/content_export.h"
 #include "content/public/common/process_type.h"
+#include "mojo/public/cpp/bindings/binding.h"
 #include "ppapi/c/pp_instance.h"
 #include "ppapi/host/resource_message_filter.h"
+#include "services/network/public/mojom/network_context.mojom.h"
 
 struct PP_HostResolver_Private_Hint;
 struct PP_NetAddress_Private;
 
 namespace net {
 class AddressList;
-class URLRequestContextGetter;
 }
 
 namespace ppapi {
@@ -38,7 +39,8 @@ namespace content {
 class BrowserPpapiHostImpl;
 
 class CONTENT_EXPORT PepperHostResolverMessageFilter
-    : public ppapi::host::ResourceMessageFilter {
+    : public ppapi::host::ResourceMessageFilter,
+      public network::mojom::ResolveHostClient {
  public:
   PepperHostResolverMessageFilter(BrowserPpapiHostImpl* host,
                                   PP_Instance instance,
@@ -61,16 +63,13 @@ class CONTENT_EXPORT PepperHostResolverMessageFilter
                        const ppapi::HostPortPair& host_port,
                        const PP_HostResolver_Private_Hint& hint);
 
-  // Backend for OnMsgResolve(). Delegates host resolution to the
-  // Browser's HostResolver. Must be called on the IO thread.
-  void DoResolve(
-      const ppapi::host::ReplyMessageContext& context,
-      const ppapi::HostPortPair& host_port,
-      const PP_HostResolver_Private_Hint& hint,
-      scoped_refptr<net::URLRequestContextGetter> url_request_context_getter);
+  // network::mojom::ResolveHostClient overrides.
+  void OnComplete(
+      int result,
+      const base::Optional<net::AddressList>& resolved_addresses) override;
 
   void OnLookupFinished(int net_result,
-                        const net::AddressList& addresses,
+                        const base::Optional<net::AddressList>& addresses,
                         const ppapi::host::ReplyMessageContext& bound_info);
   void SendResolveReply(int32_t result,
                         const std::string& canonical_name,
@@ -79,11 +78,21 @@ class CONTENT_EXPORT PepperHostResolverMessageFilter
   void SendResolveError(int32_t error,
                         const ppapi::host::ReplyMessageContext& context);
 
+  // The following members are only accessed on the IO thread.
+
   bool external_plugin_;
   bool private_api_;
   int render_process_id_;
   int render_frame_id_;
 
+  // The following members are only accessed on the UI thread.
+
+  // A reference to |this| must always be taken while |binding_| is bound to
+  // ensure that if the error callback is called the object is alive.
+  mojo::Binding<network::mojom::ResolveHostClient> binding_;
+
+  ppapi::host::ReplyMessageContext host_resolve_context_;
+
   DISALLOW_COPY_AND_ASSIGN(PepperHostResolverMessageFilter);
 };
 

content/browser/renderer_host/pepper/pepper_lookup_request.h

-// Copyright (c) 2012 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#ifndef CONTENT_BROWSER_RENDERER_HOST_PEPPER_PEPPER_LOOKUP_REQUEST_H_
-#define CONTENT_BROWSER_RENDERER_HOST_PEPPER_PEPPER_LOOKUP_REQUEST_H_
-
-#include <memory>
-
-#include "base/callback.h"
-#include "base/macros.h"
-#include "net/base/address_list.h"
-#include "net/base/net_errors.h"
-#include "net/dns/host_resolver.h"
-#include "net/log/net_log_with_source.h"
-
-namespace content {
-
-template <class T>
-class PepperLookupRequest {
- public:
-  typedef base::Callback<void(int, const net::AddressList&, const T&)>
-      LookupRequestCallback;
-
-  // Takes ownership over |bound_info|. |bound_info| will be passed to
-  // callback, when lookup will finish.
-  PepperLookupRequest(net::HostResolver* resolver,
-                      const net::HostResolver::RequestInfo& request_info,
-                      net::RequestPriority priority,
-                      T* bound_info,
-                      const LookupRequestCallback& callback)
-      : resolver_(resolver),
-        request_info_(request_info),
-        priority_(priority),
-        bound_info_(bound_info),
-        callback_(callback) {}
-
-  void Start() {
-    int result = resolver_->Resolve(
-        request_info_, priority_, &addresses_,
-        base::BindOnce(&PepperLookupRequest<T>::OnLookupFinished,
-                       base::Unretained(this)),
-        &request_, net::NetLogWithSource());
-    if (result != net::ERR_IO_PENDING)
-      OnLookupFinished(result);
-  }
-
- private:
-  void OnLookupFinished(int result) {
-    callback_.Run(result, addresses_, *bound_info_);
-    delete this;
-  }
-
-  net::HostResolver* resolver_;
-  std::unique_ptr<net::HostResolver::Request> request_;
-  net::HostResolver::RequestInfo request_info_;
-  net::RequestPriority priority_;
-  std::unique_ptr<T> bound_info_;
-  LookupRequestCallback callback_;
-
-  net::AddressList addresses_;
-
-  DISALLOW_COPY_AND_ASSIGN(PepperLookupRequest);
-};
-
-}  // namespace content
-
-#endif  // CONTENT_BROWSER_RENDERER_HOST_PEPPER_PEPPER_LOOKUP_REQUEST_H_

ppapi/BUILD.gn

@@ -60,6 +60,8 @@ test_common_source_files = [
   "tests/test_graphics_3d.h",
   "tests/test_host_resolver.cc",
   "tests/test_host_resolver.h",
+  "tests/test_host_resolver_crash.cc",
+  "tests/test_host_resolver_crash.h",
   "tests/test_host_resolver_private.cc",
   "tests/test_host_resolver_private.h",
   "tests/test_host_resolver_private_disallowed.cc",

ppapi/tests/test_host_resolver_crash.cc

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "ppapi/tests/test_host_resolver_crash.h"
+
+#include <stddef.h>
+
+#include "ppapi/cpp/host_resolver.h"
+#include "ppapi/tests/test_utils.h"
+#include "ppapi/tests/testing_instance.h"
+
+REGISTER_TEST_CASE(HostResolverCrash);
+
+TestHostResolverCrash::TestHostResolverCrash(TestingInstance* instance)
+    : TestCase(instance) {}
+
+bool TestHostResolverCrash::Init() {
+  return pp::HostResolver::IsAvailable();
+}
+
+void TestHostResolverCrash::RunTests(const std::string& filter) {
+  // No need to run this test with the various callback types since that's
+  // orthogonal from the functionality being tested. It would also make the
+  // test more complicated because it would have to keep watching the network
+  // process restart and telling it to crash again on crash.com.
+  RUN_TEST(Basic, filter);
+}
+
+std::string TestHostResolverCrash::TestBasic() {
+  pp::HostResolver host_resolver(instance_);
+
+  PP_HostResolver_Hint hint;
+  hint.family = PP_NETADDRESS_FAMILY_UNSPECIFIED;
+  hint.flags = PP_HOSTRESOLVER_FLAG_CANONNAME;
+
+  TestCompletionCallback callback(instance_->pp_instance(), callback_type());
+  std::string host("crash.com");
+  callback.WaitForResult(
+      host_resolver.Resolve(host.c_str(), 80, hint, callback.GetCallback()));
+  ASSERT_EQ(PP_ERROR_FAILED, callback.result());
+
+  PASS();
+}

ppapi/tests/test_host_resolver_crash.h

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef PPAPI_TESTS_TEST_HOST_RESOLVER_CRASH_H_
+#define PPAPI_TESTS_TEST_HOST_RESOLVER_CRASH_H_
+
+#include <string>
+
+#include "ppapi/c/pp_stdint.h"
+#include "ppapi/tests/test_case.h"
+
+class TestHostResolverCrash : public TestCase {
+ public:
+  explicit TestHostResolverCrash(TestingInstance* instance);
+
+  // TestCase implementation.
+  virtual bool Init();
+  virtual void RunTests(const std::string& filter);
+
+ private:
+  std::string TestBasic();
+};
+
+#endif  // PPAPI_TESTS_TEST_HOST_RESOLVER_CRASH_H_