[websocket] Split interface mojom::WebSocketClient

This CL splits the interface to two parts so that WebRequestProxyingWebSocket or
extensions proxy only handshake of WebSocket:

interface WebSocketHandshakeClient {
  OnStartOpeningHandshake(WebSocketHandshakeRequest request);
  OnFinishOpeningHandshake(WebSocketHandshakeResponse response);
  OnAddChannelResponse(string selected_protocol, string extensions);
};

interface WebSocketClient {
  OnDataFrame(bool fin, WebSocketMessageType type, array<uint8> data);
  OnFlowControl(int64 quota);
  OnDropChannel(bool was_clean, uint16 code, string reason);
  OnClosingHandshake();
  OnFailChannel(string reason);
};

Performance gain of receive-arraybuffer-1MBx100.html:
Mine: 1050 ms
ToT : 1221 ms

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

chrome/test/data/extensions/api_test/webrequest/test_websocket_auth.js

@@ -80,7 +80,7 @@ chrome.tabs.getCurrent(function(tab) {
               type: 'websocket',
               ip: '127.0.0.1',
               fromCache: false,
-              error: 'net::ERR_ABORTED',
+              error: 'net::ERR_FAILED',
               initiator: getDomain(initiators.WEB_INITIATED)
             }
           },
@@ -169,7 +169,7 @@ chrome.tabs.getCurrent(function(tab) {
               type: 'websocket',
               ip: '127.0.0.1',
               fromCache: false,
-              error: 'net::ERR_ABORTED',
+              error: 'net::ERR_FAILED',
               initiator: getDomain(initiators.WEB_INITIATED)
             }
           },
@@ -257,7 +257,7 @@ chrome.tabs.getCurrent(function(tab) {
               type: 'websocket',
               ip: '127.0.0.1',
               fromCache: false,
-              error: 'net::ERR_ABORTED',
+              error: 'net::ERR_FAILED',
               initiator: getDomain(initiators.WEB_INITIATED)
             }
           },

extensions/browser/api/web_request/web_request_proxying_websocket.cc

@@ -78,8 +78,10 @@ void WebRequestProxyingWebSocket::AddChannelRequest(
     const std::vector<std::string>& requested_protocols,
     const GURL& site_for_cookies,
     std::vector<network::mojom::HttpHeaderPtr> additional_headers,
+    network::mojom::WebSocketHandshakeClientPtr handshake_client,
     network::mojom::WebSocketClientPtr client) {
-  if (binding_as_client_.is_bound() || !client || forwarding_client_) {
+  if (binding_as_client_.is_bound() || !handshake_client ||
+      forwarding_handshake_client_ || !client || forwarding_client_) {
     // Illegal request.
     proxied_socket_ = nullptr;
     return;
@@ -96,6 +98,7 @@ void WebRequestProxyingWebSocket::AddChannelRequest(
                                nullptr, routing_id, resource_context_, request_,
                                false /* is_download */, true /* is_async */));
 
+  forwarding_handshake_client_ = std::move(handshake_client);
   forwarding_client_ = std::move(client);
   additional_headers_ = std::move(additional_headers);
 
@@ -154,31 +157,15 @@ void WebRequestProxyingWebSocket::StartClosingHandshake(
   proxied_socket_->StartClosingHandshake(code, reason);
 }
 
-void WebRequestProxyingWebSocket::OnFailChannel(const std::string& reason) {
-  DCHECK(forwarding_client_);
-  forwarding_client_->OnFailChannel(reason);
-
-  forwarding_client_ = nullptr;
-  int rv = net::ERR_FAILED;
-  if (reason == "HTTP Authentication failed; no valid credentials available" ||
-      reason == "Proxy authentication failed") {
-    // This is needed to make some tests pass.
-    // TODO(yhirano): Remove this hack.
-    rv = net::ERR_ABORTED;
-  }
-
-  OnError(rv);
-}
-
 void WebRequestProxyingWebSocket::OnStartOpeningHandshake(
     network::mojom::WebSocketHandshakeRequestPtr request) {
-  DCHECK(forwarding_client_);
-  forwarding_client_->OnStartOpeningHandshake(std::move(request));
+  DCHECK(forwarding_handshake_client_);
+  forwarding_handshake_client_->OnStartOpeningHandshake(std::move(request));
 }
 
 void WebRequestProxyingWebSocket::OnFinishOpeningHandshake(
     network::mojom::WebSocketHandshakeResponsePtr response) {
-  DCHECK(forwarding_client_);
+  DCHECK(forwarding_handshake_client_);
 
   // response_.headers will be set in OnBeforeSendHeaders if
   // binding_as_header_client_ is set.
@@ -198,7 +185,7 @@ void WebRequestProxyingWebSocket::OnFinishOpeningHandshake(
   // OnFinishOpeningHandshake is called with the original response headers.
   // That means if OnHeadersReceived modified them the renderer won't see that
   // modification. This is the opposite of http(s) requests.
-  forwarding_client_->OnFinishOpeningHandshake(std::move(response));
+  forwarding_handshake_client_->OnFinishOpeningHandshake(std::move(response));
 
   if (!binding_as_header_client_ || response_.headers) {
     ContinueToHeadersReceived();
@@ -231,41 +218,14 @@ void WebRequestProxyingWebSocket::ContinueToHeadersReceived() {
 void WebRequestProxyingWebSocket::OnAddChannelResponse(
     const std::string& selected_protocol,
     const std::string& extensions) {
-  DCHECK(forwarding_client_);
+  DCHECK(forwarding_handshake_client_);
   DCHECK(!is_done_);
   is_done_ = true;
   ExtensionWebRequestEventRouter::GetInstance()->OnCompleted(
       browser_context_, info_map_, &info_.value(), net::ERR_WS_UPGRADE);
 
-  forwarding_client_->OnAddChannelResponse(selected_protocol, extensions);
-}
-
-void WebRequestProxyingWebSocket::OnDataFrame(
-    bool fin,
-    network::mojom::WebSocketMessageType type,
-    const std::vector<uint8_t>& data) {
-  DCHECK(forwarding_client_);
-  forwarding_client_->OnDataFrame(fin, type, data);
-}
-
-void WebRequestProxyingWebSocket::OnFlowControl(int64_t quota) {
-  DCHECK(forwarding_client_);
-  forwarding_client_->OnFlowControl(quota);
-}
-
-void WebRequestProxyingWebSocket::OnDropChannel(bool was_clean,
-                                                uint16_t code,
-                                                const std::string& reason) {
-  DCHECK(forwarding_client_);
-  forwarding_client_->OnDropChannel(was_clean, code, reason);
-
-  forwarding_client_ = nullptr;
-  OnError(net::ERR_FAILED);
-}
-
-void WebRequestProxyingWebSocket::OnClosingHandshake() {
-  DCHECK(forwarding_client_);
-  forwarding_client_->OnClosingHandshake();
+  forwarding_handshake_client_->OnAddChannelResponse(selected_protocol,
+                                                     extensions);
 }
 
 void WebRequestProxyingWebSocket::OnAuthRequired(
@@ -410,7 +370,7 @@ void WebRequestProxyingWebSocket::OnBeforeSendHeadersComplete(
 }
 
 void WebRequestProxyingWebSocket::ContinueToStartRequest(int error_code) {
-  network::mojom::WebSocketClientPtr proxy;
+  network::mojom::WebSocketHandshakeClientPtr proxy;
 
   base::flat_set<std::string> used_header_names;
   std::vector<network::mojom::HttpHeaderPtr> additional_headers;
@@ -432,7 +392,8 @@ void WebRequestProxyingWebSocket::ContinueToStartRequest(int error_code) {
                      base::Unretained(this), net::ERR_FAILED));
   proxied_socket_->AddChannelRequest(
       request_.url, websocket_protocols_, request_.site_for_cookies,
-      std::move(additional_headers), std::move(proxy));
+      std::move(additional_headers), std::move(proxy),
+      std::move(forwarding_client_));
 }
 
 void WebRequestProxyingWebSocket::OnHeadersReceivedComplete(int error_code) {
@@ -522,8 +483,6 @@ void WebRequestProxyingWebSocket::OnError(int error_code) {
         browser_context_, info_map_, &info_.value(), true /* started */,
         error_code);
   }
-  if (forwarding_client_)
-    forwarding_client_->OnFailChannel(net::ErrorToString(error_code));
 
   // Deletes |this|.
   proxies_->RemoveProxy(this);

extensions/browser/api/web_request/web_request_proxying_websocket.h

@@ -36,7 +36,7 @@ namespace extensions {
 class WebRequestProxyingWebSocket
     : public WebRequestAPI::Proxy,
       public network::mojom::WebSocket,
-      public network::mojom::WebSocketClient,
+      public network::mojom::WebSocketHandshakeClient,
       public network::mojom::AuthenticationHandler,
       public network::mojom::TrustedHeaderClient {
  public:
@@ -61,6 +61,7 @@ class WebRequestProxyingWebSocket
       const std::vector<std::string>& requested_protocols,
       const GURL& site_for_cookies,
       std::vector<network::mojom::HttpHeaderPtr> additional_headers,
+      network::mojom::WebSocketHandshakeClientPtr handshake_client,
       network::mojom::WebSocketClientPtr client) override;
   void SendFrame(bool fin,
                  network::mojom::WebSocketMessageType type,
@@ -68,22 +69,13 @@ class WebRequestProxyingWebSocket
   void SendFlowControl(int64_t quota) override;
   void StartClosingHandshake(uint16_t code, const std::string& reason) override;
 
-  // mojom::WebSocketClient methods:
-  void OnFailChannel(const std::string& reason) override;
+  // mojom::WebSocketHandShakeClient methods:
   void OnStartOpeningHandshake(
       network::mojom::WebSocketHandshakeRequestPtr request) override;
   void OnFinishOpeningHandshake(
       network::mojom::WebSocketHandshakeResponsePtr response) override;
   void OnAddChannelResponse(const std::string& selected_protocol,
                             const std::string& extensions) override;
-  void OnDataFrame(bool fin,
-                   network::mojom::WebSocketMessageType type,
-                   const std::vector<uint8_t>& data) override;
-  void OnFlowControl(int64_t quota) override;
-  void OnDropChannel(bool was_clean,
-                     uint16_t code,
-                     const std::string& reason) override;
-  void OnClosingHandshake() override;
 
   // mojom::AuthenticationHandler method:
   void OnAuthRequired(const net::AuthChallengeInfo& auth_info,
@@ -134,9 +126,10 @@ class WebRequestProxyingWebSocket
   InfoMap* const info_map_;
   scoped_refptr<WebRequestAPI::RequestIDGenerator> request_id_generator_;
   network::mojom::WebSocketPtr proxied_socket_;
+  network::mojom::WebSocketHandshakeClientPtr forwarding_handshake_client_;
   network::mojom::WebSocketClientPtr forwarding_client_;
   mojo::Binding<network::mojom::WebSocket> binding_as_websocket_;
-  mojo::Binding<network::mojom::WebSocketClient> binding_as_client_;
+  mojo::Binding<network::mojom::WebSocketHandshakeClient> binding_as_client_;
   mojo::Binding<network::mojom::AuthenticationHandler> binding_as_auth_handler_;
   mojo::Binding<network::mojom::TrustedHeaderClient> binding_as_header_client_;
 

services/network/public/mojom/websocket.mojom

@@ -49,9 +49,7 @@ interface AuthenticationHandler {
                  IPEndPoint remote_endpoint) => (AuthCredentials? credentials);
 };
 
-interface WebSocketClient {
-  OnFailChannel(string reason);
-
+interface WebSocketHandshakeClient {
   // Notify the renderer that the browser has started an opening handshake.
   OnStartOpeningHandshake(WebSocketHandshakeRequest request);
 
@@ -65,7 +63,9 @@ interface WebSocketClient {
   // the server selected, or empty if no sub-protocol was selected.
   // |extensions| is the list of extensions negotiated for the connection.
   OnAddChannelResponse(string selected_protocol, string extensions);
+};
 
+interface WebSocketClient {
   // Receive a non-control frame from the remote server.
   // - |fin| indicates that this frame is the last in the current message.
   // - |type| is the type of the message. On the first frame of a message, it
@@ -104,6 +104,8 @@ 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 {
@@ -123,6 +125,7 @@ interface WebSocket {
                     array<string> requested_protocols,
                     url.mojom.Url first_party_for_cookies,
                     array<HttpHeader> additional_headers,
+                    WebSocketHandshakeClient handshake_client,
                     WebSocketClient client);
 
   // Send a non-control frame to the remote server.

services/network/websocket.cc

@@ -147,7 +147,7 @@ void WebSocket::WebSocketEventHandler::OnAddChannelResponse(
   impl_->handshake_succeeded_ = true;
   impl_->pending_connection_tracker_.OnCompleteHandshake();
 
-  impl_->client_->OnAddChannelResponse(selected_protocol, extensions);
+  impl_->handshake_client_->OnAddChannelResponse(selected_protocol, extensions);
 }
 
 void WebSocket::WebSocketEventHandler::OnDataFrame(
@@ -238,7 +238,7 @@ void WebSocket::WebSocketEventHandler::OnStartOpeningHandshake(
   headers_text.append("\r\n");
   request_to_pass->headers_text = std::move(headers_text);
 
-  impl_->client_->OnStartOpeningHandshake(std::move(request_to_pass));
+  impl_->handshake_client_->OnStartOpeningHandshake(std::move(request_to_pass));
 }
 
 void WebSocket::WebSocketEventHandler::OnFinishOpeningHandshake(
@@ -272,7 +272,8 @@ void WebSocket::WebSocketEventHandler::OnFinishOpeningHandshake(
   headers_text.append("\r\n");
   response_to_pass->headers_text = headers_text;
 
-  impl_->client_->OnFinishOpeningHandshake(std::move(response_to_pass));
+  impl_->handshake_client_->OnFinishOpeningHandshake(
+      std::move(response_to_pass));
 }
 
 void WebSocket::WebSocketEventHandler::OnSSLCertificateError(
@@ -360,6 +361,7 @@ void WebSocket::AddChannelRequest(
     const std::vector<std::string>& requested_protocols,
     const GURL& site_for_cookies,
     std::vector<mojom::HttpHeaderPtr> additional_headers,
+    mojom::WebSocketHandshakeClientPtr handshake_client,
     mojom::WebSocketClientPtr client) {
   DVLOG(3) << "WebSocket::AddChannelRequest @" << reinterpret_cast<void*>(this)
            << " socket_url=\"" << socket_url << "\" requested_protocols=\""
@@ -372,6 +374,7 @@ void WebSocket::AddChannelRequest(
     return;
   }
 
+  handshake_client_ = std::move(handshake_client);
   client_ = std::move(client);
 
   DCHECK(!channel_);
@@ -446,8 +449,9 @@ void WebSocket::StartClosingHandshake(uint16_t code,
   if (!channel_) {
     // WebSocketChannel is not yet created due to the delay introduced by
     // per-renderer WebSocket throttling.
-    if (client_)
+    if (client_) {
       client_->OnDropChannel(false, net::kWebSocketErrorAbnormalClosure, "");
+    }
     return;
   }
 

services/network/websocket.h

@@ -84,6 +84,7 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) WebSocket : public mojom::WebSocket {
                          const std::vector<std::string>& requested_protocols,
                          const GURL& site_for_cookies,
                          std::vector<mojom::HttpHeaderPtr> additional_headers,
+                         mojom::WebSocketHandshakeClientPtr handshake_client,
                          mojom::WebSocketClientPtr client) override;
   void SendFrame(bool fin,
                  mojom::WebSocketMessageType type,
@@ -156,6 +157,7 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) WebSocket : public mojom::WebSocket {
   std::unique_ptr<Delegate> delegate_;
   mojo::Binding<mojom::WebSocket> binding_;
 
+  mojom::WebSocketHandshakeClientPtr handshake_client_;
   mojom::WebSocketClientPtr client_;
   mojom::AuthenticationHandlerPtr auth_handler_;
   mojom::TrustedHeaderClientPtr header_client_;

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

@@ -22,7 +22,7 @@ const uint16_t kAbnormalShutdownOpCode = 1006;
 }  // namespace
 
 WebSocketHandleImpl::WebSocketHandleImpl()
-    : client_(nullptr), client_binding_(this) {
+    : client_(nullptr), handshake_client_binding_(this), client_binding_(this) {
   NETWORK_DVLOG(1) << this << " created";
 }
 
@@ -52,19 +52,23 @@ void WebSocketHandleImpl::Connect(network::mojom::blink::WebSocketPtr websocket,
   DCHECK(client);
   client_ = client;
 
-  network::mojom::blink::WebSocketClientPtr client_proxy;
+  network::mojom::blink::WebSocketHandshakeClientPtr handshake_client_proxy;
   Vector<network::mojom::blink::HttpHeaderPtr> additional_headers;
   if (!user_agent_override.IsNull()) {
     additional_headers.push_back(network::mojom::blink::HttpHeader::New(
         http_names::kUserAgent, user_agent_override));
   }
+  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_handler(WTF::Bind(
+  client_binding_.set_connection_error_with_reason_handler(WTF::Bind(
       &WebSocketHandleImpl::OnConnectionError, WTF::Unretained(this)));
-  websocket_->AddChannelRequest(url, protocols, site_for_cookies,
-                                std::move(additional_headers),
-                                std::move(client_proxy));
+
+  websocket_->AddChannelRequest(
+      url, protocols, site_for_cookies, std::move(additional_headers),
+      std::move(handshake_client_proxy), std::move(client_proxy));
 }
 
 void WebSocketHandleImpl::Send(bool fin,
@@ -121,9 +125,16 @@ void WebSocketHandleImpl::Disconnect() {
   client_ = nullptr;
 }
 
-void WebSocketHandleImpl::OnConnectionError() {
+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");
 }
 

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

@@ -38,8 +38,10 @@
 
 namespace blink {
 
-class WebSocketHandleImpl : public WebSocketHandle,
-                            public network::mojom::blink::WebSocketClient {
+class WebSocketHandleImpl
+    : public WebSocketHandle,
+      public network::mojom::blink::WebSocketHandshakeClient,
+      public network::mojom::blink::WebSocketClient {
  public:
   WebSocketHandleImpl();
   ~WebSocketHandleImpl() override;
@@ -57,16 +59,17 @@ class WebSocketHandleImpl : public WebSocketHandle,
 
  private:
   void Disconnect();
-  void OnConnectionError();
+  void OnConnectionError(uint32_t custom_reason,
+                         const std::string& description);
 
-  // network::mojom::blink::WebSocketClient methods:
-  void OnFailChannel(const String& reason) override;
+  // network::mojom::blink::WebSocketHandshakeClient methods:
   void OnStartOpeningHandshake(
       network::mojom::blink::WebSocketHandshakeRequestPtr) override;
   void OnFinishOpeningHandshake(
       network::mojom::blink::WebSocketHandshakeResponsePtr) override;
   void OnAddChannelResponse(const String& selected_protocol,
                             const String& extensions) override;
+  // network::mojom::blink::WebSocketClient methods:
   void OnDataFrame(bool fin,
                    network::mojom::blink::WebSocketMessageType,
                    const Vector<uint8_t>& data) override;
@@ -75,10 +78,13 @@ class WebSocketHandleImpl : public WebSocketHandle,
                      uint16_t code,
                      const String& reason) override;
   void OnClosingHandshake() override;
+  void OnFailChannel(const String& reason) override;
 
   WebSocketHandleClient* client_;
 
   network::mojom::blink::WebSocketPtr websocket_;
+  mojo::Binding<network::mojom::blink::WebSocketHandshakeClient>
+      handshake_client_binding_;
   mojo::Binding<network::mojom::blink::WebSocketClient> client_binding_;
 };