jingle_glue: Add a NetworkService equivalent of ChromeAsyncSocket

The former will have some problems with some proxy setups once the Network Service
is on, so this introduces a replacement. This CL doesn't hook it up to anything,
just builds & tests it. Next steps would require an equivalent to FakeSSLClientSocket,
and then actually hooking those up and wiring the different injected config from
Chrome.

To ease the review of this somewhat, this CL was split in two, with
https://chromium-review.googlesource.com/c/chromium/src/+/1237214
just copying over ChomeAsyncSocket and its tests to new files, to produce something of
a meaningful diff. (Thanks to lassey@ for the idea)

Bug: 875032
Change-Id: Ic5d4e4276c993e0e70f059d485b9d34aeab41e6d
Reviewed-on: https://chromium-review.googlesource.com/c/1232034
Commit-Queue: Maks Orlovich <morlovich@chromium.org>
Reviewed-by: Ken Rockot <rockot@google.com>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Reviewed-by: Nicolas Zea <zea@chromium.org>
Cr-Commit-Position: refs/heads/master@{#604490}

jingle/BUILD.gn

@@ -9,6 +9,8 @@ static_library("jingle_glue") {
   sources = [
     "glue/chrome_async_socket.cc",
     "glue/chrome_async_socket.h",
+    "glue/network_service_async_socket.cc",
+    "glue/network_service_async_socket.h",
     "glue/resolving_client_socket_factory.h",
     "glue/task_pump.cc",
     "glue/task_pump.h",
@@ -28,6 +30,7 @@ static_library("jingle_glue") {
     "//base/third_party/dynamic_annotations",
     "//net",
     "//services/network:network_service",
+    "//services/network/public/mojom",
     "//third_party/libjingle_xmpp:rtc_task_runner",
   ]
 
@@ -36,9 +39,13 @@ static_library("jingle_glue") {
   if (is_nacl) {
     sources -= [
       "glue/chrome_async_socket.cc",
+      "glue/network_service_async_socket.cc",
       "glue/xmpp_client_socket_factory.cc",
     ]
-    deps -= [ "//services/network:network_service" ]
+    deps -= [
+      "//services/network:network_service",
+      "//services/network/public/mojom",
+    ]
   }
 }
 
@@ -117,8 +124,8 @@ static_library("notifier") {
     "//base",
     "//net",
     "//third_party/expat",
-    "//url",
     "//third_party/libjingle_xmpp:rtc_task_runner",
+    "//url",
   ]
 }
 
@@ -151,6 +158,7 @@ test("jingle_unittests") {
     "glue/logging_unittest.cc",
     "glue/mock_task.cc",
     "glue/mock_task.h",
+    "glue/network_service_async_socket_unittest.cc",
     "glue/task_pump_unittest.cc",
     "glue/thread_wrapper_unittest.cc",
     "notifier/base/weak_xmpp_client_unittest.cc",
@@ -166,6 +174,7 @@ test("jingle_unittests") {
     "notifier/listener/send_ping_task_unittest.cc",
     "notifier/listener/xml_element_util_unittest.cc",
     "notifier/listener/xmpp_push_client_unittest.cc",
+    "run_all_unittests.cc",
   ]
 
   if (is_android || is_ios) {
@@ -174,6 +183,7 @@ test("jingle_unittests") {
       # EXPECT_DEBUG_DEATH() uses features not enabled.
       # Should we -std=c++0x or -std=gnu++0x?
       "glue/chrome_async_socket_unittest.cc",
+      "glue/network_service_async_socket_unittest.cc",
       "notifier/base/xmpp_connection_unittest.cc",
     ]
   }
@@ -195,10 +205,13 @@ test("jingle_unittests") {
     ":notifier",
     ":notifier_test_util",
     "//base",
-    "//base/test:run_all_unittests",
     "//base/test:test_support",
+    "//mojo/core/embedder",
+    "//mojo/public/cpp/bindings",
+    "//mojo/public/cpp/system",
     "//net",
     "//net:test_support",
+    "//services/network:network_service",
     "//services/network:test_support",
     "//testing/gmock",
     "//testing/gtest",

jingle/DEPS

 include_rules = [
+  "+mojo/core/embedder",
   "+net",
   "+third_party/libjingle_xmpp/task_runner",
   "+third_party/libjingle_xmpp/xmllite",

jingle/glue/DEPS

 # Needed by logging_unittest.cc since it tests the overrides.
 include_rules = [
+  "+mojo/public/cpp",
   "+services/network/proxy_resolving_client_socket.h",
   "+services/network/proxy_resolving_client_socket_factory.h",
+  "+services/network/proxy_resolving_socket_factory_mojo.h",
+  "+services/network/public/mojom",
   "+third_party/libjingle_xmpp/task_runner",
   "+third_party/webrtc",
   "+third_party/webrtc_overrides",

jingle/glue/network_service_async_socket.h

@@ -2,10 +2,11 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 //
-// An implementation of buzz::AsyncSocket that uses Chrome sockets.
+// An implementation of buzz::AsyncSocket that uses Chrome Network Service
+// sockets.
 
-#ifndef JINGLE_GLUE_CHROME_ASYNC_SOCKET_H_
-#define JINGLE_GLUE_CHROME_ASYNC_SOCKET_H_
+#ifndef JINGLE_GLUE_NETWORK_SERVICE_ASYNC_SOCKET_H_
+#define JINGLE_GLUE_NETWORK_SERVICE_ASYNC_SOCKET_H_
 
 #include <stddef.h>
 
@@ -13,35 +14,32 @@
 #include <string>
 #include <vector>
 
-#include "base/compiler_specific.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/weak_ptr.h"
-#include "net/base/completion_callback.h"
-#include "net/base/net_errors.h"
+#include "mojo/public/cpp/bindings/binding.h"
+#include "mojo/public/cpp/system/data_pipe.h"
+#include "mojo/public/cpp/system/simple_watcher.h"
 #include "net/traffic_annotation/network_traffic_annotation.h"
+#include "services/network/public/mojom/proxy_resolving_socket.mojom.h"
 #include "third_party/libjingle_xmpp/xmpp/asyncsocket.h"
 
-namespace net {
-class IOBufferWithSize;
-class StreamSocket;
-}  // namespace net
-
 namespace jingle_glue {
 
-class ResolvingClientSocketFactory;
+using GetProxyResolvingFactoryCallback = base::RepeatingCallback<void(
+    network::mojom::ProxyResolvingSocketFactoryRequest)>;
 
-class ChromeAsyncSocket : public buzz::AsyncSocket {
+class NetworkServiceAsyncSocket : public buzz::AsyncSocket,
+                                  public network::mojom::SocketObserver {
  public:
-  // Takes ownership of |resolving_client_socket_factory|.
-  ChromeAsyncSocket(std::unique_ptr<ResolvingClientSocketFactory>
-                        resolving_client_socket_factory,
-                    size_t read_buf_size,
-                    size_t write_buf_size,
-                    const net::NetworkTrafficAnnotationTag& traffic_annotation);
+  NetworkServiceAsyncSocket(
+      GetProxyResolvingFactoryCallback get_socket_factory_callback,
+      size_t read_buf_size,
+      size_t write_buf_size,
+      const net::NetworkTrafficAnnotationTag& traffic_annotation);
 
   // Does not raise any signals.
-  ~ChromeAsyncSocket() override;
+  ~NetworkServiceAsyncSocket() override;
 
   // buzz::AsyncSocket implementation.
 
@@ -65,9 +63,8 @@ class ChromeAsyncSocket : public buzz::AsyncSocket {
   //
   // If |address| has an empty hostname or a zero port, sets error to
   // ERROR_DNS and returns false.  (We don't use the IP address even
-  // if it's present, as DNS resolution is done by
-  // |resolving_client_socket_factory_|.  But it's perfectly fine if
-  // the hostname is a stringified IP address.)
+  // if it's present, as DNS resolution is done by the network service).
+  // But it's perfectly fine if the hostname is a stringified IP address.)
   //
   // Otherwise, starts the connection process and returns true.
   // SignalConnected will be raised when the connection is successful;
@@ -146,12 +143,11 @@ class ChromeAsyncSocket : public buzz::AsyncSocket {
 
  private:
   enum AsyncIOState {
-    // An I/O op is not in progress.
+    // An I/O op is not in progress or has been handed over to the network
+    // service.
     IDLE,
-    // A function has been posted to do the I/O.
-    POSTED,
-    // An async I/O operation is pending.
-    PENDING,
+    // Waiting for network service to be ready to handle an operation.
+    WAITING,
   };
 
   bool IsOpen() const;
@@ -160,61 +156,108 @@ class ChromeAsyncSocket : public buzz::AsyncSocket {
   void DoNonNetError(Error error);
   void DoNetError(net::Error net_error);
   void DoNetErrorFromStatus(int status);
+  void ProcessSocketObserverError();
+
+  // SocketObserver implementation
+  void OnReadError(int32_t net_error) override;
+  void OnWriteError(int32_t net_error) override;
 
   // Connection functions.
-  void ProcessConnectDone(int status);
+  void ProcessConnectDone(
+      network::mojom::SocketObserverRequest socket_observer_request,
+      int status,
+      const base::Optional<net::IPEndPoint>& local_addr,
+      const base::Optional<net::IPEndPoint>& peer_addr,
+      mojo::ScopedDataPipeConsumerHandle receive_stream,
+      mojo::ScopedDataPipeProducerHandle send_stream);
 
   // Read loop functions.
-  void PostDoRead();
-  void DoRead();
-  void ProcessReadDone(int status);
+  void WatchForReadReady();
+  void ProcessReadReady(MojoResult result,
+                        const mojo::HandleSignalsState& state);
+  void ReportReadError(int net_error);
 
   // Write loop functions.
-  void PostDoWrite();
-  void DoWrite();
-  void ProcessWriteDone(int status);
+  void WatchForWriteReady();
+  void ProcessWriteReady(MojoResult result,
+                         const mojo::HandleSignalsState& state);
+  void ProcessWriteClosed(MojoResult result,
+                          const mojo::HandleSignalsState& state);
 
   // SSL/TLS connection functions.
-  void ProcessSSLConnectDone(int status);
+  void ProcessSSLConnectDone(
+      network::mojom::SocketObserverRequest socket_observer_request,
+      int status,
+      mojo::ScopedDataPipeConsumerHandle receive_stream,
+      mojo::ScopedDataPipeProducerHandle send_stream);
 
   // Close functions.
   void DoClose();
 
-  std::unique_ptr<ResolvingClientSocketFactory>
-      resolving_client_socket_factory_;
+  void ConnectPipes(mojo::ScopedDataPipeConsumerHandle receive_stream,
+                    mojo::ScopedDataPipeProducerHandle send_stream);
+  void BindSocketObserver(
+      network::mojom::SocketObserverRequest socket_observer_request);
+
+  // |socket_factory_| is recreated every time via |get_socket_factory_callback|
+  // to handle network service restarts after crashes.
+  GetProxyResolvingFactoryCallback get_socket_factory_callback_;
+  network::mojom::ProxyResolvingSocketFactoryPtr socket_factory_;
+  // The handle to the proxy resolving socket for the current connection, if one
+  // exists.
+  network::mojom::ProxyResolvingSocketPtr socket_;
+  // TLS socket, if StartTls has been called.
+  network::mojom::TLSClientSocketPtr tls_socket_;
+
+  // Used to route error notifications here.
+  mojo::Binding<network::mojom::SocketObserver> socket_observer_binding_;
 
   // buzz::AsyncSocket state.
   buzz::AsyncSocket::State state_;
   buzz::AsyncSocket::Error error_;
   net::Error net_error_;
 
-  // NULL iff state() == STATE_CLOSED.
-  std::unique_ptr<net::StreamSocket> transport_socket_;
-
   // State for the read loop.  |read_start_| <= |read_end_| <=
   // |read_buf_->size()|.  There's a read in flight (i.e.,
   // |read_state_| != IDLE) iff |read_end_| == 0.
   AsyncIOState read_state_;
-  scoped_refptr<net::IOBufferWithSize> read_buf_;
+  std::vector<char> read_buf_;
   size_t read_start_, read_end_;
+  mojo::ScopedDataPipeConsumerHandle read_pipe_;
+  std::unique_ptr<mojo::SimpleWatcher> read_watcher_;
+
+  // Handling read errors is a bit tricky since the status is reported via
+  // |socket_observer_binding_|, which is unordered compared to |read_pipe_|,
+  // so it's possible to see an end of file (or an error) there while there is
+  // still useful data pending.  As a result, the code waits to see both happen
+  // before reporting error statuses (including EOF). Likewise for write pipes.
+  //
+  bool saw_error_on_read_pipe_;
+  bool saw_error_on_write_pipe_;
+
+  // This is != net::ERR_IO_PENDING (including possibly net::OK for end-of-file)
+  // if a read error was reported via socket observer interface.
+  int saw_read_error_on_socket_observer_pipe_;
+  int saw_write_error_on_socket_observer_pipe_;
 
   // State for the write loop.  |write_end_| <= |write_buf_->size()|.
   // There's a write in flight (i.e., |write_state_| != IDLE) iff
   // |write_end_| > 0.
   AsyncIOState write_state_;
-  scoped_refptr<net::IOBufferWithSize> write_buf_;
+  std::vector<char> write_buf_;
   size_t write_end_;
+  mojo::ScopedDataPipeProducerHandle write_pipe_;
+  std::unique_ptr<mojo::SimpleWatcher> write_watcher_;
+  std::unique_ptr<mojo::SimpleWatcher> write_close_watcher_;
 
-  // Network traffic annotation for downstream socket write. ChromeAsyncSocket
-  // is not reused, hence annotation can be added in constructor and used in all
-  // subsequent writes.
+  // Network traffic annotation for downstream socket write.
+  // NetworkServiceAsyncSocket is not reused, hence annotation can be added in
+  // constructor and used in all subsequent writes.
   const net::NetworkTrafficAnnotationTag traffic_annotation_;
 
-  base::WeakPtrFactory<ChromeAsyncSocket> weak_ptr_factory_;
-
-  DISALLOW_COPY_AND_ASSIGN(ChromeAsyncSocket);
+  DISALLOW_COPY_AND_ASSIGN(NetworkServiceAsyncSocket);
 };
 
 }  // namespace jingle_glue
 
-#endif  // JINGLE_GLUE_CHROME_ASYNC_SOCKET_H_
+#endif  // JINGLE_GLUE_NETWORK_SERVICE_ASYNC_SOCKET_H_

jingle/run_all_unittests.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 "base/test/launcher/unit_test_launcher.h"
+#include "base/test/test_suite.h"
+#include "mojo/core/embedder/embedder.h"
+
+int main(int argc, char** argv) {
+  base::TestSuite test_suite(argc, argv);
+
+  mojo::core::Init();
+  return base::LaunchUnitTests(
+      argc, argv,
+      base::BindOnce(&base::TestSuite::Run, base::Unretained(&test_suite)));
+}