Revert "[websocket] Split interface mojom::WebSocketClient"

This reverts commit 38cccb45.

Reason for revert: ExtensionWebRequestApiTest.WebSocketRequestAuthRequired failing on linux-chromeos-dbg non_network_service_browser_tests

e.g. https://ci.chromium.org/p/chromium/builders/ci/linux-chromeos-dbg/12545

Original change's description:
> [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/+/1599819
> Reviewed-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}

TBR=kinuko@chromium.org,ricea@chromium.org,rockot@google.com,yhirano@chromium.org,yoichio@chromium.org,karandeepb@chromium.org

Change-Id: I8f974173d329d77258ba5b4ad45a4cb1f283bba0
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 865001, 942989
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1628497Reviewed-by: Timothy Loh <timloh@chromium.org>
Commit-Queue: Timothy Loh <timloh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#662990}

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_FAILED',
+              error: 'net::ERR_ABORTED',
               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_FAILED',
+              error: 'net::ERR_ABORTED',
               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_FAILED',
+              error: 'net::ERR_ABORTED',
               initiator: getDomain(initiators.WEB_INITIATED)
             }
           },

extensions/browser/api/web_request/web_request_proxying_websocket.cc

@@ -78,10 +78,8 @@ 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() || !handshake_client ||
-      forwarding_handshake_client_ || !client || forwarding_client_) {
+  if (binding_as_client_.is_bound() || !client || forwarding_client_) {
     // Illegal request.
     proxied_socket_ = nullptr;
     return;
@@ -98,7 +96,6 @@ 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);
 
@@ -157,15 +154,31 @@ 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_handshake_client_);
-  forwarding_handshake_client_->OnStartOpeningHandshake(std::move(request));
+  DCHECK(forwarding_client_);
+  forwarding_client_->OnStartOpeningHandshake(std::move(request));
 }
 
 void WebRequestProxyingWebSocket::OnFinishOpeningHandshake(
     network::mojom::WebSocketHandshakeResponsePtr response) {
-  DCHECK(forwarding_handshake_client_);
+  DCHECK(forwarding_client_);
 
   // response_.headers will be set in OnBeforeSendHeaders if
   // binding_as_header_client_ is set.
@@ -185,7 +198,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_handshake_client_->OnFinishOpeningHandshake(std::move(response));
+  forwarding_client_->OnFinishOpeningHandshake(std::move(response));
 
   if (!binding_as_header_client_ || response_.headers) {
     ContinueToHeadersReceived();
@@ -218,14 +231,41 @@ void WebRequestProxyingWebSocket::ContinueToHeadersReceived() {
 void WebRequestProxyingWebSocket::OnAddChannelResponse(
     const std::string& selected_protocol,
     const std::string& extensions) {
-  DCHECK(forwarding_handshake_client_);
+  DCHECK(forwarding_client_);
   DCHECK(!is_done_);
   is_done_ = true;
   ExtensionWebRequestEventRouter::GetInstance()->OnCompleted(
       browser_context_, info_map_, &info_.value(), net::ERR_WS_UPGRADE);
 
-  forwarding_handshake_client_->OnAddChannelResponse(selected_protocol,
-                                                     extensions);
+  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();
 }
 
 void WebRequestProxyingWebSocket::OnAuthRequired(
@@ -370,7 +410,7 @@ void WebRequestProxyingWebSocket::OnBeforeSendHeadersComplete(
 }
 
 void WebRequestProxyingWebSocket::ContinueToStartRequest(int error_code) {
-  network::mojom::WebSocketHandshakeClientPtr proxy;
+  network::mojom::WebSocketClientPtr proxy;
 
   base::flat_set<std::string> used_header_names;
   std::vector<network::mojom::HttpHeaderPtr> additional_headers;
@@ -392,8 +432,7 @@ 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(forwarding_client_));
+      std::move(additional_headers), std::move(proxy));
 }
 
 void WebRequestProxyingWebSocket::OnHeadersReceivedComplete(int error_code) {
@@ -483,6 +522,8 @@ 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::WebSocketHandshakeClient,
+      public network::mojom::WebSocketClient,
       public network::mojom::AuthenticationHandler,
       public network::mojom::TrustedHeaderClient {
  public:
@@ -61,7 +61,6 @@ 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,
@@ -69,13 +68,22 @@ class WebRequestProxyingWebSocket
   void SendFlowControl(int64_t quota) override;
   void StartClosingHandshake(uint16_t code, const std::string& reason) override;
 
-  // mojom::WebSocketHandShakeClient methods:
+  // mojom::WebSocketClient methods:
+  void OnFailChannel(const std::string& reason) override;
   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,
@@ -126,10 +134,9 @@ 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::WebSocketHandshakeClient> binding_as_client_;
+  mojo::Binding<network::mojom::WebSocketClient> 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,7 +49,9 @@ interface AuthenticationHandler {
                  IPEndPoint remote_endpoint) => (AuthCredentials? credentials);
 };
 
-interface WebSocketHandshakeClient {
+interface WebSocketClient {
+  OnFailChannel(string reason);
+
   // Notify the renderer that the browser has started an opening handshake.
   OnStartOpeningHandshake(WebSocketHandshakeRequest request);
 
@@ -63,9 +65,7 @@ interface WebSocketHandshakeClient {
   // 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,8 +104,6 @@ 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 {
@@ -125,7 +123,6 @@ 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_->handshake_client_->OnAddChannelResponse(selected_protocol, extensions);
+  impl_->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_->handshake_client_->OnStartOpeningHandshake(std::move(request_to_pass));
+  impl_->client_->OnStartOpeningHandshake(std::move(request_to_pass));
 }
 
 void WebSocket::WebSocketEventHandler::OnFinishOpeningHandshake(
@@ -272,8 +272,7 @@ void WebSocket::WebSocketEventHandler::OnFinishOpeningHandshake(
   headers_text.append("\r\n");
   response_to_pass->headers_text = headers_text;
 
-  impl_->handshake_client_->OnFinishOpeningHandshake(
-      std::move(response_to_pass));
+  impl_->client_->OnFinishOpeningHandshake(std::move(response_to_pass));
 }
 
 void WebSocket::WebSocketEventHandler::OnSSLCertificateError(
@@ -361,7 +360,6 @@ 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=\""
@@ -374,7 +372,6 @@ void WebSocket::AddChannelRequest(
     return;
   }
 
-  handshake_client_ = std::move(handshake_client);
   client_ = std::move(client);
 
   DCHECK(!channel_);
@@ -449,9 +446,8 @@ 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,7 +84,6 @@ 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,
@@ -157,7 +156,6 @@ 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), handshake_client_binding_(this), client_binding_(this) {
+    : client_(nullptr), client_binding_(this) {
   NETWORK_DVLOG(1) << this << " created";
 }
 
@@ -52,23 +52,19 @@ void WebSocketHandleImpl::Connect(network::mojom::blink::WebSocketPtr websocket,
   DCHECK(client);
   client_ = client;
 
-  network::mojom::blink::WebSocketHandshakeClientPtr handshake_client_proxy;
+  network::mojom::blink::WebSocketClientPtr 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_with_reason_handler(WTF::Bind(
+  client_binding_.set_connection_error_handler(WTF::Bind(
       &WebSocketHandleImpl::OnConnectionError, WTF::Unretained(this)));
-
-  websocket_->AddChannelRequest(
-      url, protocols, site_for_cookies, std::move(additional_headers),
-      std::move(handshake_client_proxy), std::move(client_proxy));
+  websocket_->AddChannelRequest(url, protocols, site_for_cookies,
+                                std::move(additional_headers),
+                                std::move(client_proxy));
 }
 
 void WebSocketHandleImpl::Send(bool fin,
@@ -125,16 +121,9 @@ void WebSocketHandleImpl::Disconnect() {
   client_ = nullptr;
 }
 
-void WebSocketHandleImpl::OnConnectionError(uint32_t custom_reason,
-                                            const std::string& description) {
+void WebSocketHandleImpl::OnConnectionError() {
   // 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,10 +38,8 @@
 
 namespace blink {
 
-class WebSocketHandleImpl
-    : public WebSocketHandle,
-      public network::mojom::blink::WebSocketHandshakeClient,
-      public network::mojom::blink::WebSocketClient {
+class WebSocketHandleImpl : public WebSocketHandle,
+                            public network::mojom::blink::WebSocketClient {
  public:
   WebSocketHandleImpl();
   ~WebSocketHandleImpl() override;
@@ -59,17 +57,16 @@ class WebSocketHandleImpl
 
  private:
   void Disconnect();
-  void OnConnectionError(uint32_t custom_reason,
-                         const std::string& description);
+  void OnConnectionError();
 
-  // network::mojom::blink::WebSocketHandshakeClient methods:
+  // network::mojom::blink::WebSocketClient methods:
+  void OnFailChannel(const String& reason) override;
   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;
@@ -78,13 +75,10 @@ class WebSocketHandleImpl
                      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_;
 };