Implement NetworkContext::CreateQuicTransport

 - Implemnt network::NetworkContext::CreateQuicTransport.
 - Have unittests use the function instead of calling the constructor.
 - Fix a bug which was caused by the synchronous deletion of
   QuicTransport in QuicTransportClient::Visitor callbacks.

Bug: 1011392
Change-Id: I8a7ce1f9d636886f66308894e3c4677198740e94
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1981527
Commit-Queue: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#728801}

services/network/network_context.cc

@@ -1162,7 +1162,8 @@ void NetworkContext::CreateQuicTransport(
     const net::NetworkIsolationKey& key,
     mojo::PendingRemote<mojom::QuicTransportHandshakeClient>
         pending_handshake_client) {
-  // TODO(yhirano): Implement this and have a security review on the impl too.
+  quic_transports_.insert(std::make_unique<QuicTransport>(
+      url, origin, key, this, std::move(pending_handshake_client)));
 }
 
 void NetworkContext::CreateNetLogExporter(
@@ -1615,6 +1616,13 @@ const net::HttpAuthPreferences* NetworkContext::GetHttpAuthPreferences() const
   return &http_auth_merged_preferences_;
 }
 
+size_t NetworkContext::NumOpenQuicTransports() const {
+  return std::count_if(quic_transports_.begin(), quic_transports_.end(),
+                       [](const std::unique_ptr<QuicTransport>& transport) {
+                         return !transport->torn_down();
+                       });
+}
+
 void NetworkContext::OnHttpAuthDynamicParamsChanged(
     const mojom::HttpAuthDynamicParams*
         http_auth_dynamic_network_service_params) {

services/network/network_context.h

@@ -467,7 +467,7 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) NetworkContext
 
   const net::HttpAuthPreferences* GetHttpAuthPreferences() const;
 
-  size_t num_open_quic_transports() const { return quic_transports_.size(); }
+  size_t NumOpenQuicTransports() const;
 
  private:
   URLRequestContextOwner MakeURLRequestContext(

services/network/public/mojom/network_context.mojom

@@ -1120,7 +1120,9 @@ interface NetworkContext {
         pending_remote<AuthenticationHandler>? auth_handler,
         pending_remote<TrustedHeaderClient>? header_client);
 
-  // Creates a QuicTransport connection to |url|.
+  // Creates a QuicTransport connection to |url|. |origin| is used for the
+  // client indication - see
+  // https://tools.ietf.org/html/draft-vvv-webtransport-quic-01#section-3.2 .
   //
   // It is recommended to detect mojo connection errors on |handshake_client|.
   CreateQuicTransport(

services/network/quic_transport.cc

@@ -4,6 +4,8 @@
 
 #include "services/network/quic_transport.h"
 
+#include "base/bind.h"
+#include "base/threading/sequenced_task_runner_handle.h"
 #include "net/base/io_buffer.h"
 #include "net/quic/platform/impl/quic_mem_slice_impl.h"
 #include "net/third_party/quiche/src/quic/core/quic_session.h"
@@ -40,6 +42,8 @@ QuicTransport::~QuicTransport() = default;
 
 void QuicTransport::SendDatagram(base::span<const uint8_t> data,
                                  base::OnceCallback<void(bool)> callback) {
+  DCHECK(!torn_down_);
+
   auto buffer = base::MakeRefCounted<net::IOBuffer>(data.size());
   memcpy(buffer->data(), data.data(), data.size());
   quic::QuicMemSlice slice(
@@ -50,6 +54,10 @@ void QuicTransport::SendDatagram(base::span<const uint8_t> data,
 }
 
 void QuicTransport::OnConnected() {
+  if (torn_down_) {
+    return;
+  }
+
   DCHECK(handshake_client_);
 
   handshake_client_->OnConnectionEstablished(
@@ -62,32 +70,52 @@ void QuicTransport::OnConnected() {
 }
 
 void QuicTransport::OnConnectionFailed() {
+  if (torn_down_) {
+    return;
+  }
+
   DCHECK(handshake_client_);
 
   handshake_client_->OnHandshakeFailed();
 
-  Dispose();
-  // |this| is deleted.
+  TearDown();
 }
 
 void QuicTransport::OnClosed() {
+  if (torn_down_) {
+    return;
+  }
+
   DCHECK(!handshake_client_);
 
-  Dispose();
-  // |this| is deleted.
+  TearDown();
 }
 
 void QuicTransport::OnError() {
+  if (torn_down_) {
+    return;
+  }
+
   DCHECK(!handshake_client_);
 
-  Dispose();
-  // |this| is deleted.
+  TearDown();
 }
 
 void QuicTransport::OnIncomingBidirectionalStreamAvailable() {}
 
 void QuicTransport::OnIncomingUnidirectionalStreamAvailable() {}
 
+void QuicTransport::TearDown() {
+  torn_down_ = true;
+  receiver_.reset();
+  handshake_client_.reset();
+  client_.reset();
+
+  base::SequencedTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE,
+      base::BindOnce(&QuicTransport::Dispose, weak_factory_.GetWeakPtr()));
+}
+
 void QuicTransport::Dispose() {
   context_->Remove(this);
   // |this| is deleted.

services/network/quic_transport.h

@@ -7,6 +7,7 @@
 
 #include <memory>
 #include "base/macros.h"
+#include "base/memory/weak_ptr.h"
 #include "mojo/public/cpp/bindings/receiver.h"
 #include "mojo/public/cpp/bindings/remote.h"
 #include "net/quic/quic_transport_client.h"
@@ -53,15 +54,23 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) QuicTransport final
   void OnIncomingBidirectionalStreamAvailable() override;
   void OnIncomingUnidirectionalStreamAvailable() override;
 
-  void Dispose();
+  bool torn_down() const { return torn_down_; }
 
  private:
+  void TearDown();
+  void Dispose();
+
   const std::unique_ptr<net::QuicTransportClient> transport_;
   NetworkContext* const context_;  // outlives |this|.
 
   mojo::Receiver<mojom::QuicTransport> receiver_;
   mojo::Remote<mojom::QuicTransportHandshakeClient> handshake_client_;
   mojo::Remote<mojom::QuicTransportClient> client_;
+
+  bool torn_down_ = false;
+
+  // This must be the last member.
+  base::WeakPtrFactory<QuicTransport> weak_factory_{this};
 };
 
 }  // namespace network

services/network/quic_transport_unittest.cc

@@ -109,22 +109,22 @@ class QuicTransportTest : public testing::Test {
   }
   ~QuicTransportTest() override = default;
 
-  std::unique_ptr<QuicTransport> CreateQuicTransport(
+  void CreateQuicTransport(
       const GURL& url,
       const url::Origin& origin,
       const net::NetworkIsolationKey& key,
       mojo::PendingRemote<mojom::QuicTransportHandshakeClient>
           handshake_client) {
-    return std::make_unique<QuicTransport>(url, origin, key, &network_context_,
-                                           std::move(handshake_client));
+    network_context_.CreateQuicTransport(url, origin, key,
+                                         std::move(handshake_client));
   }
-  std::unique_ptr<QuicTransport> CreateQuicTransport(
+  void CreateQuicTransport(
       const GURL& url,
       const url::Origin& origin,
       mojo::PendingRemote<mojom::QuicTransportHandshakeClient>
           handshake_client) {
-    return CreateQuicTransport(url, origin, net::NetworkIsolationKey(),
-                               std::move(handshake_client));
+    CreateQuicTransport(url, origin, net::NetworkIsolationKey(),
+                        std::move(handshake_client));
   }
 
   GURL GetURL(base::StringPiece suffix) {
@@ -156,14 +156,15 @@ TEST_F(QuicTransportTest, ConnectSuccessfully) {
       handshake_client.InitWithNewPipeAndPassReceiver(),
       run_loop_for_handshake.QuitClosure());
 
-  auto transport = CreateQuicTransport(GetURL("/discard"), origin(),
-                                       std::move(handshake_client));
+  CreateQuicTransport(GetURL("/discard"), origin(),
+                      std::move(handshake_client));
 
   run_loop_for_handshake.Run();
 
   EXPECT_TRUE(test_handshake_client.has_seen_connection_establishment());
   EXPECT_FALSE(test_handshake_client.has_seen_handshake_failure());
   EXPECT_FALSE(test_handshake_client.has_seen_mojo_connection_error());
+  EXPECT_EQ(1u, network_context().NumOpenQuicTransports());
 }
 
 TEST_F(QuicTransportTest, ConnectWithError) {
@@ -174,9 +175,9 @@ TEST_F(QuicTransportTest, ConnectWithError) {
       run_loop_for_handshake.QuitClosure());
 
   // This should fail due to the wrong origin
-  auto transport = CreateQuicTransport(
-      GetURL("/discard"), url::Origin::Create(GURL("https://evil.com")),
-      std::move(handshake_client));
+  CreateQuicTransport(GetURL("/discard"),
+                      url::Origin::Create(GURL("https://evil.com")),
+                      std::move(handshake_client));
 
   run_loop_for_handshake.Run();
 
@@ -193,9 +194,9 @@ TEST_F(QuicTransportTest, SendDatagram) {
       handshake_client.InitWithNewPipeAndPassReceiver(),
       run_loop_for_handshake.QuitClosure());
 
-  auto transport = CreateQuicTransport(
-      GetURL("/discard"), url::Origin::Create(GURL("https://example.org/")),
-      std::move(handshake_client));
+  CreateQuicTransport(GetURL("/discard"),
+                      url::Origin::Create(GURL("https://example.org/")),
+                      std::move(handshake_client));
 
   run_loop_for_handshake.Run();
 
@@ -220,9 +221,9 @@ TEST_F(QuicTransportTest, SendToolargeDatagram) {
       handshake_client.InitWithNewPipeAndPassReceiver(),
       run_loop_for_handshake.QuitClosure());
 
-  auto transport = CreateQuicTransport(
-      GetURL("/discard"), url::Origin::Create(GURL("https://example.org/")),
-      std::move(handshake_client));
+  CreateQuicTransport(GetURL("/discard"),
+                      url::Origin::Create(GURL("https://example.org/")),
+                      std::move(handshake_client));
 
   run_loop_for_handshake.Run();