Switch the NetworkContext PreconnectSockets interface to a credentials boolean.

Specifying privacy_mode and load_flags separately risks the callers
mixing the two up, which can corrupt the socket pool state. Indeed
network_context_unittest.cc itself mixes them up.

Along the way, fix the documentation which claims privacy_mode makes the
connection untrackable, which is false. The only thing it does is pools
it with different requests.

This CL should be a no-op behavior-wise.

Bug: 799935
Change-Id: Iaf770cd556d7dbfaa296abfac22c070ecc903c50
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1752628Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: Ken Buchanan <kenrb@chromium.org>
Reviewed-by: Egor Pasko <pasko@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Commit-Queue: David Benjamin <davidben@chromium.org>
Cr-Commit-Position: refs/heads/master@{#688237}

chrome/browser/chromeos/login/auth/auth_prewarmer.cc

@@ -16,7 +16,6 @@
 #include "content/public/browser/browser_task_traits.h"
 #include "content/public/browser/browser_thread.h"
 #include "google_apis/gaia/gaia_urls.h"
-#include "net/base/load_flags.h"
 #include "net/base/network_isolation_key.h"
 #include "services/network/public/mojom/network_context.mojom.h"
 #include "url/gurl.h"
@@ -59,16 +58,14 @@ void AuthPrewarmer::DefaultNetworkChanged(const NetworkState* network) {
 
 void AuthPrewarmer::DoPrewarm() {
   const int kConnectionsNeeded = 1;
-  const int kLoadFlags = net::LOAD_NORMAL;
-  const bool kShouldUsePrivacyMode = false;
+  const bool kAllowCredentials = true;
   const GURL& url = GaiaUrls::GetInstance()->service_login_url();
   network::mojom::NetworkContext* network_context =
       login::GetSigninNetworkContext();
   if (network_context) {
     // Do nothing if NetworkContext isn't available.
-    network_context->PreconnectSockets(kConnectionsNeeded, url, kLoadFlags,
-                                       kShouldUsePrivacyMode,
-                                       net::NetworkIsolationKey());
+    network_context->PreconnectSockets(
+        kConnectionsNeeded, url, kAllowCredentials, net::NetworkIsolationKey());
   }
   if (!completion_callback_.is_null()) {
     base::PostTask(FROM_HERE, {content::BrowserThread::UI},

chrome/browser/predictors/preconnect_manager.cc

@@ -14,7 +14,6 @@
 #include "content/public/browser/browser_task_traits.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/storage_partition.h"
-#include "net/base/load_flags.h"
 #include "services/network/public/mojom/network_context.mojom.h"
 
 namespace predictors {
@@ -169,16 +168,7 @@ void PreconnectManager::PreconnectUrl(
   if (!network_context)
     return;
 
-  bool privacy_mode = false;
-  int load_flags = net::LOAD_NORMAL;
-
-  if (!allow_credentials) {
-    privacy_mode = true;
-    load_flags = net::LOAD_DO_NOT_SEND_COOKIES | net::LOAD_DO_NOT_SAVE_COOKIES |
-                 net::LOAD_DO_NOT_SEND_AUTH_DATA;
-  }
-
-  network_context->PreconnectSockets(num_sockets, url, load_flags, privacy_mode,
+  network_context->PreconnectSockets(num_sockets, url, allow_credentials,
                                      network_isolation_key);
 }
 

services/network/network_context.cc

@@ -1536,8 +1536,7 @@ void NetworkContext::VerifyCertificateForTesting(
 void NetworkContext::PreconnectSockets(
     uint32_t num_streams,
     const GURL& original_url,
-    int32_t load_flags,
-    bool privacy_mode_enabled,
+    bool allow_credentials,
     const net::NetworkIsolationKey& network_isolation_key) {
   GURL url = GetHSTSRedirect(original_url);
 
@@ -1557,9 +1556,15 @@ void NetworkContext::PreconnectSockets(
   request_info.extra_headers.SetHeader(net::HttpRequestHeaders::kUserAgent,
                                        user_agent);
 
-  request_info.load_flags = load_flags;
-  request_info.privacy_mode = privacy_mode_enabled ? net::PRIVACY_MODE_ENABLED
-                                                   : net::PRIVACY_MODE_DISABLED;
+  if (allow_credentials) {
+    request_info.load_flags = net::LOAD_NORMAL;
+    request_info.privacy_mode = net::PRIVACY_MODE_DISABLED;
+  } else {
+    request_info.load_flags = net::LOAD_DO_NOT_SEND_COOKIES |
+                              net::LOAD_DO_NOT_SAVE_COOKIES |
+                              net::LOAD_DO_NOT_SEND_AUTH_DATA;
+    request_info.privacy_mode = net::PRIVACY_MODE_ENABLED;
+  }
   request_info.network_isolation_key = network_isolation_key;
 
   net::HttpTransactionFactory* factory =

services/network/network_context.h

@@ -316,8 +316,7 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) NetworkContext
   void PreconnectSockets(
       uint32_t num_streams,
       const GURL& url,
-      int32_t load_flags,
-      bool privacy_mode_enabled,
+      bool allow_credentials,
       const net::NetworkIsolationKey& network_isolation_key) override;
   void CreateP2PSocketManager(
       mojom::P2PTrustedSocketManagerClientPtr client,

services/network/network_context_unittest.cc

@@ -3482,9 +3482,9 @@ TEST_F(NetworkContextTest, PreconnectOne) {
   test_server.SetConnectionListener(&connection_listener);
   ASSERT_TRUE(test_server.Start());
 
-  network_context->PreconnectSockets(
-      1, test_server.base_url(), net::LOAD_NORMAL,
-      true /* privacy_mode_enabled */, net::NetworkIsolationKey());
+  network_context->PreconnectSockets(1, test_server.base_url(),
+                                     /*allow_credentials=*/true,
+                                     net::NetworkIsolationKey());
   connection_listener.WaitForAcceptedConnections(1u);
 }
 
@@ -3498,8 +3498,8 @@ TEST_F(NetworkContextTest, PreconnectHSTS) {
   ASSERT_TRUE(test_server.Start());
 
   const GURL server_http_url = GetHttpUrlFromHttps(test_server.base_url());
-  network_context->PreconnectSockets(1, server_http_url, net::LOAD_NORMAL,
-                                     true /* privacy_mode_enabled */,
+  network_context->PreconnectSockets(1, server_http_url,
+                                     /*allow_credentials=*/false,
                                      net::NetworkIsolationKey());
   connection_listener.WaitForAcceptedConnections(1u);
 
@@ -3512,8 +3512,8 @@ TEST_F(NetworkContextTest, PreconnectHSTS) {
       base::Time::Now() + base::TimeDelta::FromSeconds(1000);
   network_context->url_request_context()->transport_security_state()->AddHSTS(
       server_http_url.host(), expiry, false);
-  network_context->PreconnectSockets(1, server_http_url, net::LOAD_NORMAL,
-                                     true /* privacy_mode_enabled */,
+  network_context->PreconnectSockets(1, server_http_url,
+                                     /*allow_credentials=*/false,
                                      net::NetworkIsolationKey());
   connection_listener.WaitForAcceptedConnections(1u);
 
@@ -3533,9 +3533,9 @@ TEST_F(NetworkContextTest, PreconnectZero) {
   test_server.SetConnectionListener(&connection_listener);
   ASSERT_TRUE(test_server.Start());
 
-  network_context->PreconnectSockets(
-      0, test_server.base_url(), net::LOAD_NORMAL,
-      true /* privacy_mode_enabled */, net::NetworkIsolationKey());
+  network_context->PreconnectSockets(0, test_server.base_url(),
+                                     /*allow_credentials=*/true,
+                                     net::NetworkIsolationKey());
   base::RunLoop().RunUntilIdle();
 
   int num_sockets =
@@ -3555,9 +3555,9 @@ TEST_F(NetworkContextTest, PreconnectTwo) {
   test_server.SetConnectionListener(&connection_listener);
   ASSERT_TRUE(test_server.Start());
 
-  network_context->PreconnectSockets(
-      2, test_server.base_url(), net::LOAD_NORMAL,
-      true /* privacy_mode_enabled */, net::NetworkIsolationKey());
+  network_context->PreconnectSockets(2, test_server.base_url(),
+                                     /*allow_credentials=*/true,
+                                     net::NetworkIsolationKey());
   connection_listener.WaitForAcceptedConnections(2u);
 
   int num_sockets =
@@ -3574,9 +3574,9 @@ TEST_F(NetworkContextTest, PreconnectFour) {
   test_server.SetConnectionListener(&connection_listener);
   ASSERT_TRUE(test_server.Start());
 
-  network_context->PreconnectSockets(
-      4, test_server.base_url(), net::LOAD_NORMAL,
-      true /* privacy_mode_enabled */, net::NetworkIsolationKey());
+  network_context->PreconnectSockets(4, test_server.base_url(),
+                                     /*allow_credentials=*/true,
+                                     net::NetworkIsolationKey());
 
   connection_listener.WaitForAcceptedConnections(4u);
 
@@ -3598,9 +3598,9 @@ TEST_F(NetworkContextTest, PreconnectMax) {
       GetSocketPoolInfo(network_context.get(), "max_sockets_per_group");
   EXPECT_GT(76, max_num_sockets);
 
-  network_context->PreconnectSockets(
-      76, test_server.base_url(), net::LOAD_NORMAL,
-      true /* privacy_mode_enabled */, net::NetworkIsolationKey());
+  network_context->PreconnectSockets(76, test_server.base_url(),
+                                     /*allow_credentials=*/true,
+                                     net::NetworkIsolationKey());
 
   // Wait until |max_num_sockets| have been connected.
   connection_listener.WaitForAcceptedConnections(max_num_sockets);
@@ -3635,11 +3635,9 @@ TEST_F(NetworkContextTest, PreconnectNetworkIsolationKey) {
   const net::NetworkIsolationKey kKey1(kOriginFoo, kOriginFoo);
   const net::NetworkIsolationKey kKey2(kOriginBar, kOriginBar);
   network_context->PreconnectSockets(1, test_server.base_url(),
-                                     net::LOAD_NORMAL,
-                                     true /* privacy_mode_enabled */, kKey1);
+                                     /*allow_credentials=*/false, kKey1);
   network_context->PreconnectSockets(2, test_server.base_url(),
-                                     net::LOAD_NORMAL,
-                                     true /* privacy_mode_enabled */, kKey2);
+                                     /*allow_credentials=*/false, kKey2);
   connection_listener.WaitForAcceptedConnections(3u);
 
   net::ClientSocketPool::GroupId group_id1(

services/network/public/mojom/network_context.mojom

@@ -903,18 +903,14 @@ interface NetworkContext {
 
   // Tries to preconnect to |url|. |num_streams| may be used to request more
   // than one connection be established in parallel.
-  // |load_flags| is passed into the HttpRequestInfo used to make the request.
-  // See |load_flags.h| for possible values.
-  // |privacy_mode_enabled| is also passed into the HttpRequestInfo class: if
-  // it is true, then the request must be sent over a connection that cannot be
-  // tracked by the server.
+  // |allow_credentials| is true if the connection is to be pooled with
+  // credentialed requests and false otherwise.
   // |network_isolation_key| specifies the NetworkIsolationKey to associate
   // with the preconnected sockets. The sockets will only be used for requests
   // associated with the same key.
   PreconnectSockets(uint32 num_streams,
                     url.mojom.Url url,
-                    int32 load_flags,
-                    bool privacy_mode_enabled,
+                    bool allow_credentials,
                     NetworkIsolationKey network_isolation_key);
 
   // Creates a P2PSocketManager instance, used for WebRTC.

services/network/test/test_network_context.h

@@ -196,8 +196,7 @@ class TestNetworkContext : public mojom::NetworkContext {
   void PreconnectSockets(
       uint32_t num_streams,
       const GURL& url,
-      int32_t load_flags,
-      bool privacy_mode_enabled,
+      bool allow_credentials,
       const net::NetworkIsolationKey& network_isolation_key) override {}
   void CreateP2PSocketManager(
       mojom::P2PTrustedSocketManagerClientPtr client,