Add NetworkIsolationKey support to PreconnectManager.

Currently, nothing passes in anything other than a default constructed
one. Consumers will be updated one-at-a-time in followup CLs, and the
default empty NetworkIsolationKey will be removed from method
declarations.

Bug: 966896
Change-Id: If5cc69299de2b0fc6886c304710ffa40423f9536
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1672147
Commit-Queue: Matt Menke <mmenke@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Alex Ilin <alexilin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#680037}

chrome/browser/predictors/loading_predictor_unittest.cc

@@ -49,8 +49,10 @@ class MockPreconnectManager : public PreconnectManager {
   MOCK_METHOD1(StartPreresolveHost, void(const GURL& url));
   MOCK_METHOD1(StartPreresolveHosts,
                void(const std::vector<std::string>& hostnames));
-  MOCK_METHOD2(StartPreconnectUrl,
-               void(const GURL& url, bool allow_credentials));
+  MOCK_METHOD3(StartPreconnectUrl,
+               void(const GURL& url,
+                    bool allow_credentials,
+                    net::NetworkIsolationKey network_isolation_key));
   MOCK_METHOD1(Stop, void(const GURL& url));
 
   void Start(const GURL& url,
@@ -303,7 +305,8 @@ TEST_F(LoadingPredictorTest, TestDontPredictOmniboxHints) {
 TEST_F(LoadingPredictorPreconnectTest, TestHandleOmniboxHint) {
   const GURL preconnect_suggestion = GURL("http://search.com/kittens");
   EXPECT_CALL(*mock_preconnect_manager_,
-              StartPreconnectUrl(preconnect_suggestion, true));
+              StartPreconnectUrl(preconnect_suggestion, true,
+                                 net::NetworkIsolationKey()));
   predictor_->PrepareForPageLoad(preconnect_suggestion, HintOrigin::OMNIBOX,
                                  true);
   // The second suggestion for the same host should be filtered out.

chrome/browser/predictors/preconnect_manager.cc

@@ -46,10 +46,23 @@ PreresolveInfo::~PreresolveInfo() = default;
 PreresolveJob::PreresolveJob(const GURL& url,
                              int num_sockets,
                              bool allow_credentials,
+                             net::NetworkIsolationKey network_isolation_key,
                              PreresolveInfo* info)
     : url(url),
       num_sockets(num_sockets),
       allow_credentials(allow_credentials),
+      network_isolation_key(std::move(network_isolation_key)),
+      info(info) {
+  DCHECK_GE(num_sockets, 0);
+}
+
+PreresolveJob::PreresolveJob(PreconnectRequest preconnect_request,
+                             PreresolveInfo* info)
+    : url(std::move(preconnect_request.origin)),
+      num_sockets(preconnect_request.num_sockets),
+      allow_credentials(preconnect_request.allow_credentials),
+      network_isolation_key(
+          std::move(preconnect_request.network_isolation_key)),
       info(info) {
   DCHECK_GE(num_sockets, 0);
 }
@@ -79,11 +92,11 @@ void PreconnectManager::Start(const GURL& url,
       host, std::make_unique<PreresolveInfo>(url, requests.size()));
   PreresolveInfo* info = iterator_and_whether_inserted.first->second.get();
 
-  for (const auto& request : requests) {
-    DCHECK(request.origin.GetOrigin() == request.origin);
+  for (auto request_it = requests.begin(); request_it != requests.end();
+       ++request_it) {
+    DCHECK(request_it->origin.GetOrigin() == request_it->origin);
     PreresolveJobId job_id = preresolve_jobs_.Add(
-        std::make_unique<PreresolveJob>(request.origin, request.num_sockets,
-                                        request.allow_credentials, info));
+        std::make_unique<PreresolveJob>(std::move(*request_it), info));
     queued_jobs_.push_back(job_id);
   }
 
@@ -95,7 +108,8 @@ void PreconnectManager::StartPreresolveHost(const GURL& url) {
   if (!url.SchemeIsHTTPOrHTTPS())
     return;
   PreresolveJobId job_id = preresolve_jobs_.Add(std::make_unique<PreresolveJob>(
-      url.GetOrigin(), 0, kAllowCredentialsOnPreconnectByDefault, nullptr));
+      url.GetOrigin(), 0, kAllowCredentialsOnPreconnectByDefault,
+      net::NetworkIsolationKey(), nullptr));
   queued_jobs_.push_front(job_id);
 
   TryToLaunchPreresolveJobs();
@@ -109,20 +123,23 @@ void PreconnectManager::StartPreresolveHosts(
     PreresolveJobId job_id =
         preresolve_jobs_.Add(std::make_unique<PreresolveJob>(
             GURL("http://" + *it), 0, kAllowCredentialsOnPreconnectByDefault,
-            nullptr));
+            net::NetworkIsolationKey(), nullptr));
     queued_jobs_.push_front(job_id);
   }
 
   TryToLaunchPreresolveJobs();
 }
 
-void PreconnectManager::StartPreconnectUrl(const GURL& url,
-                                           bool allow_credentials) {
+void PreconnectManager::StartPreconnectUrl(
+    const GURL& url,
+    bool allow_credentials,
+    net::NetworkIsolationKey network_isolation_key) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   if (!url.SchemeIsHTTPOrHTTPS())
     return;
   PreresolveJobId job_id = preresolve_jobs_.Add(std::make_unique<PreresolveJob>(
-      url.GetOrigin(), 1, allow_credentials, nullptr));
+      url.GetOrigin(), 1, allow_credentials, std::move(network_isolation_key),
+      nullptr));
   queued_jobs_.push_front(job_id);
 
   TryToLaunchPreresolveJobs();
@@ -138,9 +155,11 @@ void PreconnectManager::Stop(const GURL& url) {
   it->second->was_canceled = true;
 }
 
-void PreconnectManager::PreconnectUrl(const GURL& url,
-                                      int num_sockets,
-                                      bool allow_credentials) const {
+void PreconnectManager::PreconnectUrl(
+    const GURL& url,
+    int num_sockets,
+    bool allow_credentials,
+    const net::NetworkIsolationKey& network_isolation_key) const {
   DCHECK(url.GetOrigin() == url);
   DCHECK(url.SchemeIsHTTPOrHTTPS());
   if (observer_)
@@ -159,9 +178,8 @@ void PreconnectManager::PreconnectUrl(const GURL& url,
                  net::LOAD_DO_NOT_SEND_AUTH_DATA;
   }
 
-  // TODO(mmenke): Use an appropriate NetworkIsolationKey().
   network_context->PreconnectSockets(num_sockets, url, load_flags, privacy_mode,
-                                     net::NetworkIsolationKey());
+                                     network_isolation_key);
 }
 
 std::unique_ptr<ResolveHostClientImpl> PreconnectManager::PreresolveUrl(
@@ -275,8 +293,10 @@ void PreconnectManager::FinishPreresolveJob(PreresolveJobId job_id,
   DCHECK(job);
 
   bool need_preconnect = success && job->need_preconnect();
-  if (need_preconnect)
-    PreconnectUrl(job->url, job->num_sockets, job->allow_credentials);
+  if (need_preconnect) {
+    PreconnectUrl(job->url, job->num_sockets, job->allow_credentials,
+                  job->network_isolation_key);
+  }
 
   PreresolveInfo* info = job->info;
   if (info)

chrome/browser/predictors/preconnect_manager.h

@@ -17,6 +17,7 @@
 #include "chrome/browser/predictors/proxy_lookup_client_impl.h"
 #include "chrome/browser/predictors/resolve_host_client_impl.h"
 #include "chrome/browser/predictors/resource_prefetch_predictor.h"
+#include "net/base/network_isolation_key.h"
 #include "url/gurl.h"
 
 class Profile;
@@ -75,7 +76,9 @@ struct PreresolveJob {
   PreresolveJob(const GURL& url,
                 int num_sockets,
                 bool allow_credentials,
+                net::NetworkIsolationKey network_isolation_key,
                 PreresolveInfo* info);
+  PreresolveJob(PreconnectRequest preconnect_request, PreresolveInfo* info);
   PreresolveJob(PreresolveJob&& other);
   ~PreresolveJob();
   bool need_preconnect() const {
@@ -85,8 +88,9 @@ struct PreresolveJob {
   GURL url;
   int num_sockets;
   bool allow_credentials;
+  net::NetworkIsolationKey network_isolation_key;
   // Raw pointer usage is fine here because even though PreresolveJob can
-  // outlive PreresolveInfo it's only accessed on PreconnectManager class
+  // outlive PreresolveInfo. It's only accessed on PreconnectManager class
   // context and PreresolveInfo lifetime is tied to PreconnectManager.
   // May be equal to nullptr in case of detached job.
   PreresolveInfo* info;
@@ -145,7 +149,19 @@ class PreconnectManager {
   // than trackable requests thus they are put in the front of the jobs queue.
   virtual void StartPreresolveHost(const GURL& url);
   virtual void StartPreresolveHosts(const std::vector<std::string>& hostnames);
-  virtual void StartPreconnectUrl(const GURL& url, bool allow_credentials);
+  // |network_isolation_key| specifies the key that network requests for the
+  // preconnected URL are expected to use. If a request is issued with a
+  // different key, it may not use the preconnected socket.
+  //
+  // TODO(https://crbug.com/966896): Update consumers and make
+  // |network_isolation_key| a mandatory argument. Note that this is a temporary
+  // style guide violation, but keeping this until all consumers correctly fill
+  // the argument reduces the chances of forgetting to update one.
+  virtual void StartPreconnectUrl(
+      const GURL& url,
+      bool allow_credentials,
+      net::NetworkIsolationKey network_isolation_key =
+          net::NetworkIsolationKey());
 
   // No additional jobs keyed by the |url| will be queued after this.
   virtual void Stop(const GURL& url);
@@ -166,9 +182,11 @@ class PreconnectManager {
   using PreresolveJobId = PreresolveJobMap::KeyType;
   friend class PreconnectManagerTest;
 
-  void PreconnectUrl(const GURL& url,
-                     int num_sockets,
-                     bool allow_credentials) const;
+  void PreconnectUrl(
+      const GURL& url,
+      int num_sockets,
+      bool allow_credentials,
+      const net::NetworkIsolationKey& network_isolation_key) const;
   std::unique_ptr<ResolveHostClientImpl> PreresolveUrl(
       const GURL& url,
       ResolveHostCallback callback) const;

chrome/browser/predictors/preconnect_manager_unittest.cc

@@ -219,6 +219,27 @@ TEST_F(PreconnectManagerTest, TestStartOneUrlPreconnect) {
   mock_network_context_->CompleteHostLookup(url_to_preconnect.host(), net::OK);
 }
 
+TEST_F(PreconnectManagerTest,
+       TestStartOneUrlPreconnectWithNetworkIsolationKey) {
+  GURL main_frame_url("http://google.com");
+  GURL url_to_preconnect("http://cdn.google.com");
+  url::Origin requesting_origin = url::Origin::Create(GURL("http://foo.test"));
+  net::NetworkIsolationKey network_isolation_key(requesting_origin,
+                                                 requesting_origin);
+
+  EXPECT_CALL(*mock_network_context_,
+              ResolveHostProxy(url_to_preconnect.host()));
+  preconnect_manager_->Start(
+      main_frame_url,
+      {PreconnectRequest(url_to_preconnect, 1, network_isolation_key)});
+  EXPECT_CALL(*mock_network_context_,
+              PreconnectSockets(1, url_to_preconnect, kNormalLoadFlags,
+                                false /* privacy_mode_enabled */,
+                                network_isolation_key));
+  EXPECT_CALL(*mock_delegate_, PreconnectFinishedProxy(main_frame_url));
+  mock_network_context_->CompleteHostLookup(url_to_preconnect.host(), net::OK);
+}
+
 // Sends preconnect request for a webpage, and stops the request before
 // all pertaining preconnect requests finish. Next, preconnect request
 // for the same webpage is sent again. Verifies that all the preconnects
@@ -599,7 +620,8 @@ TEST_F(PreconnectManagerTest, TestStartPreconnectUrl) {
   bool allow_credentials = false;
 
   EXPECT_CALL(*mock_network_context_, ResolveHostProxy(origin.host()));
-  preconnect_manager_->StartPreconnectUrl(url, allow_credentials);
+  preconnect_manager_->StartPreconnectUrl(url, allow_credentials,
+                                          net::NetworkIsolationKey());
 
   EXPECT_CALL(
       *mock_network_context_,
@@ -609,7 +631,26 @@ TEST_F(PreconnectManagerTest, TestStartPreconnectUrl) {
 
   // Non http url shouldn't be preconnected.
   GURL non_http_url("file:///tmp/index.html");
-  preconnect_manager_->StartPreconnectUrl(non_http_url, allow_credentials);
+  preconnect_manager_->StartPreconnectUrl(non_http_url, allow_credentials,
+                                          net::NetworkIsolationKey());
+}
+
+TEST_F(PreconnectManagerTest, TestStartPreconnectUrlWithNetworkIsolationKey) {
+  GURL url("http://cdn.google.com/script.js");
+  GURL origin("http://cdn.google.com");
+  bool allow_credentials = false;
+  url::Origin requesting_origin = url::Origin::Create(GURL("http://foo.test"));
+  net::NetworkIsolationKey network_isolation_key(requesting_origin,
+                                                 requesting_origin);
+
+  EXPECT_CALL(*mock_network_context_, ResolveHostProxy(origin.host()));
+  preconnect_manager_->StartPreconnectUrl(url, allow_credentials,
+                                          network_isolation_key);
+
+  EXPECT_CALL(*mock_network_context_,
+              PreconnectSockets(1, origin, kPrivateLoadFlags,
+                                !allow_credentials, network_isolation_key));
+  mock_network_context_->CompleteHostLookup(origin.host(), net::OK);
 }
 
 TEST_F(PreconnectManagerTest, TestDetachedRequestHasHigherPriority) {

chrome/browser/predictors/resource_prefetch_predictor.cc

@@ -59,8 +59,13 @@ void InitializeOnDBSequence(
 
 }  // namespace
 
-PreconnectRequest::PreconnectRequest(const GURL& origin, int num_sockets)
-    : origin(origin), num_sockets(num_sockets) {
+PreconnectRequest::PreconnectRequest(
+    const GURL& origin,
+    int num_sockets,
+    net::NetworkIsolationKey network_isolation_key)
+    : origin(origin),
+      num_sockets(num_sockets),
+      network_isolation_key(std::move(network_isolation_key)) {
   DCHECK_GE(num_sockets, 0);
 }
 

chrome/browser/predictors/resource_prefetch_predictor.h

@@ -16,6 +16,7 @@
 #include "base/gtest_prod_util.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
+#include "base/optional.h"
 #include "base/scoped_observer.h"
 #include "base/task/cancelable_task_tracker.h"
 #include "base/time/time.h"
@@ -28,6 +29,7 @@
 #include "components/history/core/browser/history_types.h"
 #include "components/keyed_service/core/keyed_service.h"
 #include "content/public/common/resource_type.h"
+#include "net/base/network_isolation_key.h"
 #include "url/gurl.h"
 
 class PredictorsHandler;
@@ -54,12 +56,23 @@ class ResourcePrefetcherManager;
 // Stores all values needed to trigger a preconnect/preresolve job to a single
 // origin.
 struct PreconnectRequest {
-  PreconnectRequest(const GURL& origin, int num_sockets);
+  // |network_isolation_key| specifies the key that network requests for the
+  // preconnected URL are expected to use. If a request is issued with a
+  // different key, it may not use the preconnected socket. It has no effect
+  // when |num_sockets| == 0.
+  //
+  // TODO(https://crbug.com/966896): Update consumers and make
+  // |network_isolation_key| a mandatory argument.
+  PreconnectRequest(const GURL& origin,
+                    int num_sockets,
+                    net::NetworkIsolationKey network_isolation_key =
+                        net::NetworkIsolationKey());
 
   GURL origin;
   // A zero-value means that we need to preresolve a host only.
   int num_sockets = 0;
   bool allow_credentials = true;
+  net::NetworkIsolationKey network_isolation_key;
 };
 
 // Stores a result of preconnect prediction. The |requests| vector is the main

chrome/browser/renderer_host/chrome_render_message_filter.cc

@@ -32,6 +32,7 @@
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/render_process_host.h"
 #include "extensions/buildflags/buildflags.h"
+#include "net/base/network_isolation_key.h"
 #include "ppapi/buildflags/buildflags.h"
 
 #if BUILDFLAG(ENABLE_EXTENSIONS)
@@ -130,12 +131,17 @@ void ChromeRenderMessageFilter::OnPreconnect(const GURL& url,
     return;
   }
 
-  if (preconnect_manager_initialized_) {
-    base::PostTaskWithTraits(
-        FROM_HERE, {BrowserThread::UI},
-        base::BindOnce(&predictors::PreconnectManager::StartPreconnectUrl,
-                       preconnect_manager_, url, allow_credentials));
-  }
+  if (!preconnect_manager_initialized_)
+    return;
+
+  // TODO(mmenke):  Use process and frame ids to populate NetworkIsolationKey.
+  // May also need to think about enabling cross-site preconnects, though that
+  // will result in at least some cross-site information leakage.
+  base::PostTaskWithTraits(
+      FROM_HERE, {BrowserThread::UI},
+      base::BindOnce(&predictors::PreconnectManager::StartPreconnectUrl,
+                     preconnect_manager_, url, allow_credentials,
+                     net::NetworkIsolationKey()));
 }
 
 void ChromeRenderMessageFilter::OnAllowDatabase(