Convert PepperTCPSocketMessageFilter to use the mojo host resolver.

Bug: 821021
Cq-Include-Trybots: luci.chromium.try:linux_mojo
Change-Id: Ic6422ecc5f2298c25a8720a7dfeac17114ede0ad
Reviewed-on: https://chromium-review.googlesource.com/1173492
Commit-Queue: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Reviewed-by: Will Harris <wfh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#583419}

chrome/test/ppapi/ppapi_browsertest.cc

@@ -27,13 +27,19 @@
 #include "components/nacl/common/nacl_switches.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/common/content_switches.h"
+#include "content/public/common/service_manager_connection.h"
+#include "content/public/common/service_names.mojom.h"
 #include "content/public/common/url_constants.h"
+#include "content/public/test/browser_test_utils.h"
 #include "content/public/test/javascript_test_observer.h"
 #include "content/public/test/test_renderer_host.h"
 #include "extensions/common/constants.h"
 #include "extensions/test/extension_test_message_listener.h"
 #include "ppapi/shared_impl/test_utils.h"
 #include "rlz/buildflags/buildflags.h"
+#include "services/network/public/cpp/features.h"
+#include "services/network/public/mojom/network_service_test.mojom.h"
+#include "services/service_manager/public/cpp/connector.h"
 
 #if defined(OS_MACOSX)
 #include "base/mac/mac_util.h"
@@ -319,6 +325,21 @@ TEST_PPAPI_NACL_WITH_SSL_SERVER(TCPSocketPrivate)
 
 TEST_PPAPI_OUT_OF_PROCESS_WITH_SSL_SERVER(TCPSocketPrivateTrusted)
 
+IN_PROC_BROWSER_TEST_F(OutOfProcessPPAPITest, TCPSocketPrivateCrash_Resolve) {
+  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(TCPSocketPrivateCrash_Resolve));
+}
+
 // UDPSocket tests.
 
 #define UDPSOCKET_TEST(_test) \

content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.cc

@@ -33,8 +33,6 @@
 #include "net/socket/ssl_client_socket.h"
 #include "net/socket/tcp_client_socket.h"
 #include "net/traffic_annotation/network_traffic_annotation.h"
-#include "net/url_request/url_request_context.h"
-#include "net/url_request/url_request_context_getter.h"
 #include "ppapi/host/dispatch_host_message.h"
 #include "ppapi/host/error_conversion.h"
 #include "ppapi/host/ppapi_host.h"
@@ -70,6 +68,7 @@ PepperTCPSocketMessageFilter::PepperTCPSocketMessageFilter(
       external_plugin_(host->external_plugin()),
       render_process_id_(0),
       render_frame_id_(0),
+      binding_(this),
       host_(host),
       factory_(factory),
       instance_(instance),
@@ -107,6 +106,7 @@ PepperTCPSocketMessageFilter::PepperTCPSocketMessageFilter(
       external_plugin_(host->external_plugin()),
       render_process_id_(0),
       render_frame_id_(0),
+      binding_(this),
       host_(host),
       factory_(nullptr),
       instance_(instance),
@@ -214,6 +214,20 @@ void PepperTCPSocketMessageFilter::OnHostDestroyed() {
   host_ = nullptr;
 }
 
+void PepperTCPSocketMessageFilter::OnComplete(
+    int result,
+    const base::Optional<net::AddressList>& resolved_addresses) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  binding_.Close();
+
+  BrowserThread::PostTask(
+      BrowserThread::IO, FROM_HERE,
+      base::BindOnce(&PepperTCPSocketMessageFilter::OnResolveCompleted, this,
+                     result, std::move(resolved_addresses)));
+
+  Release();  // Balances AddRef in OnMsgConnect.
+}
+
 int32_t PepperTCPSocketMessageFilter::OnMsgBind(
     const ppapi::host::HostMessageContext* context,
     const PP_NetAddress_Private& net_addr) {
@@ -267,13 +281,24 @@ int32_t PepperTCPSocketMessageFilter::OnMsgConnect(
   if (!render_process_host)
     return PP_ERROR_FAILED;
   auto* storage_partition = render_process_host->GetStoragePartition();
+  // 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(&PepperTCPSocketMessageFilter::OnComplete,
+                     base::Unretained(this), net::ERR_FAILED, base::nullopt));
+  storage_partition->GetNetworkContext()->ResolveHost(
+      net::HostPortPair(host, port), nullptr, std::move(client_ptr));
 
   BrowserThread::PostTask(
       BrowserThread::IO, FROM_HERE,
-      base::BindOnce(
-          &PepperTCPSocketMessageFilter::DoConnect, this,
-          context->MakeReplyMessageContext(), host, port,
-          base::WrapRefCounted(storage_partition->GetURLRequestContext())));
+      base::BindOnce(&PepperTCPSocketMessageFilter::HostResolvingStarted, this,
+                     context->MakeReplyMessageContext()));
   return PP_OK_COMPLETIONPENDING;
 }
 
@@ -614,43 +639,20 @@ void PepperTCPSocketMessageFilter::DoBind(
   state_.DoTransition(TCPSocketState::BIND, false);
 }
 
-void PepperTCPSocketMessageFilter::DoConnect(
-    const ppapi::host::ReplyMessageContext& context,
-    const std::string& host,
-    uint16_t port,
-    scoped_refptr<net::URLRequestContextGetter> url_request_context_getter) {
+void PepperTCPSocketMessageFilter::HostResolvingStarted(
+    const ppapi::host::ReplyMessageContext& context) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
-
   if (!state_.IsValidTransition(TCPSocketState::CONNECT)) {
-    SendConnectError(context, PP_ERROR_FAILED);
-    return;
-  }
-
-  auto* url_request_context =
-      url_request_context_getter->GetURLRequestContext();
-  if (!url_request_context) {
-    SendConnectError(context, PP_ERROR_FAILED);
-    return;
-  }
-
-  net::HostResolver* host_resolver = url_request_context->host_resolver();
-  if (!host_resolver) {
+    NOTREACHED() << "This shouldn't be reached since the renderer only tries "
+                 << "to connect once.";
     SendConnectError(context, PP_ERROR_FAILED);
     return;
   }
 
   state_.SetPendingTransition(TCPSocketState::CONNECT);
   address_index_ = 0;
   address_list_.clear();
-  net::HostResolver::RequestInfo request_info(net::HostPortPair(host, port));
-
-  int net_result = host_resolver->Resolve(
-      request_info, net::DEFAULT_PRIORITY, &address_list_,
-      base::BindOnce(&PepperTCPSocketMessageFilter::OnResolveCompleted,
-                     base::Unretained(this), context),
-      &request_, net::NetLogWithSource());
-  if (net_result != net::ERR_IO_PENDING)
-    OnResolveCompleted(context, net_result);
+  host_resolve_context_ = context;
 }
 
 void PepperTCPSocketMessageFilter::DoConnectWithNetAddress(
@@ -772,9 +774,14 @@ void PepperTCPSocketMessageFilter::DoListen(
 }
 
 void PepperTCPSocketMessageFilter::OnResolveCompleted(
-    const ppapi::host::ReplyMessageContext& context,
-    int net_result) {
+    int net_result,
+    const base::Optional<net::AddressList>& resolved_addresses) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  if (!host_resolve_context_.is_valid())
+    return;
+
+  ppapi::host::ReplyMessageContext context = host_resolve_context_;
+  host_resolve_context_ = ppapi::host::ReplyMessageContext();
 
   if (!state_.IsPending(TCPSocketState::CONNECT)) {
     DCHECK(state_.state() == TCPSocketState::CLOSED);
@@ -788,6 +795,7 @@ void PepperTCPSocketMessageFilter::OnResolveCompleted(
     return;
   }
 
+  address_list_ = resolved_addresses.value();
   StartConnect(context);
 }
 

content/browser/renderer_host/pepper/pepper_tcp_socket_message_filter.h

@@ -19,6 +19,7 @@
 #include "content/browser/renderer_host/pepper/browser_ppapi_host_impl.h"
 #include "content/browser/renderer_host/pepper/ssl_context_helper.h"
 #include "content/common/content_export.h"
+#include "mojo/public/cpp/bindings/binding.h"
 #include "net/base/address_list.h"
 #include "net/base/ip_endpoint.h"
 #include "net/dns/host_resolver.h"
@@ -28,6 +29,7 @@
 #include "ppapi/c/private/ppb_net_address_private.h"
 #include "ppapi/host/resource_message_filter.h"
 #include "ppapi/shared_impl/ppb_tcp_socket_shared.h"
+#include "services/network/public/mojom/network_context.mojom.h"
 
 #if defined(OS_CHROMEOS)
 #include "chromeos/network/firewall_hole.h"
@@ -38,7 +40,6 @@ namespace net {
 class DrainableIOBuffer;
 class IOBuffer;
 class SSLClientSocket;
-class URLRequestContextGetter;
 }
 
 namespace ppapi {
@@ -56,7 +57,8 @@ class ContentBrowserPepperHostFactory;
 
 class CONTENT_EXPORT PepperTCPSocketMessageFilter
     : public ppapi::host::ResourceMessageFilter,
-      public BrowserPpapiHostImpl::InstanceObserver {
+      public BrowserPpapiHostImpl::InstanceObserver,
+      public network::mojom::ResolveHostClient {
  public:
   PepperTCPSocketMessageFilter(ContentBrowserPepperHostFactory* factory,
                                BrowserPpapiHostImpl* host,
@@ -91,6 +93,11 @@ class CONTENT_EXPORT PepperTCPSocketMessageFilter
   void OnThrottleStateChanged(bool is_throttled) override;
   void OnHostDestroyed() override;
 
+  // network::mojom::ResolveHostClient overrides.
+  void OnComplete(
+      int result,
+      const base::Optional<net::AddressList>& resolved_addresses) override;
+
   int32_t OnMsgBind(const ppapi::host::HostMessageContext* context,
                     const PP_NetAddress_Private& net_addr);
   int32_t OnMsgConnect(const ppapi::host::HostMessageContext* context,
@@ -119,19 +126,16 @@ class CONTENT_EXPORT PepperTCPSocketMessageFilter
 
   void DoBind(const ppapi::host::ReplyMessageContext& context,
               const PP_NetAddress_Private& net_addr);
-  void DoConnect(
-      const ppapi::host::ReplyMessageContext& context,
-      const std::string& host,
-      uint16_t port,
-      scoped_refptr<net::URLRequestContextGetter> url_request_context_getter);
+  void HostResolvingStarted(const ppapi::host::ReplyMessageContext& context);
   void DoConnectWithNetAddress(const ppapi::host::ReplyMessageContext& context,
                                const PP_NetAddress_Private& net_addr);
   void DoWrite(const ppapi::host::ReplyMessageContext& context);
   void DoListen(const ppapi::host::ReplyMessageContext& context,
                 int32_t backlog);
 
-  void OnResolveCompleted(const ppapi::host::ReplyMessageContext& context,
-                          int net_result);
+  void OnResolveCompleted(
+      int net_result,
+      const base::Optional<net::AddressList>& resolved_addresses);
   void StartConnect(const ppapi::host::ReplyMessageContext& context);
 
   void OnConnectCompleted(const ppapi::host::ReplyMessageContext& context,
@@ -198,6 +202,10 @@ class CONTENT_EXPORT PepperTCPSocketMessageFilter
   int render_process_id_;
   int render_frame_id_;
 
+  // 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_;
+
   // The following fields are used only on the IO thread.
   // Non-owning ptr.
   BrowserPpapiHostImpl* host_;
@@ -237,6 +245,7 @@ class CONTENT_EXPORT PepperTCPSocketMessageFilter
   net::AddressList address_list_;
   // Where we are in the above list.
   size_t address_index_;
+  ppapi::host::ReplyMessageContext host_resolve_context_;
 
   // Non-null unless an SSL connection is requested.
   std::unique_ptr<net::TCPSocket> socket_;

content/public/test/network_service_test_helper.cc

@@ -26,6 +26,7 @@
 #include "net/test/embedded_test_server/embedded_test_server.h"
 #include "net/test/spawned_test_server/spawned_test_server.h"
 #include "net/test/test_data_directory.h"
+#include "services/network/host_resolver.h"
 #include "services/network/network_context.h"
 #include "services/network/network_service.h"
 #include "services/network/public/cpp/features.h"
@@ -38,6 +39,13 @@
 #endif
 
 namespace content {
+namespace {
+void CrashResolveHost(const std::string& host_to_crash,
+                      const std::string& host) {
+  if (host_to_crash == host)
+    base::Process::TerminateCurrentProcessImmediately(1);
+}
+}  // namespace
 
 class NetworkServiceTestHelper::NetworkServiceTestImpl
     : public network::mojom::NetworkServiceTest,
@@ -125,6 +133,11 @@ class NetworkServiceTestHelper::NetworkServiceTestImpl
     std::move(callback).Run();
   }
 
+  void CrashOnResolveHost(const std::string& host) override {
+    network::HostResolver::SetResolveHostCallbackForTesting(
+        base::BindRepeating(CrashResolveHost, host));
+  }
+
   void BindRequest(network::mojom::NetworkServiceTestRequest request) {
     bindings_.AddBinding(this, std::move(request));
     if (!registered_as_destruction_observer_) {

ppapi/BUILD.gn

@@ -104,6 +104,8 @@ test_common_source_files = [
   "tests/test_tcp_socket.h",
   "tests/test_tcp_socket_private.cc",
   "tests/test_tcp_socket_private.h",
+  "tests/test_tcp_socket_private_crash.cc",
+  "tests/test_tcp_socket_private_crash.h",
   "tests/test_test_internals.cc",
   "tests/test_test_internals.h",
   "tests/test_trace_event.cc",

ppapi/tests/test_tcp_socket_private_crash.cc

@@ -0,0 +1,45 @@
+// 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_tcp_socket_private_crash.h"
+
+#include <stddef.h>
+#include <stdlib.h>
+
+#include <new>
+
+#include "ppapi/cpp/private/tcp_socket_private.h"
+#include "ppapi/tests/test_utils.h"
+#include "ppapi/tests/testing_instance.h"
+
+REGISTER_TEST_CASE(TCPSocketPrivateCrash);
+
+TestTCPSocketPrivateCrash::TestTCPSocketPrivateCrash(TestingInstance* instance)
+    : TestCase(instance) {}
+
+bool TestTCPSocketPrivateCrash::Init() {
+  return pp::TCPSocketPrivate::IsAvailable();
+}
+
+void TestTCPSocketPrivateCrash::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(Resolve, filter);
+}
+
+std::string TestTCPSocketPrivateCrash::TestResolve() {
+  pp::TCPSocketPrivate socket(instance_);
+
+  TestCompletionCallback cb(instance_->pp_instance(), callback_type());
+
+  std::string host("crash.com");
+  cb.WaitForResult(socket.Connect(host.c_str(), 80, cb.GetCallback()));
+  ASSERT_EQ(PP_ERROR_FAILED, cb.result());
+
+  socket.Disconnect();
+
+  PASS();
+}

ppapi/tests/test_tcp_socket_private_crash.h

@@ -0,0 +1,25 @@
+// 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_TCP_SOCKET_PRIVATE_CRASH_H_
+#define PPAPI_TESTS_TEST_TCP_SOCKET_PRIVATE_CRASH_H_
+
+#include <string>
+
+#include "ppapi/c/pp_stdint.h"
+#include "ppapi/tests/test_case.h"
+
+class TestTCPSocketPrivateCrash : public TestCase {
+ public:
+  explicit TestTCPSocketPrivateCrash(TestingInstance* instance);
+
+  // TestCase implementation.
+  virtual bool Init();
+  virtual void RunTests(const std::string& filter);
+
+ private:
+  std::string TestResolve();
+};
+
+#endif  // PPAPI_TESTS_TEST_TCP_SOCKET_PRIVATE_CRASH_H_