Fix H2+WebSockets when using an HTTPS proxy.

When using an HTTPS proxy, WebSockets would always try to use an
established H2 session for WebSockets, even if H2 over WebSockets was
disabled. Moreover, if H2 over WebSockets was disabled, it would even
use an H2 session that hadn't advertised WebSocket support.

These problems were a result of only checking the HTTPS proxy server
type, and not the scheme, when deciding if a request should try and
use H2, and using the wrong value when telling the H2 code that only
a session supporting WebSockets was needed.

This CL fixes these issues.

Bug: 1009612
Change-Id: Ic8ffdac454f3e079e47b8c35b1a5b4d8cac807e5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1835253
Commit-Queue: Matt Menke <mmenke@chromium.org>
Reviewed-by: Adam Rice <ricea@chromium.org>
Reviewed-by: Bence Béky <bnc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#702441}

net/http/http_stream_factory_job.cc

@@ -397,12 +397,17 @@ bool HttpStreamFactory::Job::CanUseExistingSpdySession() const {
     return false;
   }
 
+  if (is_websocket_)
+    return try_websocket_over_http2_;
+
+  DCHECK(origin_url_.SchemeIsHTTPOrHTTPS());
+
   // We need to make sure that if a HTTP/2 session was created for
   // https://somehost/ then we do not use that session for http://somehost:443/.
   // The only time we can use an existing session is if the request URL is
   // https (the normal case) or if we are connecting to a HTTP/2 proxy.
   // https://crbug.com/133176
-  return origin_url_.SchemeIs(url::kHttpsScheme) || try_websocket_over_http2_ ||
+  return origin_url_.SchemeIs(url::kHttpsScheme) ||
          proxy_info_.proxy_server().is_https();
 }
 
@@ -792,9 +797,9 @@ int HttpStreamFactory::Job::DoInitConnectionImpl() {
 
         bool is_blocking_request_for_session;
         existing_spdy_session_ = session_->spdy_session_pool()->RequestSession(
-            spdy_session_key_, enable_ip_based_pooling_,
-            try_websocket_over_http2_, net_log_, resume_callback, this,
-            &spdy_session_request_, &is_blocking_request_for_session);
+            spdy_session_key_, enable_ip_based_pooling_, is_websocket_,
+            net_log_, resume_callback, this, &spdy_session_request_,
+            &is_blocking_request_for_session);
         if (!existing_spdy_session_ && should_throttle_connect &&
             !is_blocking_request_for_session) {
           net_log_.AddEvent(NetLogEventType::HTTP_STREAM_JOB_THROTTLED);
@@ -813,8 +818,8 @@ int HttpStreamFactory::Job::DoInitConnectionImpl() {
         // callback.
         existing_spdy_session_ =
             session_->spdy_session_pool()->FindAvailableSession(
-                spdy_session_key_, enable_ip_based_pooling_,
-                try_websocket_over_http2_, net_log_);
+                spdy_session_key_, enable_ip_based_pooling_, is_websocket_,
+                net_log_);
       }
     }
     if (existing_spdy_session_) {
@@ -950,6 +955,13 @@ int HttpStreamFactory::Job::DoInitConnectionComplete(int result) {
       was_alpn_negotiated_ = true;
       negotiated_protocol_ = proxy_socket->GetProxyNegotiatedProtocol();
       using_spdy_ = true;
+
+      // Using unencrypted websockets over an H2 proxy is not currently
+      // supported.
+      // TODO(mmenke): Should this case be treated like
+      // |try_websocket_over_http2_|, or should we force HTTP/1.1?
+      if (is_websocket_ && !try_websocket_over_http2_)
+        return ERR_NOT_IMPLEMENTED;
     }
   }
 
@@ -1091,7 +1103,7 @@ int HttpStreamFactory::Job::DoCreateStream() {
     // WebSocket over HTTP/2 is only allowed to use existing HTTP/2 connections.
     // Therefore |using_spdy_| could not have been set unless a connection had
     // already been found.
-    DCHECK(!try_websocket_over_http2_);
+    DCHECK(!is_websocket_);
 
     session_->spdy_session_pool()->push_promise_index()->ClaimPushedStream(
         spdy_session_key_, origin_url_, request_info_, &existing_spdy_session_,

net/spdy/spdy_network_transaction_unittest.cc

@@ -8555,8 +8555,6 @@ TEST_F(SpdyNetworkTransactionTest,
   ssl_provider2->next_protos_expected_in_ssl_config = NextProtoVector{};
   // Force socket to use HTTP/1.1, the default protocol without ALPN.
   ssl_provider2->next_proto = kProtoHTTP11;
-  // ssl_provider2->ssl_info.cert =
-  //  ImportCertFromFile(GetTestCertsDirectory(), "spdy_pooling.pem");
   helper.AddDataWithSSLSocketDataProvider(&data2, std::move(ssl_provider2));
 
   TestCompletionCallback callback1;
@@ -8749,6 +8747,157 @@ TEST_F(SpdyNetworkTransactionTest, WebSocketOverHTTP2) {
                                       /* expected_count = */ 1);
 }
 
+// Make sure that a WebSocket job doesn't pick up a newly created SpdySession
+// that supports WebSockets through an HTTPS proxy when an H2 server doesn't
+// support websockets and |enable_websocket_over_http2| is false. See
+// https://crbug.com/1010491.
+TEST_F(SpdyNetworkTransactionTest,
+       WebSocketDoesNotUseNewH2SessionWithoutWebSocketSupportOverHttpsProxy) {
+  auto session_deps = std::make_unique<SpdySessionDependencies>(
+      ProxyResolutionService::CreateFixed("https://proxy:70",
+                                          TRAFFIC_ANNOTATION_FOR_TESTS));
+
+  // Note: Once WebSocket over H2 is enabled by default, this line can be
+  // deleted, and this test will still be useful to keep, though its description
+  // will need to be updated.
+  session_deps->enable_websocket_over_http2 = false;
+
+  NormalSpdyTransactionHelper helper(request_, HIGHEST, log_,
+                                     std::move(session_deps));
+  helper.RunPreTestSetup();
+
+  spdy::SpdySerializedFrame req(
+      spdy_util_.ConstructSpdyGet(nullptr, 0, 1, HIGHEST));
+
+  MockWrite writes[] = {MockWrite(SYNCHRONOUS, 0,
+                                  "CONNECT www.example.org:443 HTTP/1.1\r\n"
+                                  "Host: www.example.org:443\r\n"
+                                  "Proxy-Connection: keep-alive\r\n\r\n"),
+                        CreateMockWrite(req, 2)};
+
+  spdy::SpdySerializedFrame resp1(
+      spdy_util_.ConstructSpdyGetReply(nullptr, 0, 1));
+  spdy::SpdySerializedFrame body1(spdy_util_.ConstructSpdyDataFrame(1, true));
+  MockRead reads[] = {MockRead(SYNCHRONOUS, 1, "HTTP/1.1 200 OK\r\n\r\n"),
+                      CreateMockRead(resp1, 3), CreateMockRead(body1, 4),
+                      MockRead(SYNCHRONOUS, ERR_IO_PENDING, 5)};
+
+  // SSL data for the proxy.
+  SSLSocketDataProvider tunnel_ssl_data(ASYNC, OK);
+  helper.session_deps()->socket_factory->AddSSLSocketDataProvider(
+      &tunnel_ssl_data);
+
+  SequencedSocketData data(
+      // Just as with other operations, this means to pause during connection
+      // establishment.
+      MockConnect(ASYNC, ERR_IO_PENDING), reads, writes);
+  helper.AddData(&data);
+
+  MockWrite writes2[] = {
+      MockWrite(SYNCHRONOUS, 0,
+                "CONNECT www.example.org:443 HTTP/1.1\r\n"
+                "Host: www.example.org:443\r\n"
+                "Proxy-Connection: keep-alive\r\n\r\n"),
+      MockWrite(SYNCHRONOUS, 2,
+                "GET / HTTP/1.1\r\n"
+                "Host: www.example.org\r\n"
+                "Connection: Upgrade\r\n"
+                "Upgrade: websocket\r\n"
+                "Origin: http://www.example.org\r\n"
+                "Sec-WebSocket-Version: 13\r\n"
+                "Sec-WebSocket-Key: dGhlIHNhbXBsZSBub25jZQ==\r\n"
+                "Sec-WebSocket-Extensions: permessage-deflate; "
+                "client_max_window_bits\r\n\r\n")};
+
+  MockRead reads2[] = {
+      MockRead(SYNCHRONOUS, 1, "HTTP/1.1 200 OK\r\n\r\n"),
+      MockRead(SYNCHRONOUS, 3,
+               "HTTP/1.1 101 Switching Protocols\r\n"
+               "Upgrade: websocket\r\n"
+               "Connection: Upgrade\r\n"
+               "Sec-WebSocket-Accept: s3pPLMBiTxaQ9kYGzzhZRbK+xOo=\r\n\r\n")};
+  SequencedSocketData data2(MockConnect(ASYNC, ERR_IO_PENDING), reads2,
+                            writes2);
+
+  // SSL data for the proxy.
+  SSLSocketDataProvider tunnel_ssl_data2(ASYNC, OK);
+  helper.session_deps()->socket_factory->AddSSLSocketDataProvider(
+      &tunnel_ssl_data2);
+
+  auto ssl_provider2 = std::make_unique<SSLSocketDataProvider>(ASYNC, OK);
+  // Test that request has empty |alpn_protos|, that is, HTTP/2 is disabled.
+  ssl_provider2->next_protos_expected_in_ssl_config = NextProtoVector{};
+  // Force socket to use HTTP/1.1, the default protocol without ALPN.
+  ssl_provider2->next_proto = kProtoHTTP11;
+  helper.AddDataWithSSLSocketDataProvider(&data2, std::move(ssl_provider2));
+
+  TestCompletionCallback callback1;
+  int rv = helper.trans()->Start(&request_, callback1.callback(), log_);
+  ASSERT_THAT(rv, IsError(ERR_IO_PENDING));
+
+  // Create HTTP/2 connection.
+  base::RunLoop().RunUntilIdle();
+
+  HttpRequestInfo request2;
+  request2.method = "GET";
+  request2.url = GURL("wss://www.example.org/");
+  request2.traffic_annotation =
+      net::MutableNetworkTrafficAnnotationTag(TRAFFIC_ANNOTATION_FOR_TESTS);
+  EXPECT_TRUE(HostPortPair::FromURL(request_.url)
+                  .Equals(HostPortPair::FromURL(request2.url)));
+  request2.extra_headers.SetHeader("Connection", "Upgrade");
+  request2.extra_headers.SetHeader("Upgrade", "websocket");
+  request2.extra_headers.SetHeader("Origin", "http://www.example.org");
+  request2.extra_headers.SetHeader("Sec-WebSocket-Version", "13");
+
+  TestWebSocketHandshakeStreamCreateHelper websocket_stream_create_helper;
+
+  HttpNetworkTransaction trans2(MEDIUM, helper.session());
+  trans2.SetWebSocketHandshakeStreamCreateHelper(
+      &websocket_stream_create_helper);
+
+  TestCompletionCallback callback2;
+  rv = trans2.Start(&request2, callback2.callback(), log_);
+  ASSERT_THAT(rv, IsError(ERR_IO_PENDING));
+
+  // Run until waiting on both connections.
+  base::RunLoop().RunUntilIdle();
+
+  // The H2 connection completes.
+  data.socket()->OnConnectComplete(MockConnect(SYNCHRONOUS, OK));
+  EXPECT_EQ(OK, callback1.WaitForResult());
+  const HttpResponseInfo* response = helper.trans()->GetResponseInfo();
+  ASSERT_TRUE(response->headers);
+  EXPECT_TRUE(response->was_fetched_via_spdy);
+  EXPECT_EQ("HTTP/1.1 200", response->headers->GetStatusLine());
+  std::string response_data;
+  rv = ReadTransaction(helper.trans(), &response_data);
+  EXPECT_THAT(rv, IsOk());
+  EXPECT_EQ("hello!", response_data);
+
+  SpdySessionKey key(
+      HostPortPair::FromURL(request_.url),
+      ProxyServer::FromURI("https://proxy:70", ProxyServer::SCHEME_HTTPS),
+      PRIVACY_MODE_DISABLED, SpdySessionKey::IsProxySession::kFalse,
+      SocketTag());
+
+  base::WeakPtr<SpdySession> spdy_session =
+      helper.session()->spdy_session_pool()->FindAvailableSession(
+          key, /* enable_ip_based_pooling = */ true,
+          /* is_websocket = */ false, log_);
+  ASSERT_TRUE(spdy_session);
+  EXPECT_FALSE(spdy_session->support_websocket());
+
+  EXPECT_FALSE(callback2.have_result());
+
+  // Create WebSocket stream.
+  data2.socket()->OnConnectComplete(MockConnect(SYNCHRONOUS, OK));
+
+  rv = callback2.WaitForResult();
+  ASSERT_THAT(rv, IsOk());
+  helper.VerifyDataConsumed();
+}
+
 // Same as above, but checks that a WebSocket connection avoids creating a new
 // socket if it detects an H2 session when host resolution completes, and
 // requests also use different hostnames.