Fix HttpServerProperties' handling of WebSockets, alternative approach.

It was using separate HostSchemePorts for wss/https and ws/http. This CL
makes it normalize scheme to https and http.

Bug: 997241
Change-Id: Ic0688f6eb40a6a70f940ae7c4547c9d1475baaa2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1822819
Commit-Queue: Matt Menke <mmenke@chromium.org>
Reviewed-by: Zhongyi Shi <zhongyi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#700300}

net/http/http_network_transaction.cc

@@ -326,8 +326,7 @@ void HttpNetworkTransaction::PrepareForAuthRestart(HttpAuth::Target target) {
   if (target == HttpAuth::AUTH_SERVER &&
       auth_controllers_[target]->NeedsHTTP11()) {
     session_->http_server_properties()->SetHTTP11Required(
-        HttpServerProperties::GetNormalizedSchemeHostPort(request_->url),
-        network_isolation_key_);
+        url::SchemeHostPort(request_->url), network_isolation_key_);
   }
 
   bool keep_alive = false;

net/http/http_server_properties.h

@@ -164,15 +164,21 @@ class NET_EXPORT HttpServerProperties
 
   struct NET_EXPORT ServerInfoMapKey {
     // If |use_network_isolation_key| is false, an empty NetworkIsolationKey is
-    // used instead of |network_isolation_key|.
-    ServerInfoMapKey(const url::SchemeHostPort& server,
+    // used instead of |network_isolation_key|. Note that |server| can be passed
+    // in via std::move(), since most callsites can pass a recently created
+    // SchemeHostPort.
+    ServerInfoMapKey(url::SchemeHostPort server,
                      const NetworkIsolationKey& network_isolation_key,
                      bool use_network_isolation_key);
     ~ServerInfoMapKey();
 
     bool operator<(const ServerInfoMapKey& other) const;
 
+    // IMPORTANT: The constructor normalizes the scheme so that "ws" is replaced
+    // by "http" and "wss" by "https", so this should never be compared directly
+    // with values passed into to HttpServerProperties methods.
     url::SchemeHostPort server;
+
     NetworkIsolationKey network_isolation_key;
   };
 
@@ -236,10 +242,6 @@ class NET_EXPORT HttpServerProperties
 
   ~HttpServerProperties() override;
 
-  // Returns the SchemeHostPort for |url|, with the exception that it replaces
-  // "wss" with "https" and "ws" with "http", so they're grouped together.
-  static url::SchemeHostPort GetNormalizedSchemeHostPort(const GURL& url);
-
   // Deletes all data. If |callback| is non-null, flushes data to disk
   // and invokes the callback asynchronously once changes have been written to
   // disk.
@@ -468,6 +470,48 @@ class NET_EXPORT HttpServerProperties
       QuicCanonicalMap;
   typedef std::vector<std::string> CanonicalSuffixList;
 
+  // Internal implementations of public methods. SchemeHostPort argument must be
+  // normalized before calling (ws/wss replaced with http/https). Use wrapped
+  // functions instead of putting the normalization in the public functions to
+  // reduce chance of regression - normalization in ServerInfoMapKey's
+  // constructor would leave |server.scheme| as wrong if not access through the
+  // key, and explicit normalization to create |normalized_server| means the one
+  // with the incorrect scheme would still be available.
+  bool GetSupportsSpdyInternal(
+      url::SchemeHostPort server,
+      const net::NetworkIsolationKey& network_isolation_key);
+  void SetSupportsSpdyInternal(
+      url::SchemeHostPort server,
+      const net::NetworkIsolationKey& network_isolation_key,
+      bool supports_spdy);
+  bool RequiresHTTP11Internal(
+      url::SchemeHostPort server,
+      const net::NetworkIsolationKey& network_isolation_key);
+  void SetHTTP11RequiredInternal(
+      url::SchemeHostPort server,
+      const net::NetworkIsolationKey& network_isolation_key);
+  void MaybeForceHTTP11Internal(
+      url::SchemeHostPort server,
+      const net::NetworkIsolationKey& network_isolation_key,
+      SSLConfig* ssl_config);
+  AlternativeServiceInfoVector GetAlternativeServiceInfosInternal(
+      const url::SchemeHostPort& origin,
+      const net::NetworkIsolationKey& network_isolation_key);
+  void SetAlternativeServicesInternal(
+      const url::SchemeHostPort& origin,
+      const net::NetworkIsolationKey& network_isolation_key,
+      const AlternativeServiceInfoVector& alternative_service_info_vector);
+  void SetServerNetworkStatsInternal(
+      url::SchemeHostPort server,
+      const NetworkIsolationKey& network_isolation_key,
+      ServerNetworkStats stats);
+  void ClearServerNetworkStatsInternal(
+      url::SchemeHostPort server,
+      const NetworkIsolationKey& network_isolation_key);
+  const ServerNetworkStats* GetServerNetworkStatsInternal(
+      url::SchemeHostPort server,
+      const NetworkIsolationKey& network_isolation_key);
+
   // Helper functions to use the passed in parameters and
   // |use_network_isolation_key_| to create a [Quic]ServerInfoMapKey.
   ServerInfoMapKey CreateServerInfoKey(

net/http/http_server_properties_manager.cc

@@ -441,10 +441,10 @@ void HttpServerPropertiesManager::AddServerData(
     ParseNetworkStats(spdy_server, server_dict, &server_info);
 
   if (!server_info.empty()) {
-    server_info_map->Put(
-        HttpServerProperties::ServerInfoMapKey(
-            spdy_server, network_isolation_key, use_network_isolation_key),
-        std::move(server_info));
+    server_info_map->Put(HttpServerProperties::ServerInfoMapKey(
+                             std::move(spdy_server), network_isolation_key,
+                             use_network_isolation_key),
+                         std::move(server_info));
   }
 }
 

net/http/http_server_properties_unittest.cc

@@ -135,53 +135,6 @@ class HttpServerPropertiesTest : public TestWithTaskEnvironment {
   HttpServerProperties impl_;
 };
 
-TEST_F(HttpServerPropertiesTest, GetNormalizedSchemeHostPort) {
-  url::SchemeHostPort server =
-      HttpServerProperties::GetNormalizedSchemeHostPort(
-          GURL("https://foo.test/"));
-  EXPECT_EQ("https", server.scheme());
-  EXPECT_EQ("foo.test", server.host());
-  EXPECT_EQ(443, server.port());
-
-  server = HttpServerProperties::GetNormalizedSchemeHostPort(
-      GURL("wss://foo.test/"));
-  EXPECT_EQ("https", server.scheme());
-  EXPECT_EQ("foo.test", server.host());
-  EXPECT_EQ(443, server.port());
-
-  server = HttpServerProperties::GetNormalizedSchemeHostPort(
-      GURL("wss://bar.test:7/"));
-  EXPECT_EQ("https", server.scheme());
-  EXPECT_EQ("bar.test", server.host());
-  EXPECT_EQ(7, server.port());
-
-  server = HttpServerProperties::GetNormalizedSchemeHostPort(
-      GURL("http://foo.test/"));
-  EXPECT_EQ("http", server.scheme());
-  EXPECT_EQ("foo.test", server.host());
-  EXPECT_EQ(80, server.port());
-
-  server =
-      HttpServerProperties::GetNormalizedSchemeHostPort(GURL("ws://foo.test/"));
-  EXPECT_EQ("http", server.scheme());
-  EXPECT_EQ("foo.test", server.host());
-  EXPECT_EQ(80, server.port());
-
-  server = HttpServerProperties::GetNormalizedSchemeHostPort(
-      GURL("ws://bar.test:7/"));
-  EXPECT_EQ("http", server.scheme());
-  EXPECT_EQ("bar.test", server.host());
-  EXPECT_EQ(7, server.port());
-
-  // Neither of these should happen, except possibly when loading a bad
-  // properties file, but neither should crash.
-  server = HttpServerProperties::GetNormalizedSchemeHostPort(GURL());
-  EXPECT_EQ("", server.host());
-  server = HttpServerProperties::GetNormalizedSchemeHostPort(
-      GURL("file:///foo/bar"));
-  EXPECT_EQ(server, url::SchemeHostPort(GURL("file:///foo/bar")));
-}
-
 TEST_F(HttpServerPropertiesTest, SetSupportsSpdy) {
   // Check spdy servers are correctly set with SchemeHostPort key.
   url::SchemeHostPort https_www_server("https", "www.google.com", 443);
@@ -230,6 +183,44 @@ TEST_F(HttpServerPropertiesTest, SetSupportsSpdy) {
       impl_.SupportsRequestPriority(https_mail_server, NetworkIsolationKey()));
 }
 
+TEST_F(HttpServerPropertiesTest, SetSupportsSpdyWebSockets) {
+  // The https and wss servers should be treated as the same server, as should
+  // the http and ws servers.
+  url::SchemeHostPort https_server("https", "www.test.com", 443);
+  url::SchemeHostPort wss_server("wss", "www.test.com", 443);
+  url::SchemeHostPort http_server("http", "www.test.com", 443);
+  url::SchemeHostPort ws_server("ws", "www.test.com", 443);
+
+  EXPECT_FALSE(impl_.GetSupportsSpdy(https_server, NetworkIsolationKey()));
+  EXPECT_FALSE(impl_.GetSupportsSpdy(wss_server, NetworkIsolationKey()));
+  EXPECT_FALSE(impl_.GetSupportsSpdy(http_server, NetworkIsolationKey()));
+  EXPECT_FALSE(impl_.GetSupportsSpdy(ws_server, NetworkIsolationKey()));
+
+  impl_.SetSupportsSpdy(wss_server, NetworkIsolationKey(), true);
+  EXPECT_TRUE(impl_.GetSupportsSpdy(https_server, NetworkIsolationKey()));
+  EXPECT_TRUE(impl_.GetSupportsSpdy(wss_server, NetworkIsolationKey()));
+  EXPECT_FALSE(impl_.GetSupportsSpdy(http_server, NetworkIsolationKey()));
+  EXPECT_FALSE(impl_.GetSupportsSpdy(ws_server, NetworkIsolationKey()));
+
+  impl_.SetSupportsSpdy(http_server, NetworkIsolationKey(), true);
+  EXPECT_TRUE(impl_.GetSupportsSpdy(https_server, NetworkIsolationKey()));
+  EXPECT_TRUE(impl_.GetSupportsSpdy(wss_server, NetworkIsolationKey()));
+  EXPECT_TRUE(impl_.GetSupportsSpdy(http_server, NetworkIsolationKey()));
+  EXPECT_TRUE(impl_.GetSupportsSpdy(ws_server, NetworkIsolationKey()));
+
+  impl_.SetSupportsSpdy(https_server, NetworkIsolationKey(), false);
+  EXPECT_FALSE(impl_.GetSupportsSpdy(https_server, NetworkIsolationKey()));
+  EXPECT_FALSE(impl_.GetSupportsSpdy(wss_server, NetworkIsolationKey()));
+  EXPECT_TRUE(impl_.GetSupportsSpdy(http_server, NetworkIsolationKey()));
+  EXPECT_TRUE(impl_.GetSupportsSpdy(ws_server, NetworkIsolationKey()));
+
+  impl_.SetSupportsSpdy(ws_server, NetworkIsolationKey(), false);
+  EXPECT_FALSE(impl_.GetSupportsSpdy(https_server, NetworkIsolationKey()));
+  EXPECT_FALSE(impl_.GetSupportsSpdy(wss_server, NetworkIsolationKey()));
+  EXPECT_FALSE(impl_.GetSupportsSpdy(http_server, NetworkIsolationKey()));
+  EXPECT_FALSE(impl_.GetSupportsSpdy(ws_server, NetworkIsolationKey()));
+}
+
 TEST_F(HttpServerPropertiesTest, SetSupportsSpdyWithNetworkIsolationKey) {
   const url::SchemeHostPort kServer("https", "foo.test", 443);
 
@@ -685,6 +676,88 @@ TEST_F(AlternateProtocolServerPropertiesTest, Set) {
   EXPECT_EQ(expiration4, (*service_info)[0].expiration());
 }
 
+TEST_F(AlternateProtocolServerPropertiesTest, SetWebSockets) {
+  // The https and wss servers should be treated as the same server, as should
+  // the http and ws servers.
+  url::SchemeHostPort https_server("https", "www.test.com", 443);
+  url::SchemeHostPort wss_server("wss", "www.test.com", 443);
+  url::SchemeHostPort http_server("http", "www.test.com", 443);
+  url::SchemeHostPort ws_server("ws", "www.test.com", 443);
+
+  AlternativeService alternative_service(kProtoHTTP2, "bar", 443);
+
+  EXPECT_EQ(
+      0u, impl_.GetAlternativeServiceInfos(https_server, NetworkIsolationKey())
+              .size());
+  EXPECT_EQ(0u,
+            impl_.GetAlternativeServiceInfos(wss_server, NetworkIsolationKey())
+                .size());
+  EXPECT_EQ(0u,
+            impl_.GetAlternativeServiceInfos(http_server, NetworkIsolationKey())
+                .size());
+  EXPECT_EQ(0u,
+            impl_.GetAlternativeServiceInfos(ws_server, NetworkIsolationKey())
+                .size());
+
+  SetAlternativeService(wss_server, alternative_service);
+  EXPECT_EQ(
+      1u, impl_.GetAlternativeServiceInfos(https_server, NetworkIsolationKey())
+              .size());
+  EXPECT_EQ(1u,
+            impl_.GetAlternativeServiceInfos(wss_server, NetworkIsolationKey())
+                .size());
+  EXPECT_EQ(0u,
+            impl_.GetAlternativeServiceInfos(http_server, NetworkIsolationKey())
+                .size());
+  EXPECT_EQ(0u,
+            impl_.GetAlternativeServiceInfos(ws_server, NetworkIsolationKey())
+                .size());
+
+  SetAlternativeService(http_server, alternative_service);
+  EXPECT_EQ(
+      1u, impl_.GetAlternativeServiceInfos(https_server, NetworkIsolationKey())
+              .size());
+  EXPECT_EQ(1u,
+            impl_.GetAlternativeServiceInfos(wss_server, NetworkIsolationKey())
+                .size());
+  EXPECT_EQ(1u,
+            impl_.GetAlternativeServiceInfos(http_server, NetworkIsolationKey())
+                .size());
+  EXPECT_EQ(1u,
+            impl_.GetAlternativeServiceInfos(ws_server, NetworkIsolationKey())
+                .size());
+
+  impl_.SetAlternativeServices(https_server, NetworkIsolationKey(),
+                               AlternativeServiceInfoVector());
+  EXPECT_EQ(
+      0u, impl_.GetAlternativeServiceInfos(https_server, NetworkIsolationKey())
+              .size());
+  EXPECT_EQ(0u,
+            impl_.GetAlternativeServiceInfos(wss_server, NetworkIsolationKey())
+                .size());
+  EXPECT_EQ(1u,
+            impl_.GetAlternativeServiceInfos(http_server, NetworkIsolationKey())
+                .size());
+  EXPECT_EQ(1u,
+            impl_.GetAlternativeServiceInfos(ws_server, NetworkIsolationKey())
+                .size());
+
+  impl_.SetAlternativeServices(ws_server, NetworkIsolationKey(),
+                               AlternativeServiceInfoVector());
+  EXPECT_EQ(
+      0u, impl_.GetAlternativeServiceInfos(https_server, NetworkIsolationKey())
+              .size());
+  EXPECT_EQ(0u,
+            impl_.GetAlternativeServiceInfos(wss_server, NetworkIsolationKey())
+                .size());
+  EXPECT_EQ(0u,
+            impl_.GetAlternativeServiceInfos(http_server, NetworkIsolationKey())
+                .size());
+  EXPECT_EQ(0u,
+            impl_.GetAlternativeServiceInfos(ws_server, NetworkIsolationKey())
+                .size());
+}
+
 TEST_F(AlternateProtocolServerPropertiesTest, SetWithNetworkIsolationKey) {
   const url::SchemeHostPort kServer("https", "foo.test", 443);
   const AlternativeServiceInfoVector kAlternativeServices(

net/http/http_stream_factory_job.cc

@@ -392,7 +392,7 @@ bool HttpStreamFactory::Job::CanUseExistingSpdySession() const {
 
   if (proxy_info_.is_direct() &&
       session_->http_server_properties()->RequiresHTTP11(
-          HttpServerProperties::GetNormalizedSchemeHostPort(request_info_.url),
+          url::SchemeHostPort(request_info_.url),
           request_info_.network_isolation_key)) {
     return false;
   }
@@ -838,7 +838,7 @@ int HttpStreamFactory::Job::DoInitConnectionImpl() {
       session_->http_server_properties();
   if (http_server_properties) {
     http_server_properties->MaybeForceHTTP11(
-        HttpServerProperties::GetNormalizedSchemeHostPort(request_info_.url),
+        url::SchemeHostPort(request_info_.url),
         request_info_.network_isolation_key, &server_ssl_config_);
     if (proxy_info_.is_https()) {
       http_server_properties->MaybeForceHTTP11(