Skip to content

GitLab

T
tangled
Commit 01b3aaef authored by Adam Rice's avatar Adam Rice Committed by Commit Bot
Browse files
Options

WebRequestProxyingWebSocket: Wait for renderer to close mojo 

WebRequestProxyingWebSocket actively tore down mojo connections in
response to normal WebSocket termination. This led to race conditions
where the error for the teardown would be reported to JavaScript before
the correct termination reason.

Wait for the render process to tear down mojo connections for normal
WebSocket termination. This ensures the correct termination reason is
reported to JavaScript.

BUG=937790

Change-Id: I747224ff77bd17b8ef5bff71d743a66cad5da79d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1508475
Commit-Queue: Adam Rice <ricea@chromium.org>
Reviewed-by: default avatarKen Rockot <rockot@google.com>
Reviewed-by: default avatarYutaka Hirano <yhirano@chromium.org>
Cr-Commit-Position: refs/heads/master@{#645705}
parent de4be6bc
Hide whitespace changes
Inline Side-by-side
Showing with 125 additions and 27 deletions
chrome/browser/extensions/api/web_request/web_request_apitest.cc
View file @ 01b3aaef
......@@ -1563,6 +1563,17 @@ IN_PROC_BROWSER_TEST_F(ExtensionWebRequestApiTest,
web_socket.reset();
}
// Tests that a clean close from the server is not reported as an error when
// there is a race between OnDropChannel and SendFrame.
// Regression test for https://crbug.com/937790.
IN_PROC_BROWSER_TEST_F(ExtensionWebRequestApiTest, WebSocketCleanClose) {
ASSERT_TRUE(StartEmbeddedTestServer());
ASSERT_TRUE(StartWebSocketServer(net::GetWebSocketTestDataDirectory()));
ASSERT_TRUE(
RunExtensionSubtest("webrequest", "test_websocket_clean_close.html"))
<< message_;
}
// Test behavior when intercepting requests from a browser-initiated url fetch.
IN_PROC_BROWSER_TEST_F(ExtensionWebRequestApiTest,
WebRequestURLLoaderInterception) {
......
chrome/test/data/extensions/api_test/webrequest/test_websocket_clean_close.html 0 → 100644
View file @ 01b3aaef
<!--
* Copyright 2019 The Chromium Authors. All rights reserved.
* Use of this source code is governed by a BSD-style license that can be
* found in the LICENSE file.
-->
<body>
<script src="framework.js"></script>
<script src="test_websocket_clean_close.js"></script>
</body>
chrome/test/data/extensions/api_test/webrequest/test_websocket_clean_close.js 0 → 100644
View file @ 01b3aaef
// Copyright 2019 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
// Opens a WebSocket, busy waits for 100ms, sends a message. Verifies that the
// close was clean. If it fails, it will fail flakily, so repeat it 10 times to
// get a deterministic answer.
function sendDoesntError(iteration = 0, done = undefined) {
let ws = new WebSocket('ws://localhost:' + testWebSocketPort +
'/close-immediately');
if (!done)
done = chrome.test.callbackAdded();
ws.onclose = event => {
chrome.test.log('WebSocket ' + iteration + ' closed ' +
(event.wasClean ? 'cleanly.' : 'uncleanly.'));
chrome.test.assertTrue(event.wasClean);
if (iteration < 10) {
++iteration;
sendDoesntError(iteration, done);
} else {
done();
}
}
ws.onopen = () => {
chrome.test.log('WebSocket ' + iteration + ' opened.');
const start = performance.now();
while (performance.now() - start < 100) {}
ws.send('message');
};
}
chrome.tabs.getCurrent(tab => runTestsForTab([sendDoesntError], tab));
extensions/browser/api/web_request/web_request_proxying_websocket.cc
View file @ 01b3aaef
......@@ -44,12 +44,10 @@ WebRequestProxyingWebSocket::WebRequestProxyingWebSocket(
binding_as_websocket_.Bind(std::move(proxied_request));
binding_as_auth_handler_.Bind(std::move(auth_request));
binding_as_websocket_.set_connection_error_handler(
base::BindRepeating(&WebRequestProxyingWebSocket::OnError,
base::Unretained(this), net::ERR_FAILED));
binding_as_auth_handler_.set_connection_error_handler(
base::BindRepeating(&WebRequestProxyingWebSocket::OnError,
base::Unretained(this), net::ERR_FAILED));
binding_as_websocket_.set_connection_error_handler(base::BindOnce(
&WebRequestProxyingWebSocket::OnMojoError, base::Unretained(this)));
binding_as_auth_handler_.set_connection_error_handler(base::BindOnce(
&WebRequestProxyingWebSocket::OnMojoError, base::Unretained(this)));
if (header_client_request)
binding_as_header_client_.Bind(std::move(header_client_request));
......@@ -122,7 +120,7 @@ void WebRequestProxyingWebSocket::AddChannelRequest(
DCHECK(!should_collapse_initiator);
if (result == net::ERR_BLOCKED_BY_CLIENT) {
OnError(result);
HandleErrorDuringHandshake(result);
return;
}
......@@ -164,7 +162,8 @@ void WebRequestProxyingWebSocket::OnFailChannel(const std::string& reason) {
rv = net::ERR_ABORTED;
}
OnError(rv);
DoErrorOccurredIfNeeded(rv);
// Wait for the render process to delete us to avoid race conditions.
}
void WebRequestProxyingWebSocket::OnStartOpeningHandshake(
......@@ -175,6 +174,11 @@ void WebRequestProxyingWebSocket::OnStartOpeningHandshake(
void WebRequestProxyingWebSocket::OnFinishOpeningHandshake(
network::mojom::WebSocketHandshakeResponsePtr response) {
if (is_handshake_done_) {
LOG(WARNING) << "Network service called OnFinishOpeningHandshake too late";
return;
}
DCHECK(forwarding_client_);
// response_.headers will be set in OnBeforeSendHeaders if
......@@ -213,7 +217,7 @@ void WebRequestProxyingWebSocket::ContinueToHeadersReceived() {
response_.headers.get(), &override_headers_, &redirect_url_);
if (result == net::ERR_BLOCKED_BY_CLIENT) {
OnError(result);
HandleErrorDuringHandshake(result);
return;
}
......@@ -229,8 +233,13 @@ void WebRequestProxyingWebSocket::OnAddChannelResponse(
const std::string& selected_protocol,
const std::string& extensions) {
DCHECK(forwarding_client_);
DCHECK(!is_done_);
is_done_ = true;
if (is_handshake_done_) {
DLOG(WARNING)
<< "Multiple OnAddChannelResponse messages from network service";
return;
}
is_handshake_done_ = true;
ExtensionWebRequestEventRouter::GetInstance()->OnCompleted(
browser_context_, info_map_, &info_.value(), net::ERR_WS_UPGRADE);
......@@ -257,7 +266,7 @@ void WebRequestProxyingWebSocket::OnDropChannel(bool was_clean,
forwarding_client_->OnDropChannel(was_clean, code, reason);
forwarding_client_ = nullptr;
OnError(net::ERR_FAILED);
// Wait for the render process to delete us to do cleanup.
}
void WebRequestProxyingWebSocket::OnClosingHandshake() {
......@@ -270,8 +279,13 @@ void WebRequestProxyingWebSocket::OnAuthRequired(
const scoped_refptr<net::HttpResponseHeaders>& headers,
const net::IPEndPoint& remote_endpoint,
OnAuthRequiredCallback callback) {
if (is_handshake_done_) {
LOG(WARNING) << "Unexpected call to OnAuthRequired from network service";
return;
}
if (!auth_info || !callback) {
OnError(net::ERR_FAILED);
HandleErrorDuringHandshake(net::ERR_FAILED);
return;
}
......@@ -287,7 +301,7 @@ void WebRequestProxyingWebSocket::OnAuthRequired(
response_.headers.get(), &override_headers_, &redirect_url_);
if (result == net::ERR_BLOCKED_BY_CLIENT) {
OnError(result);
HandleErrorDuringHandshake(result);
return;
}
......@@ -357,7 +371,7 @@ void WebRequestProxyingWebSocket::OnBeforeRequestComplete(int error_code) {
DCHECK(binding_as_header_client_ || !binding_as_client_.is_bound());
DCHECK(request_.url.SchemeIsWSOrWSS());
if (error_code != net::OK) {
OnError(error_code);
HandleErrorDuringHandshake(error_code);
return;
}
......@@ -371,7 +385,7 @@ void WebRequestProxyingWebSocket::OnBeforeRequestComplete(int error_code) {
&request_.headers);
if (result == net::ERR_BLOCKED_BY_CLIENT) {
OnError(result);
HandleErrorDuringHandshake(result);
return;
}
......@@ -385,7 +399,7 @@ void WebRequestProxyingWebSocket::OnBeforeRequestComplete(int error_code) {
void WebRequestProxyingWebSocket::OnBeforeSendHeadersComplete(int error_code) {
DCHECK(binding_as_header_client_ || !binding_as_client_.is_bound());
if (error_code != net::OK) {
OnError(error_code);
HandleErrorDuringHandshake(error_code);
return;
}
......@@ -412,9 +426,8 @@ void WebRequestProxyingWebSocket::ContinueToStartRequest(int error_code) {
}
binding_as_client_.Bind(mojo::MakeRequest(&proxy));
binding_as_client_.set_connection_error_handler(
base::BindOnce(&WebRequestProxyingWebSocket::OnError,
base::Unretained(this), net::ERR_FAILED));
binding_as_client_.set_connection_error_handler(base::BindOnce(
&WebRequestProxyingWebSocket::OnMojoError, base::Unretained(this)));
proxied_socket_->AddChannelRequest(
request_.url, websocket_protocols_, request_.site_for_cookies,
std::move(additional_headers), std::move(proxy));
......@@ -422,7 +435,7 @@ void WebRequestProxyingWebSocket::ContinueToStartRequest(int error_code) {
void WebRequestProxyingWebSocket::OnHeadersReceivedComplete(int error_code) {
if (error_code != net::OK) {
OnError(error_code);
HandleErrorDuringHandshake(error_code);
return;
}
......@@ -467,7 +480,7 @@ void WebRequestProxyingWebSocket::OnHeadersReceivedCompleteForAuth(
scoped_refptr<net::AuthChallengeInfo> auth_info,
int rv) {
if (rv != net::OK) {
OnError(rv);
HandleErrorDuringHandshake(rv);
return;
}
ResumeIncomingMethodCallProcessing();
......@@ -500,13 +513,26 @@ void WebRequestProxyingWebSocket::ResumeIncomingMethodCallProcessing() {
binding_as_header_client_.ResumeIncomingMethodCallProcessing();
}
void WebRequestProxyingWebSocket::OnError(int error_code) {
if (!is_done_ && info_.has_value()) {
is_done_ = true;
void WebRequestProxyingWebSocket::DoErrorOccurredIfNeeded(int error_code) {
if (!is_handshake_done_ && info_.has_value()) {
is_handshake_done_ = true;
ExtensionWebRequestEventRouter::GetInstance()->OnErrorOccurred(
browser_context_, info_map_, &info_.value(), true /* started */,
error_code);
}
}
void WebRequestProxyingWebSocket::OnMojoError() {
HandleGenericError(net::ERR_FAILED);
}
void WebRequestProxyingWebSocket::HandleErrorDuringHandshake(int error_code) {
DCHECK(!is_handshake_done_);
HandleGenericError(error_code);
}
void WebRequestProxyingWebSocket::HandleGenericError(int error_code) {
DoErrorOccurredIfNeeded(error_code);
if (forwarding_client_)
forwarding_client_->OnFailChannel(net::ErrorToString(error_code));
......
extensions/browser/api/web_request/web_request_proxying_websocket.h
View file @ 01b3aaef
......@@ -123,7 +123,10 @@ class WebRequestProxyingWebSocket
void PauseIncomingMethodCallProcessing();
void ResumeIncomingMethodCallProcessing();
void OnError(int result);
void DoErrorOccurredIfNeeded(int error_code);
void OnMojoError();
void HandleErrorDuringHandshake(int error_code);
void HandleGenericError(int error_code);
const int process_id_;
const int render_frame_id_;
......@@ -150,7 +153,7 @@ class WebRequestProxyingWebSocket
OnHeadersReceivedCallback on_headers_received_callback_;
GURL redirect_url_;
bool is_done_ = false;
bool is_handshake_done_ = false;
bool waiting_for_header_client_headers_received_ = false;
base::Optional<WebRequestInfo> info_;
......
net/data/websocket/README
View file @ 01b3aaef
......@@ -47,6 +47,9 @@ Multiple tests may share one resource, or URI handler.
Used by kinds of PPAPI tests for WebSocket, ExtensionApiTest.WebSocket,
and WorkerTest.WebSocketSharedWorker.
- close-immediately_wsh.py : A WebSocket URL handler that performs an immediate
clean close as soon as the connection is established.
- close_wsh.py : A WebSocket URL handler for testing outgoing close code and
reason.
Used by kinds of PPAPI tests for WebSocket.
......
net/data/websocket/close-immediately_wsh.py 0 → 100644
View file @ 01b3aaef
# Copyright (c) 2012 The Chromium Authors. All rights reserved.
# Use of this source code is governed by a BSD-style license that can be
# found in the LICENSE file.
def web_socket_do_extra_handshake(_request):
pass # Always accept.
def web_socket_transfer_data(_request):
pass # Close immediately
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment