Commit fbeb883a authored by Bence Béky's avatar Bence Béky Committed by Commit Bot

Do not crash if WebSocket server negotiates HTTP/2.

I overzealously landed https://crrev.com/c/990493 assuming that if a TCP
connection is open (directly, not through a proxy) with an empty ALPN
list, then the server cannot negotiate HTTP/2.  While there are no
crashes seen during the first day of Canary, https://crbug.com/828865
shows that it is unwise to rely on the TLS layer to fail connection
negotiation if server's ALPN response does not match the client
handshake.  While it is still unclear if this crash can occur in the
while, I am adding a unittest in this CL that triggers it, and removing
the CHECK in favor of just return ERR_NOT_IMPLEMENTED in this weird
case.

Bug: 819101
Change-Id: I2c2f70c62485714e61176d75f95d93a67484b800
Reviewed-on: https://chromium-review.googlesource.com/997152
Commit-Queue: Bence Béky <bnc@chromium.org>
Reviewed-by: default avatarRyan Hamilton <rch@chromium.org>
Cr-Commit-Position: refs/heads/master@{#548609}
parent 0a0af3cd
......@@ -1057,13 +1057,6 @@ int HttpStreamFactoryImpl::Job::DoInitConnectionComplete(int result) {
base::Bind(&NetLogHttpStreamProtoCallback, negotiated_protocol_));
if (negotiated_protocol_ == kProtoHTTP2) {
if (is_websocket_) {
// If request is WebSocket with no proxy, then HTTP/2 must not have
// been advertised in the TLS handshake. The TLS layer must not
// have accepted the server choosing HTTP/2.
// TODO(bnc): Change to DCHECK once https://crbug.com/819101 is
// fixed.
CHECK(!proxy_info_.is_direct());
// WebSocket is not supported over a fresh HTTP/2 connection.
return ERR_NOT_IMPLEMENTED;
}
......
......@@ -7217,6 +7217,47 @@ TEST_F(SpdyNetworkTransactionTest, WebSocketOverHTTP2) {
helper.VerifyDataConsumed();
}
TEST_F(SpdyNetworkTransactionTest, WebSocketNegotiatesHttp2) {
HttpRequestInfo request;
request.method = "GET";
request.url = GURL("wss://www.example.org/");
request.traffic_annotation =
net::MutableNetworkTrafficAnnotationTag(TRAFFIC_ANNOTATION_FOR_TESTS);
EXPECT_TRUE(HostPortPair::FromURL(request_.url)
.Equals(HostPortPair::FromURL(request.url)));
request.extra_headers.SetHeader("Connection", "Upgrade");
request.extra_headers.SetHeader("Upgrade", "websocket");
request.extra_headers.SetHeader("Origin", "http://www.example.org");
request.extra_headers.SetHeader("Sec-WebSocket-Version", "13");
NormalSpdyTransactionHelper helper(request_, DEFAULT_PRIORITY, log_, nullptr);
helper.RunPreTestSetup();
StaticSocketDataProvider data(nullptr, 0, nullptr, 0);
auto ssl_provider = std::make_unique<SSLSocketDataProvider>(ASYNC, OK);
// Test that request has empty |alpn_protos|, that is, HTTP/2 is disabled.
ssl_provider->next_protos_expected_in_ssl_config = NextProtoVector{};
// Force socket to use HTTP/1.1, the default protocol without ALPN.
ssl_provider->next_proto = kProtoHTTP2;
ssl_provider->ssl_info.cert =
ImportCertFromFile(GetTestCertsDirectory(), "spdy_pooling.pem");
helper.AddDataWithSSLSocketDataProvider(&data, std::move(ssl_provider));
HttpNetworkTransaction* trans = helper.trans();
TestWebSocketHandshakeStreamCreateHelper websocket_stream_create_helper;
trans->SetWebSocketHandshakeStreamCreateHelper(
&websocket_stream_create_helper);
TestCompletionCallback callback;
int rv = trans->Start(&request, callback.callback(), log_);
ASSERT_THAT(rv, IsError(ERR_IO_PENDING));
rv = callback.WaitForResult();
ASSERT_THAT(rv, IsError(ERR_NOT_IMPLEMENTED));
helper.VerifyDataConsumed();
}
// Plaintext WebSocket over HTTP/2 is not implemented, see
// https://crbug.com/684681.
TEST_F(SpdyNetworkTransactionTest, PlaintextWebSocketOverHttp2Proxy) {
......
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