[WebSocket] Fix and document mojo connection error handling

Fix mojo connection error handling on extensions to fix flaky tests.
Document that in network_context.mojom.

Bug: 981467, 967524
Change-Id: I51cd834ca71cb712b1538196ba5e2d730a0c2eed
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1689554
Commit-Queue: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Yoichi Osato <yoichio@chromium.org>
Reviewed-by: Ken Rockot <rockot@google.com>
Reviewed-by: Adam Rice <ricea@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#676334}

extensions/browser/api/web_request/web_request_proxying_websocket.cc

@@ -343,24 +343,20 @@ void WebRequestProxyingWebSocket::ContinueToStartRequest(int error_code) {
     }
   }
 
+  // Here we detect mojo connection errors on |handshake_client|. See also
+  // CreateWebSocket in //network/services/public/mojom/network_context.mojom.
+  // Here we don't have |connection_client| so using |handshake_client| is the
+  // best.
   network::mojom::WebSocketHandshakeClientPtr handshake_client;
   binding_as_handshake_client_.Bind(mojo::MakeRequest(&handshake_client));
   binding_as_handshake_client_.set_connection_error_handler(
       base::BindOnce(&WebRequestProxyingWebSocket::OnError,
                      base::Unretained(this), net::ERR_FAILED));
-
   network::mojom::AuthenticationHandlerPtr auth_handler;
   binding_as_auth_handler_.Bind(mojo::MakeRequest(&auth_handler));
-  binding_as_auth_handler_.set_connection_error_handler(
-      base::BindOnce(&WebRequestProxyingWebSocket::OnError,
-                     base::Unretained(this), net::ERR_FAILED));
-
   network::mojom::TrustedHeaderClientPtr trusted_header_client;
   if (binding_as_header_client_.impl()) {
     binding_as_header_client_.Bind(mojo::MakeRequest(&trusted_header_client));
-    binding_as_header_client_.set_connection_error_handler(
-        base::BindOnce(&WebRequestProxyingWebSocket::OnError,
-                       base::Unretained(this), net::ERR_FAILED));
   }
 
   std::move(factory_).Run(info_.url, std::move(additional_headers),

services/network/public/mojom/network_context.mojom

@@ -841,13 +841,22 @@ interface NetworkContext {
   // "sec-websocket-protocol" is constructed from  |requested_protocols| in
   // this message). |site_for_cookies| represents the first-party origin for
   // the request. |options| may be a combination of the kWebSocketOption* flags.
-  // The connection will be shut down when |handshake_client| gets disconnected
-  // during the handshake phase, or |connection_client| gets disconnected.
+  //
   // If |header_client| is set, requests with the kURLLoadOptionUseHeaderClient
   // option will callback to the |header_client|, allowing the Cookie/Referrer
   // request headers and Cookie response headers to be modified. This has a
   // performance impact because of the extra process hops, so use should be
   // minimized.
+  //
+  // Detect mojo connection errors on |handshake_client| in the caller side.
+  // Do *NOT* interpret mojo connection errors on |auth_handler| and
+  // |header_client| as WebSocket connection errors. They are disconnected when
+  // the connection is established, and due to message ordering uncertainty we
+  // cannot know what happened. |handshake_client| doesn't have such an issue
+  // because OnConnectionEstablished will be called when the connection is
+  // established, but it is still recommended to rely only on
+  // |connection_client| because |connection_client| can be disconnected with
+  // custom reasons.
   CreateWebSocket(
         url.mojom.Url url,
         array<string> requested_protocols,

services/network/websocket.cc

@@ -385,6 +385,10 @@ WebSocket::WebSocket(
     header_client_.set_connection_error_handler(
         base::BindOnce(&WebSocket::OnConnectionError, base::Unretained(this)));
   }
+  handshake_client_.set_connection_error_handler(
+      base::BindOnce(&WebSocket::OnConnectionError, base::Unretained(this)));
+  client_.set_connection_error_handler(
+      base::BindOnce(&WebSocket::OnConnectionError, base::Unretained(this)));
   if (delay_ > base::TimeDelta()) {
     base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
         FROM_HERE,

services/network/websocket_factory.cc

@@ -106,7 +106,7 @@ void WebSocketFactory::CreateWebSocket(
     mojom::TrustedHeaderClientPtr header_client) {
   if (throttler_.HasTooManyPendingConnections(process_id)) {
     // Too many websockets!
-    handshake_client.ResetWithReason(
+    client.ResetWithReason(
         mojom::WebSocket::kInsufficientResources,
         "Error in connection establishment: net::ERR_INSUFFICIENT_RESOURCES");
     return;

third_party/blink/public/mojom/websockets/websocket_connector.mojom

@@ -13,6 +13,8 @@ interface WebSocketConnector {
   // Starts an opening handshake.
   // |user_agent| is a "user-agent" request header value. For other params, see
   // CreateWebSocket in //services/network/public/mojom/network_context.mojom.
+  // It is recommended to detect mojo connection errors on |connection_client|,
+  // not on |handshake_client|. See network_context.mojom for details.
   Connect(url.mojom.Url url,
           array<string> requested_protocols,
           url.mojom.Url site_for_cookies,

third_party/blink/renderer/modules/websockets/websocket_handle_impl.cc

@@ -48,6 +48,8 @@ void WebSocketHandleImpl::Connect(mojom::blink::WebSocketConnectorPtr connector,
   DCHECK(channel);
   channel_ = channel;
 
+  // Here we detect mojo connection errors on |client_binding_|. See also
+  // CreateWebSocket in //network/services/public/mojom/network_context.mojom.
   network::mojom::blink::WebSocketHandshakeClientPtr handshake_client_proxy;
   Vector<network::mojom::blink::HttpHeaderPtr> additional_headers;
   handshake_client_binding_.Bind(