[WebSocket] Bind WebSocketClient when connection is established

In order to deal with mojo message ordering uncertainty between
WebSocketHandshakeClient and WebSocketClient, this CL has
WebSocketHandleImpl::binding_as_client_ binds when
OnConnectionEstablished is called.

mojom.WebSocketClient.OnFailChannel is replaced with
InterfacePtr::ResetWithReason because |client_| may not be available
when network::WebSocket::WebSocketEventHandler::OnFailChannel is called.

Bug: 989406, 967524
Change-Id: Ic14357133a85317a07271a3615a21ff3e61fe37d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1728917Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Ken Rockot <rockot@google.com>
Reviewed-by: Adam Rice <ricea@chromium.org>
Reviewed-by: Yoichi Osato <yoichio@chromium.org>
Commit-Queue: Yutaka Hirano <yhirano@chromium.org>
Cr-Commit-Position: refs/heads/master@{#683477}

extensions/browser/api/web_request/web_request_proxying_websocket.cc

@@ -378,9 +378,9 @@ void WebRequestProxyingWebSocket::ContinueToStartRequest(int error_code) {
   // 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));
+  binding_as_handshake_client_.set_connection_error_with_reason_handler(
+      base::BindOnce(&WebRequestProxyingWebSocket::OnMojoConnectionError,
+                     base::Unretained(this)));
   network::mojom::AuthenticationHandlerPtr auth_handler;
   binding_as_auth_handler_.Bind(mojo::MakeRequest(&auth_handler));
   network::mojom::TrustedHeaderClientPtr trusted_header_client;
@@ -484,4 +484,12 @@ void WebRequestProxyingWebSocket::OnError(int error_code) {
   proxies_->RemoveProxy(this);
 }
 
+void WebRequestProxyingWebSocket::OnMojoConnectionError(
+    uint32_t custom_reason,
+    const std::string& description) {
+  forwarding_handshake_client_.ResetWithReason(custom_reason, description);
+  OnError(net::ERR_FAILED);
+  // Deletes |this|.
+}
+
 }  // namespace extensions

extensions/browser/api/web_request/web_request_proxying_websocket.h

@@ -107,6 +107,8 @@ class WebRequestProxyingWebSocket
   void PauseIncomingMethodCallProcessing();
   void ResumeIncomingMethodCallProcessing();
   void OnError(int result);
+  void OnMojoConnectionError(uint32_t custom_reason,
+                             const std::string& description);
 
   WebSocketFactory factory_;
   content::BrowserContext* const browser_context_;

services/network/public/mojom/websocket.mojom

@@ -112,14 +112,13 @@ interface WebSocketClient {
   // Notify the renderer that a closing handshake has been initiated by the
   // server, so that it can set the Javascript readyState to CLOSING.
   OnClosingHandshake();
-
-  OnFailChannel(string reason);
 };
 
 interface WebSocket {
   // The client side may observe the following disconnection reason from the
   // service side:
   const uint32 kInsufficientResources = 1;
+  const uint32 kInternalFailure = 2;
 
   // Send a non-control frame to the remote server.
   // - |fin| indicates that this frame is the last in the current message.

services/network/websocket.cc

@@ -222,7 +222,9 @@ void WebSocket::WebSocketEventHandler::OnFailChannel(
   DVLOG(3) << "WebSocketEventHandler::OnFailChannel @"
            << reinterpret_cast<void*>(this) << " message=\"" << message << "\"";
 
-  impl_->client_->OnFailChannel(message);
+  impl_->handshake_client_.ResetWithReason(mojom::WebSocket::kInternalFailure,
+                                           message);
+  impl_->client_.ResetWithReason(mojom::WebSocket::kInternalFailure, message);
   impl_->Reset();
 }
 

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

@@ -179,7 +179,8 @@ WebSocketChannelImpl* WebSocketChannelImpl::Create(
     std::unique_ptr<SourceLocation> location) {
   auto* channel = MakeGarbageCollected<WebSocketChannelImpl>(
       execution_context, client, std::move(location),
-      std::make_unique<WebSocketHandleImpl>());
+      std::make_unique<WebSocketHandleImpl>(
+          execution_context->GetTaskRunner(TaskType::kNetworking)));
   channel->handshake_throttle_ =
       channel->GetBaseFetchContext()->CreateWebSocketHandshakeThrottle();
   return channel;
@@ -260,11 +261,9 @@ bool WebSocketChannelImpl::Connect(const KURL& url, const String& protocol) {
     // a mojo connection error.
     mojo::MakeRequest(&connector);
   }
-  handle_->Connect(
-      std::move(connector), url, protocols,
-      GetBaseFetchContext()->GetSiteForCookies(),
-      execution_context_->UserAgent(), this,
-      execution_context_->GetTaskRunner(TaskType::kNetworking).get());
+  handle_->Connect(std::move(connector), url, protocols,
+                   GetBaseFetchContext()->GetSiteForCookies(),
+                   execution_context_->UserAgent(), this);
 
   if (handshake_throttle_) {
     // The use of WrapWeakPersistent is safe and motivated by the fact that if

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

@@ -92,8 +92,7 @@ class MockWebSocketHandle : public WebSocketHandle {
                const Vector<String>& protocols,
                const KURL& site_for_cookies,
                const String& user_agent_override,
-               WebSocketChannelImpl* channel,
-               base::SingleThreadTaskRunner*) override {
+               WebSocketChannelImpl* channel) override {
     Connect(url, protocols, site_for_cookies, user_agent_override, channel);
   }
   MOCK_METHOD5(Connect,

third_party/blink/renderer/modules/websockets/websocket_handle.h

@@ -32,7 +32,6 @@
 #define THIRD_PARTY_BLINK_RENDERER_MODULES_WEBSOCKETS_WEBSOCKET_HANDLE_H_
 
 #include <stdint.h>
-#include "base/single_thread_task_runner.h"
 #include "services/network/public/mojom/websocket.mojom-blink.h"
 #include "third_party/blink/public/mojom/websockets/websocket_connector.mojom-blink.h"
 #include "third_party/blink/renderer/platform/wtf/forward.h"
@@ -66,8 +65,7 @@ class WebSocketHandle {
                        const Vector<String>& protocols,
                        const KURL& site_for_cookies,
                        const String& user_agent_override,
-                       WebSocketChannelImpl*,
-                       base::SingleThreadTaskRunner*) = 0;
+                       WebSocketChannelImpl*) = 0;
   virtual void Send(bool fin, MessageType, const char* data, wtf_size_t) = 0;
   virtual void AddReceiveFlowControlQuota(int64_t quota) = 0;
   virtual void Close(uint16_t code, const String& reason) = 0;

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

@@ -21,8 +21,10 @@ const uint16_t kAbnormalShutdownOpCode = 1006;
 
 }  // namespace
 
-WebSocketHandleImpl::WebSocketHandleImpl()
-    : channel_(nullptr),
+WebSocketHandleImpl::WebSocketHandleImpl(
+    scoped_refptr<base::SingleThreadTaskRunner> task_runner)
+    : task_runner_(std::move(task_runner)),
+      channel_(nullptr),
       handshake_client_binding_(this),
       client_binding_(this) {
   NETWORK_DVLOG(1) << this << " created";
@@ -40,25 +42,21 @@ void WebSocketHandleImpl::Connect(mojom::blink::WebSocketConnectorPtr connector,
                                   const Vector<String>& protocols,
                                   const KURL& site_for_cookies,
                                   const String& user_agent_override,
-                                  WebSocketChannelImpl* channel,
-                                  base::SingleThreadTaskRunner* task_runner) {
+                                  WebSocketChannelImpl* channel) {
   NETWORK_DVLOG(1) << this << " connect(" << url.GetString() << ")";
 
   DCHECK(!channel_);
   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;
+  network::mojom::blink::WebSocketHandshakeClientPtr handshake_client_proxy;
   handshake_client_binding_.Bind(
-      mojo::MakeRequest(&handshake_client_proxy, task_runner), task_runner);
-  network::mojom::blink::WebSocketClientPtr client_proxy;
-  client_binding_.Bind(mojo::MakeRequest(&client_proxy, task_runner),
-                       task_runner);
-  client_binding_.set_connection_error_with_reason_handler(WTF::Bind(
+      mojo::MakeRequest(&handshake_client_proxy, task_runner_), task_runner_);
+  handshake_client_binding_.set_connection_error_with_reason_handler(WTF::Bind(
       &WebSocketHandleImpl::OnConnectionError, WTF::Unretained(this)));
+  network::mojom::blink::WebSocketClientPtr client_proxy;
+  pending_client_receiver_ = mojo::MakeRequest(&client_proxy);
 
   connector->Connect(url, protocols, site_for_cookies, user_agent_override,
                      std::move(handshake_client_proxy),
@@ -121,19 +119,12 @@ void WebSocketHandleImpl::Disconnect() {
 
 void WebSocketHandleImpl::OnConnectionError(uint32_t custom_reason,
                                             const std::string& description) {
-  // Our connection to the WebSocket was dropped. This could be due to
-  // exceeding the maximum number of concurrent websockets from this process.
-  // This handler is sufficient to detect all mojo connection errors, as
-  // any error will result in the data connection being dropped.
-  // By detecting the errors on this channel, we ensure that any FailChannel
-  // messages from the network service will be processed first.
   NETWORK_DVLOG(1) << " OnConnectionError( reason: " << custom_reason
                    << ", description:" << description;
-  OnFailChannel("Unknown reason");
-}
-
-void WebSocketHandleImpl::OnFailChannel(const String& message) {
-  NETWORK_DVLOG(1) << this << " OnFailChannel(" << message << ")";
+  String message = "Unknown reason";
+  if (custom_reason == network::mojom::blink::WebSocket::kInternalFailure) {
+    message = String::FromUTF8(description.c_str(), description.size());
+  }
 
   WebSocketChannelImpl* channel = channel_;
   Disconnect();
@@ -169,6 +160,12 @@ void WebSocketHandleImpl::OnConnectionEstablished(
   if (!channel_)
     return;
 
+  // From now on, we will detect mojo errors via |client_binding_|.
+  handshake_client_binding_.Close();
+  client_binding_.Bind(std::move(pending_client_receiver_), task_runner_);
+  client_binding_.set_connection_error_with_reason_handler(WTF::Bind(
+      &WebSocketHandleImpl::OnConnectionError, WTF::Unretained(this)));
+
   DCHECK(!websocket_);
   websocket_ = std::move(websocket);
   channel_->DidConnect(this, protocol, extensions, receive_quota_threshold);

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

@@ -38,6 +38,10 @@
 #include "third_party/blink/renderer/platform/heap/persistent.h"
 #include "third_party/blink/renderer/platform/wtf/wtf_size_t.h"
 
+namespace base {
+class SingleThreadTaskRunner;
+}  // namespace base
+
 namespace blink {
 
 class WebSocketHandleImpl
@@ -45,7 +49,7 @@ class WebSocketHandleImpl
       public network::mojom::blink::WebSocketHandshakeClient,
       public network::mojom::blink::WebSocketClient {
  public:
-  WebSocketHandleImpl();
+  explicit WebSocketHandleImpl(scoped_refptr<base::SingleThreadTaskRunner>);
   ~WebSocketHandleImpl() override;
 
   void Connect(mojom::blink::WebSocketConnectorPtr,
@@ -53,8 +57,7 @@ class WebSocketHandleImpl
                const Vector<String>& protocols,
                const KURL& site_for_cookies,
                const String& user_agent_override,
-               WebSocketChannelImpl*,
-               base::SingleThreadTaskRunner*) override;
+               WebSocketChannelImpl*) override;
   void Send(bool fin, MessageType, const char* data, wtf_size_t) override;
   void AddReceiveFlowControlQuota(int64_t quota) override;
   void Close(uint16_t code, const String& reason) override;
@@ -82,11 +85,12 @@ class WebSocketHandleImpl
                      uint16_t code,
                      const String& reason) override;
   void OnClosingHandshake() override;
-  void OnFailChannel(const String& reason) override;
 
+  scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
   WeakPersistent<WebSocketChannelImpl> channel_;
 
   network::mojom::blink::WebSocketPtr websocket_;
+  network::mojom::blink::WebSocketClientRequest pending_client_receiver_;
   mojo::Binding<network::mojom::blink::WebSocketHandshakeClient>
       handshake_client_binding_;
   mojo::Binding<network::mojom::blink::WebSocketClient> client_binding_;