Add basic support for NetworkIsolationKeys to LoadingPredictor.

This CL makes LoadingPredictor always use the NetworkIsolationKey for
the initial main frame URL when preconnecting. In a future CL, I'll
make it use a NetworkIsolationKey of the expected destination URL,
after redirects, instead.

BUG=987735

Change-Id: I221a6bd4175005758ebf86b55cc21bdd98b167eb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1716620
Commit-Queue: Matt Menke <mmenke@chromium.org>
Reviewed-by: Alex Ilin <alexilin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#682722}

chrome/browser/predictors/loading_predictor.cc

@@ -13,6 +13,8 @@
 #include "chrome/browser/predictors/navigation_id.h"
 #include "chrome/browser/predictors/resource_prefetch_predictor.h"
 #include "content/public/browser/browser_thread.h"
+#include "net/base/network_isolation_key.h"
+#include "url/origin.h"
 
 namespace predictors {
 
@@ -39,8 +41,10 @@ bool AddInitialUrlToPreconnectPrediction(const GURL& initial_url,
         std::max(prediction->requests.front().num_sockets, kMinSockets);
   } else if (initial_origin.is_valid() &&
              initial_origin.SchemeIsHTTPOrHTTPS()) {
+    url::Origin origin = url::Origin::Create(initial_origin);
     prediction->requests.emplace(prediction->requests.begin(), initial_origin,
-                                 kMinSockets);
+                                 kMinSockets,
+                                 net::NetworkIsolationKey(origin, origin));
   }
 
   return !prediction->requests.empty();

chrome/browser/predictors/loading_predictor_unittest.cc

@@ -18,9 +18,11 @@
 #include "components/prefs/pref_service.h"
 #include "content/public/test/test_browser_thread_bundle.h"
 #include "content/public/test/test_utils.h"
+#include "net/base/network_isolation_key.h"
 #include "net/url_request/url_request_context.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
+#include "url/origin.h"
 
 using testing::_;
 using testing::Return;
@@ -71,6 +73,12 @@ LoadingPredictorConfig CreateConfig() {
   return config;
 }
 
+// Creates a NetworkIsolationKey for a main frame navigation to URL.
+net::NetworkIsolationKey CreateNetworkIsolationKey(const GURL& main_frame_url) {
+  url::Origin origin = url::Origin::Create(main_frame_url);
+  return net::NetworkIsolationKey(origin, origin);
+}
+
 }  // namespace
 
 class LoadingPredictorTest : public testing::Test {
@@ -332,10 +340,11 @@ TEST_F(LoadingPredictorPreconnectTest, TestAddInitialUrlToEmptyPrediction) {
   GURL main_frame_url("http://search.com/kittens");
   EXPECT_CALL(*mock_predictor_, PredictPreconnectOrigins(main_frame_url, _))
       .WillOnce(Return(false));
-  EXPECT_CALL(
-      *mock_preconnect_manager_,
-      StartProxy(main_frame_url, std::vector<PreconnectRequest>(
-                                     {{GURL("http://search.com"), 2}})));
+  EXPECT_CALL(*mock_preconnect_manager_,
+              StartProxy(main_frame_url,
+                         std::vector<PreconnectRequest>(
+                             {{GURL("http://search.com"), 2,
+                               CreateNetworkIsolationKey(main_frame_url)}})));
   predictor_->PrepareForPageLoad(main_frame_url, HintOrigin::NAVIGATION);
 }
 
@@ -343,19 +352,23 @@ TEST_F(LoadingPredictorPreconnectTest, TestAddInitialUrlToEmptyPrediction) {
 // if the list already containts the origin.
 TEST_F(LoadingPredictorPreconnectTest, TestAddInitialUrlMatchesPrediction) {
   GURL main_frame_url("http://search.com/kittens");
-  PreconnectPrediction prediction =
-      CreatePreconnectPrediction("search.com", true,
-                                 {{GURL("http://search.com"), 1},
-                                  {GURL("http://cdn.search.com"), 1},
-                                  {GURL("http://ads.search.com"), 0}});
+  net::NetworkIsolationKey network_isolation_key =
+      CreateNetworkIsolationKey(main_frame_url);
+  PreconnectPrediction prediction = CreatePreconnectPrediction(
+      "search.com", true,
+      {{GURL("http://search.com"), 1, network_isolation_key},
+       {GURL("http://cdn.search.com"), 1, network_isolation_key},
+       {GURL("http://ads.search.com"), 0, network_isolation_key}});
   EXPECT_CALL(*mock_predictor_, PredictPreconnectOrigins(main_frame_url, _))
       .WillOnce(DoAll(SetArgPointee<1>(prediction), Return(true)));
   EXPECT_CALL(
       *mock_preconnect_manager_,
-      StartProxy(main_frame_url, std::vector<PreconnectRequest>(
-                                     {{GURL("http://search.com"), 2},
-                                      {GURL("http://cdn.search.com"), 1},
-                                      {GURL("http://ads.search.com"), 0}})));
+      StartProxy(
+          main_frame_url,
+          std::vector<PreconnectRequest>(
+              {{GURL("http://search.com"), 2, network_isolation_key},
+               {GURL("http://cdn.search.com"), 1, network_isolation_key},
+               {GURL("http://ads.search.com"), 0, network_isolation_key}})));
   predictor_->PrepareForPageLoad(main_frame_url, HintOrigin::EXTERNAL);
 }
 
@@ -364,20 +377,24 @@ TEST_F(LoadingPredictorPreconnectTest, TestAddInitialUrlMatchesPrediction) {
 // url redirects to another host.
 TEST_F(LoadingPredictorPreconnectTest, TestAddInitialUrlDoesntMatchPrediction) {
   GURL main_frame_url("http://search.com/kittens");
-  PreconnectPrediction prediction =
-      CreatePreconnectPrediction("search.com", true,
-                                 {{GURL("http://en.search.com"), 1},
-                                  {GURL("http://cdn.search.com"), 1},
-                                  {GURL("http://ads.search.com"), 0}});
+  net::NetworkIsolationKey network_isolation_key =
+      CreateNetworkIsolationKey(main_frame_url);
+  PreconnectPrediction prediction = CreatePreconnectPrediction(
+      "search.com", true,
+      {{GURL("http://en.search.com"), 1, network_isolation_key},
+       {GURL("http://cdn.search.com"), 1, network_isolation_key},
+       {GURL("http://ads.search.com"), 0, network_isolation_key}});
   EXPECT_CALL(*mock_predictor_, PredictPreconnectOrigins(main_frame_url, _))
       .WillOnce(DoAll(SetArgPointee<1>(prediction), Return(true)));
   EXPECT_CALL(
       *mock_preconnect_manager_,
-      StartProxy(main_frame_url, std::vector<PreconnectRequest>(
-                                     {{GURL("http://search.com"), 2},
-                                      {GURL("http://en.search.com"), 1},
-                                      {GURL("http://cdn.search.com"), 1},
-                                      {GURL("http://ads.search.com"), 0}})));
+      StartProxy(
+          main_frame_url,
+          std::vector<PreconnectRequest>(
+              {{GURL("http://search.com"), 2, network_isolation_key},
+               {GURL("http://en.search.com"), 1, network_isolation_key},
+               {GURL("http://cdn.search.com"), 1, network_isolation_key},
+               {GURL("http://ads.search.com"), 0, network_isolation_key}})));
   predictor_->PrepareForPageLoad(main_frame_url, HintOrigin::EXTERNAL);
 }
 

chrome/browser/predictors/loading_stats_collector_unittest.cc

@@ -13,6 +13,7 @@
 #include "chrome/test/base/testing_profile.h"
 #include "content/public/test/test_browser_thread_bundle.h"
 #include "content/public/test/test_utils.h"
+#include "net/base/network_isolation_key.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 using testing::_;
@@ -77,7 +78,7 @@ void LoadingStatsCollectorTest::TestRedirectStatusHistogram(
   const std::string& script_url = "https://cdn.google.com/script.js";
   PreconnectPrediction prediction = CreatePreconnectPrediction(
       GURL(prediction_url).host(), initial_url != prediction_url,
-      {{GURL(script_url).GetOrigin(), 1}});
+      {{GURL(script_url).GetOrigin(), 1, net::NetworkIsolationKey()}});
   EXPECT_CALL(*mock_predictor_, PredictPreconnectOrigins(GURL(initial_url), _))
       .WillOnce(DoAll(SetArgPointee<1>(prediction), Return(true)));
 
@@ -105,12 +106,12 @@ TEST_F(LoadingStatsCollectorTest, TestPreconnectPrecisionRecallHistograms) {
   };
 
   // Predicts 4 origins: 2 useful, 2 useless.
-  PreconnectPrediction prediction =
-      CreatePreconnectPrediction(GURL(main_frame_url).host(), false,
-                                 {{GURL(main_frame_url).GetOrigin(), 1},
-                                  {GURL(gen(1)).GetOrigin(), 1},
-                                  {GURL(gen(2)).GetOrigin(), 1},
-                                  {GURL(gen(3)).GetOrigin(), 0}});
+  PreconnectPrediction prediction = CreatePreconnectPrediction(
+      GURL(main_frame_url).host(), false,
+      {{GURL(main_frame_url).GetOrigin(), 1, net::NetworkIsolationKey()},
+       {GURL(gen(1)).GetOrigin(), 1, net::NetworkIsolationKey()},
+       {GURL(gen(2)).GetOrigin(), 1, net::NetworkIsolationKey()},
+       {GURL(gen(3)).GetOrigin(), 0, net::NetworkIsolationKey()}});
   EXPECT_CALL(*mock_predictor_,
               PredictPreconnectOrigins(GURL(main_frame_url), _))
       .WillOnce(DoAll(SetArgPointee<1>(prediction), Return(true)));

chrome/browser/predictors/loading_test_util.cc

@@ -211,7 +211,8 @@ std::ostream& operator<<(std::ostream& os, const NavigationID& navigation_id) {
 
 std::ostream& operator<<(std::ostream& os, const PreconnectRequest& request) {
   return os << "[" << request.origin << "," << request.num_sockets << ","
-            << request.allow_credentials << "]";
+            << request.allow_credentials << ","
+            << request.network_isolation_key.ToDebugString() << "]";
 }
 
 std::ostream& operator<<(std::ostream& os,
@@ -284,7 +285,8 @@ bool operator==(const OriginStat& lhs, const OriginStat& rhs) {
 
 bool operator==(const PreconnectRequest& lhs, const PreconnectRequest& rhs) {
   return lhs.origin == rhs.origin && lhs.num_sockets == rhs.num_sockets &&
-         lhs.allow_credentials == rhs.allow_credentials;
+         lhs.allow_credentials == rhs.allow_credentials &&
+         lhs.network_isolation_key == rhs.network_isolation_key;
 }
 
 bool operator==(const PreconnectPrediction& lhs,

chrome/browser/predictors/resource_prefetch_predictor.cc

@@ -24,6 +24,7 @@
 #include "components/history/core/browser/url_utils.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/web_contents.h"
+#include "url/origin.h"
 
 using content::BrowserThread;
 
@@ -61,10 +62,10 @@ void InitializeOnDBSequence(
 PreconnectRequest::PreconnectRequest(
     const GURL& origin,
     int num_sockets,
-    net::NetworkIsolationKey network_isolation_key)
+    const net::NetworkIsolationKey& network_isolation_key)
     : origin(origin),
       num_sockets(num_sockets),
-      network_isolation_key(std::move(network_isolation_key)) {
+      network_isolation_key(network_isolation_key) {
   DCHECK_GE(num_sockets, 0);
 }
 
@@ -224,6 +225,16 @@ bool ResourcePrefetchPredictor::PredictPreconnectOrigins(
   }
 
   bool has_any_prediction = false;
+
+  // TODO(https://crbug.com/987735): Use the NetworkIsolationKey of the final
+  // destination in the case of redirects. This will require recording the port,
+  // which is not currently logged. That will not result in using the correct
+  // NetworkIsolationKey in the case of intermediary redirects, but given the
+  // relatively low accurace of redirect predictions, seems likely not worth
+  // fixing.
+  url::Origin origin = url::Origin::Create(url);
+  net::NetworkIsolationKey network_isolation_key(origin, origin);
+
   for (const OriginStat& origin : data.origins()) {
     float confidence = static_cast<float>(origin.number_of_hits()) /
                        (origin.number_of_hits() + origin.number_of_misses());
@@ -232,10 +243,13 @@ bool ResourcePrefetchPredictor::PredictPreconnectOrigins(
 
     has_any_prediction = true;
     if (prediction) {
-      if (confidence > kMinOriginConfidenceToTriggerPreconnect)
-        prediction->requests.emplace_back(GURL(origin.origin()), 1);
-      else
-        prediction->requests.emplace_back(GURL(origin.origin()), 0);
+      if (confidence > kMinOriginConfidenceToTriggerPreconnect) {
+        prediction->requests.emplace_back(GURL(origin.origin()), 1,
+                                          network_isolation_key);
+      } else {
+        prediction->requests.emplace_back(GURL(origin.origin()), 0,
+                                          network_isolation_key);
+      }
     }
   }
 

chrome/browser/predictors/resource_prefetch_predictor.h

@@ -60,13 +60,9 @@ struct PreconnectRequest {
   // 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());
+                    const net::NetworkIsolationKey& network_isolation_key);
 
   GURL origin;
   // A zero-value means that we need to preresolve a host only.

chrome/browser/predictors/resource_prefetch_predictor_unittest.cc

@@ -25,8 +25,11 @@
 #include "components/sessions/core/session_id.h"
 #include "content/public/test/test_browser_thread_bundle.h"
 #include "content/public/test/test_utils.h"
+#include "net/base/network_isolation_key.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
+#include "url/gurl.h"
+#include "url/origin.h"
 
 using testing::StrictMock;
 using testing::UnorderedElementsAre;
@@ -676,6 +679,8 @@ TEST_F(ResourcePrefetchPredictorTest, GetRedirectEndpoint) {
 
 TEST_F(ResourcePrefetchPredictorTest, TestPredictPreconnectOrigins) {
   const GURL main_frame_url("http://google.com/?query=cats");
+  const url::Origin origin = url::Origin::Create(main_frame_url);
+  const net::NetworkIsolationKey network_isolation_key(origin, origin);
   auto prediction = std::make_unique<PreconnectPrediction>();
   // No prefetch data.
   EXPECT_FALSE(predictor_->IsUrlPreconnectable(main_frame_url));
@@ -704,7 +709,8 @@ TEST_F(ResourcePrefetchPredictorTest, TestPredictPreconnectOrigins) {
   EXPECT_EQ(*prediction,
             CreatePreconnectPrediction(
                 "google.com", false,
-                {{GURL(gen_origin(1)), 1}, {GURL(gen_origin(2)), 0}}));
+                {{GURL(gen_origin(1)), 1, network_isolation_key},
+                 {GURL(gen_origin(2)), 0, network_isolation_key}}));
 
   // Add a redirect.
   RedirectData redirect = CreateRedirectData("google.com", 3);
@@ -729,9 +735,9 @@ TEST_F(ResourcePrefetchPredictorTest, TestPredictPreconnectOrigins) {
   EXPECT_TRUE(predictor_->IsUrlPreconnectable(main_frame_url));
   EXPECT_TRUE(
       predictor_->PredictPreconnectOrigins(main_frame_url, prediction.get()));
-  EXPECT_EQ(*prediction,
-            CreatePreconnectPrediction("www.google.com", true,
-                                       {{GURL(gen_origin(4)), 1}}));
+  EXPECT_EQ(*prediction, CreatePreconnectPrediction("www.google.com", true,
+                                                    {{GURL(gen_origin(4)), 1,
+                                                      network_isolation_key}}));
 }
 
 }  // namespace predictors