Reland "Cleanup lifetime handling in P2P sockets."

This is a reland of e6df634

Original change's description:
> Cleanup lifetime handling in P2P sockets.
>
> Several fixes for P2P sockets:
>  1. P2PSocketTcpBase no longer posts tasks with base::Unretained().
>  2. Previously P2P sockets were destroyed only in response to Mojo
>     interfaces errors. They were not destroyed on other error. Fixed
>     it now.
>  3. Simplified TCP server socket protocol. Now the accepted socket is
>     passed directly in IncomingTcpConnection.
>  4. Updated unittests to verify that P2P sockets are destroyed in
>     response to errors.
>  5. Other minor cleanups, particularly moved packet dump logic to
>     SocketManager and removed some unittests that are not relevant
>     after migration to mojo.
>
> Bug: 877515,  877514
> Cq-Include-Trybots: luci.chromium.try:linux_mojo
> Change-Id: I55276e372185c558667289a1efdfcf0421c3d7bc
> Reviewed-on: https://chromium-review.googlesource.com/1189083
> Reviewed-by: Nasko Oskov <[email protected]>
> Reviewed-by: John Abd-El-Malek <[email protected]>
> Commit-Queue: Sergey Ulanov <[email protected]>
> Cr-Commit-Position: refs/heads/master@{#588430}

[email protected],[email protected]

Bug: 877515, 877514,880218
Change-Id: I0cc79616b70f7900e6c551a7c91e404de2fb7dd3
Cq-Include-Trybots: luci.chromium.try:linux_mojo
Reviewed-on: https://chromium-review.googlesource.com/1206338
Commit-Queue: John Abd-El-Malek <[email protected]>
Reviewed-by: Sergey Ulanov <[email protected]>
Cr-Original-Commit-Position: refs/heads/master@{#588887}(cherry picked from commit e07f397)
Reviewed-on: https://chromium-review.googlesource.com/1207091
Reviewed-by: John Abd-El-Malek <[email protected]>
Cr-Commit-Position: refs/branch-heads/3538@{#51}
Cr-Branched-From: 79f7c91-refs/heads/master@{#587811}

Author: SergeyUlanov

content/renderer/p2p/socket_client_impl.cc

@@ -135,29 +135,28 @@ void P2PSocketClientImpl::SendComplete(
 }
 
 void P2PSocketClientImpl::IncomingTcpConnection(
-    const net::IPEndPoint& socket_address) {
+    const net::IPEndPoint& socket_address,
+    network::mojom::P2PSocketPtr socket,
+    network::mojom::P2PSocketClientRequest client_request) {
   DCHECK_EQ(state_, STATE_OPEN);
 
   auto new_client =
       std::make_unique<P2PSocketClientImpl>(dispatcher_, traffic_annotation_);
   new_client->state_ = STATE_OPEN;
 
   network::mojom::P2PSocketClientPtr socket_client;
-  new_client->binding_.Bind(mojo::MakeRequest(&socket_client));
+  new_client->socket_ = std::move(socket);
+  new_client->binding_.Bind(std::move(client_request));
   new_client->binding_.set_connection_error_handler(base::Bind(
       &P2PSocketClientImpl::OnConnectionError, base::Unretained(this)));
 
-  socket_->AcceptIncomingTcpConnection(socket_address, std::move(socket_client),
-                                       mojo::MakeRequest(&new_client->socket_));
-
   DCHECK(thread_checker_.CalledOnValidThread());
   if (delegate_) {
     delegate_->OnIncomingTcpConnection(socket_address, std::move(new_client));
   } else {
     // Just close the socket if there is no delegate to accept it.
     new_client->Close();
   }
-
 }
 
 void P2PSocketClientImpl::DataReceived(const net::IPEndPoint& socket_address,

content/renderer/p2p/socket_client_impl.h

@@ -87,7 +87,10 @@ class P2PSocketClientImpl : public P2PSocketClient,
   void SocketCreated(const net::IPEndPoint& local_address,
                      const net::IPEndPoint& remote_address) override;
   void SendComplete(const network::P2PSendPacketMetrics& send_metrics) override;
-  void IncomingTcpConnection(const net::IPEndPoint& socket_address) override;
+  void IncomingTcpConnection(
+      const net::IPEndPoint& socket_address,
+      network::mojom::P2PSocketPtr socket,
+      network::mojom::P2PSocketClientRequest client_request) override;
   void DataReceived(const net::IPEndPoint& socket_address,
                     const std::vector<int8_t>& data,
                     base::TimeTicks timestamp) override;

services/network/p2p/socket.cc

@@ -9,13 +9,10 @@
 #include "net/base/net_errors.h"
 #include "net/url_request/url_request_context.h"
 #include "net/url_request/url_request_context_getter.h"
-#include "services/network/p2p/socket_manager.h"
 #include "services/network/p2p/socket_tcp.h"
 #include "services/network/p2p/socket_tcp_server.h"
 #include "services/network/p2p/socket_udp.h"
 #include "services/network/proxy_resolving_client_socket_factory.h"
-#include "third_party/webrtc/media/base/rtputils.h"
-#include "third_party/webrtc/media/base/turnutils.h"
 
 namespace {
 
@@ -33,22 +30,6 @@ enum class SocketErrorCode {
 };
 
 const uint32_t kStunMagicCookie = 0x2112A442;
-const size_t kMinRtcpHeaderLength = 8;
-const size_t kDtlsRecordHeaderLength = 13;
-
-bool IsDtlsPacket(const int8_t* data, size_t length) {
-  const uint8_t* u = reinterpret_cast<const uint8_t*>(data);
-  return (length >= kDtlsRecordHeaderLength && (u[0] > 19 && u[0] < 64));
-}
-
-bool IsRtcpPacket(const int8_t* data, size_t length) {
-  if (length < kMinRtcpHeaderLength) {
-    return false;
-  }
-
-  int type = (static_cast<uint8_t>(data[1]) & 0x7F);
-  return (type >= 64 && type < 96);
-}
 
 // Map the network error to SocketErrorCode for the UMA histogram.
 // static
@@ -79,24 +60,22 @@ static SocketErrorCode MapNetErrorToSocketErrorCode(int net_err) {
 
 namespace network {
 
-P2PSocket::P2PSocket(P2PSocketManager* socket_manager,
+P2PSocket::P2PSocket(Delegate* delegate,
                      mojom::P2PSocketClientPtr client,
                      mojom::P2PSocketRequest socket,
                      ProtocolType protocol_type)
-    : socket_manager_(socket_manager),
+    : delegate_(delegate),
       client_(std::move(client)),
       binding_(this, std::move(socket)),
       state_(STATE_UNINITIALIZED),
-      dump_incoming_rtp_packet_(false),
-      dump_outgoing_rtp_packet_(false),
       protocol_type_(protocol_type),
       send_packets_delayed_total_(0),
       send_packets_total_(0),
       send_bytes_delayed_max_(0),
       send_bytes_delayed_cur_(0),
       weak_ptr_factory_(this) {
   binding_.set_connection_error_handler(
-      base::BindOnce(&P2PSocket::OnConnectionError, base::Unretained(this)));
+      base::BindOnce(&P2PSocket::OnError, base::Unretained(this)));
 }
 
 P2PSocket::~P2PSocket() {
@@ -122,7 +101,7 @@ P2PSocket::~P2PSocket() {
 
 // Verifies that the packet |data| has a valid STUN header.
 // static
-bool P2PSocket::GetStunPacketType(const int8_t* data,
+bool P2PSocket::GetStunPacketType(const uint8_t* data,
                                   int data_size,
                                   StunMessageType* type) {
   if (data_size < kStunHeaderSize) {
@@ -182,7 +161,7 @@ void P2PSocket::ReportSocketError(int result, const char* histogram_name) {
 
 // static
 P2PSocket* P2PSocket::Create(
-    P2PSocketManager* socket_manager,
+    Delegate* delegate,
     mojom::P2PSocketClientPtr client,
     mojom::P2PSocketRequest socket,
     P2PSocketType type,
@@ -191,28 +170,27 @@ P2PSocket* P2PSocket::Create(
     P2PMessageThrottler* throttler) {
   switch (type) {
     case P2P_SOCKET_UDP:
-      return new P2PSocketUdp(socket_manager, std::move(client),
-                              std::move(socket), throttler, net_log);
+      return new P2PSocketUdp(delegate, std::move(client), std::move(socket),
+                              throttler, net_log);
     case P2P_SOCKET_TCP_SERVER:
-      return new P2PSocketTcpServer(socket_manager, std::move(client),
+      return new P2PSocketTcpServer(delegate, std::move(client),
                                     std::move(socket), P2P_SOCKET_TCP_CLIENT);
 
     case P2P_SOCKET_STUN_TCP_SERVER:
-      return new P2PSocketTcpServer(socket_manager, std::move(client),
+      return new P2PSocketTcpServer(delegate, std::move(client),
                                     std::move(socket),
                                     P2P_SOCKET_STUN_TCP_CLIENT);
 
     case P2P_SOCKET_TCP_CLIENT:
     case P2P_SOCKET_SSLTCP_CLIENT:
     case P2P_SOCKET_TLS_CLIENT:
-      return new P2PSocketTcp(socket_manager, std::move(client),
-                              std::move(socket), type,
-                              proxy_resolving_socket_factory);
+      return new P2PSocketTcp(delegate, std::move(client), std::move(socket),
+                              type, proxy_resolving_socket_factory);
 
     case P2P_SOCKET_STUN_TCP_CLIENT:
     case P2P_SOCKET_STUN_SSLTCP_CLIENT:
     case P2P_SOCKET_STUN_TLS_CLIENT:
-      return new P2PSocketStunTcp(socket_manager, std::move(client),
+      return new P2PSocketStunTcp(delegate, std::move(client),
                                   std::move(socket), type,
                                   proxy_resolving_socket_factory);
   }
@@ -221,30 +199,6 @@ P2PSocket* P2PSocket::Create(
   return nullptr;
 }
 
-void P2PSocket::StartRtpDump(bool incoming, bool outgoing) {
-  DCHECK(incoming || outgoing);
-
-  if (incoming) {
-    dump_incoming_rtp_packet_ = true;
-  }
-
-  if (outgoing) {
-    dump_outgoing_rtp_packet_ = true;
-  }
-}
-
-void P2PSocket::StopRtpDump(bool incoming, bool outgoing) {
-  DCHECK(incoming || outgoing);
-
-  if (incoming) {
-    dump_incoming_rtp_packet_ = false;
-  }
-
-  if (outgoing) {
-    dump_outgoing_rtp_packet_ = false;
-  }
-}
-
 mojom::P2PSocketClientPtr P2PSocket::ReleaseClientForTesting() {
   return std::move(client_);
 }
@@ -253,36 +207,6 @@ mojom::P2PSocketRequest P2PSocket::ReleaseBindingForTesting() {
   return binding_.Unbind();
 }
 
-void P2PSocket::DumpRtpPacket(const int8_t* packet,
-                              size_t length,
-                              bool incoming) {
-  if (!socket_manager_ || IsDtlsPacket(packet, length) ||
-      IsRtcpPacket(packet, length)) {
-    return;
-  }
-
-  size_t rtp_packet_pos = 0;
-  size_t rtp_packet_length = length;
-  if (!cricket::UnwrapTurnPacket(reinterpret_cast<const uint8_t*>(packet),
-                                 length, &rtp_packet_pos, &rtp_packet_length)) {
-    return;
-  }
-
-  packet += rtp_packet_pos;
-
-  size_t header_length = 0;
-  bool valid =
-      cricket::ValidateRtpHeader(reinterpret_cast<const uint8_t*>(packet),
-                                 rtp_packet_length, &header_length);
-  if (!valid) {
-    NOTREACHED();
-    return;
-  }
-
-  socket_manager_->DumpPacket(packet, header_length, rtp_packet_length,
-                              incoming);
-}
-
 void P2PSocket::IncrementDelayedPackets() {
   send_packets_delayed_total_++;
 }
@@ -303,9 +227,10 @@ void P2PSocket::DecrementDelayedBytes(uint32_t size) {
   DCHECK_GE(send_bytes_delayed_cur_, 0);
 }
 
-void P2PSocket::OnConnectionError() {
-  if (socket_manager_)
-    socket_manager_->DestroySocket(this);
+void P2PSocket::OnError() {
+  binding_.Close();
+  client_.reset();
+  delegate_->DestroySocket(this);
 }
 
 }  // namespace network

services/network/p2p/socket.h

@@ -9,7 +9,9 @@
 #include <stdint.h>
 
 #include <memory>
+
 #include "base/component_export.h"
+#include "base/containers/span.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "mojo/public/cpp/bindings/binding.h"
@@ -24,18 +26,38 @@ class NetLog;
 }
 
 namespace network {
+
 class ProxyResolvingClientSocketFactory;
-class P2PSocketManager;
 class P2PMessageThrottler;
 
 // Base class for P2P sockets.
 class COMPONENT_EXPORT(NETWORK_SERVICE) P2PSocket : public mojom::P2PSocket {
  public:
+  // Interface implemented in P2PSocketManager.
+  class Delegate {
+   public:
+    Delegate() = default;
+
+    // Destroys |socket| and removes it from the list of sockets.
+    virtual void DestroySocket(P2PSocket* socket) = 0;
+
+    // Called by P2PSocketTcpServer after a new socket is created for an
+    // incoming connection.
+    virtual void AddAcceptedConnection(std::unique_ptr<P2PSocket> socket) = 0;
+
+    // Called for each incoming/outgoing packet.
+    virtual void DumpPacket(base::span<const uint8_t> data, bool incoming) = 0;
+
+   protected:
+    virtual ~Delegate() = default;
+  };
+
   static const int kStunHeaderSize = 20;
   static const size_t kMaximumPacketSize = 32768;
+
   // Creates P2PSocket of the specific type.
   static P2PSocket* Create(
-      P2PSocketManager* socket_manager,
+      Delegate* delegate,
       mojom::P2PSocketClientPtr client,
       mojom::P2PSocketRequest socket,
       P2PSocketType type,
@@ -45,7 +67,7 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) P2PSocket : public mojom::P2PSocket {
 
   ~P2PSocket() override;
 
-  // Initalizes the socket. Returns false when initialization fails.
+  // Initializes the socket. Returns false when initialization fails.
   // |min_port| and |max_port| specify the valid range of allowed ports.
   // |min_port| must be less than or equal to |max_port|.
   // If |min_port| is zero, |max_port| must also be zero and it means all ports
@@ -54,14 +76,11 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) P2PSocket : public mojom::P2PSocket {
   // in the valid range.
   // If |local_address.port()| is nonzero and not in the valid range,
   // initialization will fail.
-  virtual bool Init(const net::IPEndPoint& local_address,
+  virtual void Init(const net::IPEndPoint& local_address,
                     uint16_t min_port,
                     uint16_t max_port,
                     const P2PHostAndIPEndPoint& remote_address) = 0;
 
-  void StartRtpDump(bool incoming, bool outgoing);
-  void StopRtpDump(bool incoming, bool outgoing);
-
   mojom::P2PSocketClientPtr ReleaseClientForTesting();
   mojom::P2PSocketRequest ReleaseBindingForTesting();
 
@@ -101,43 +120,41 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) P2PSocket : public mojom::P2PSocket {
     STATE_UNINITIALIZED,
     STATE_CONNECTING,
     STATE_OPEN,
-    STATE_ERROR,
   };
 
-  P2PSocket(P2PSocketManager* socket_manager,
+  P2PSocket(Delegate* delegate,
             mojom::P2PSocketClientPtr client,
             mojom::P2PSocketRequest socket,
             ProtocolType protocol_type);
 
   // Verifies that the packet |data| has a valid STUN header. In case
   // of success stores type of the message in |type|.
-  static bool GetStunPacketType(const int8_t* data,
+  static bool GetStunPacketType(const uint8_t* data,
                                 int data_size,
                                 StunMessageType* type);
   static bool IsRequestOrResponse(StunMessageType type);
 
   static void ReportSocketError(int result, const char* histogram_name);
 
-  // Calls |socket_manager_| to record the RTP header.
-  void DumpRtpPacket(const int8_t* packet, size_t length, bool incoming);
+  // Should be called by subclasses on protocol errors. Closes P2PSocket and
+  // P2PSocketClient channels and calls delegate_->DestroySocket() to
+  // destroy the socket.
+  void OnError();
 
   // Used by subclasses to track the metrics of delayed bytes and packets.
   void IncrementDelayedPackets();
   void IncrementTotalSentPackets();
   void IncrementDelayedBytes(uint32_t size);
   void DecrementDelayedBytes(uint32_t size);
 
-  P2PSocketManager* socket_manager_;
+  Delegate* delegate_;
   mojom::P2PSocketClientPtr client_;
   mojo::Binding<mojom::P2PSocket> binding_;
   State state_;
-  bool dump_incoming_rtp_packet_;
-  bool dump_outgoing_rtp_packet_;
 
   ProtocolType protocol_type_;
 
  private:
-  void OnConnectionError();
   // Track total delayed packets for calculating how many packets are
   // delayed by system at the end of call.
   uint32_t send_packets_delayed_total_;