Add NetworkIsolationKey argument to NetworkContext::LookUpProxyForURL().

This is needed to separate out host resolutions made from different
sites. Also make PreconnectManager pass in the correct
NetworkIsolationKey. The two other consumers of the API currently pass
in empty NetworkIsolationKeys, and will be fixed in followup CLs.

Bug: 1021661
TBR: hashimoto@chromium.org, raymes@chromium.org, jam@chromium.org
Change-Id: I49d6d147f25dacce9d02112b2d0bfbe75a8a9b0b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1925136Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Oliver Chang <ochang@chromium.org>
Reviewed-by: Eric Roman <eroman@chromium.org>
Reviewed-by: Alex Ilin <alexilin@chromium.org>
Commit-Queue: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#718957}

chrome/browser/chromeos/dbus/proxy_resolution_service_provider.cc

@@ -19,6 +19,7 @@
 #include "mojo/public/cpp/bindings/pending_remote.h"
 #include "mojo/public/cpp/bindings/receiver.h"
 #include "net/base/net_errors.h"
+#include "net/base/network_isolation_key.h"
 #include "services/network/public/mojom/network_context.mojom.h"
 #include "third_party/cros_system_api/dbus/service_constants.h"
 #include "url/gurl.h"
@@ -53,7 +54,9 @@ class ProxyLookupRequest : public network::mojom::ProxyLookupClient {
         &ProxyLookupRequest::OnProxyLookupComplete, base::Unretained(this),
         net::ERR_ABORTED, base::nullopt));
 
+    // TODO(https://crbug.com/1021661): Pass in a non-empty NetworkIsolationKey.
     network_context->LookUpProxyForURL(source_url,
+                                       net::NetworkIsolationKey::Todo(),
                                        std::move(proxy_lookup_client));
   }
 

chrome/browser/chromeos/dbus/proxy_resolution_service_provider_unittest.cc

@@ -45,6 +45,7 @@ class MockNetworkContext : public network::TestNetworkContext {
   // network::mojom::NetworkContext implementation:
   void LookUpProxyForURL(
       const GURL& url,
+      const net::NetworkIsolationKey& network_isolation_key,
       mojo::PendingRemote<::network::mojom::ProxyLookupClient>
           proxy_lookup_client) override {
     mojo::Remote<::network::mojom::ProxyLookupClient>

chrome/browser/predictors/loading_predictor_browsertest.cc

@@ -293,12 +293,16 @@ class TestPreconnectManagerObserver : public PreconnectManager::Observer {
     CheckForWaitingLoop();
   }
 
-  void OnProxyLookupFinished(const GURL& url, bool success) override {
-    GURL origin = url.GetOrigin();
+  void OnProxyLookupFinished(
+      const GURL& url,
+      const net::NetworkIsolationKey& network_isolation_key,
+      bool success) override {
+    ResolveProxyRequestInfo resolve_info{url::Origin::Create(url),
+                                         network_isolation_key};
     if (success)
-      successful_proxy_lookups_.insert(origin);
+      successful_proxy_lookups_.insert(resolve_info);
     else
-      unsuccessful_proxy_lookups_.insert(origin);
+      unsuccessful_proxy_lookups_.insert(resolve_info);
     CheckForWaitingLoop();
   }
 
@@ -309,10 +313,13 @@ class TestPreconnectManagerObserver : public PreconnectManager::Observer {
     Wait();
   }
 
-  void WaitUntilProxyLookedUp(const GURL& url) {
+  void WaitUntilProxyLookedUp(
+      const GURL& url,
+      const net::NetworkIsolationKey& network_isolation_key) {
     wait_event_ = WaitEvent::kProxy;
-    DCHECK(waiting_on_proxy_.is_empty());
-    waiting_on_proxy_ = url;
+    DCHECK(waiting_on_proxy_.IsEmpty());
+    waiting_on_proxy_ = ResolveProxyRequestInfo{url::Origin::Create(url),
+                                                network_isolation_key};
     Wait();
   }
 
@@ -330,18 +337,40 @@ class TestPreconnectManagerObserver : public PreconnectManager::Observer {
     return base::Contains(successful_dns_lookups_, host);
   }
 
-  bool HasProxyBeenLookedUp(const GURL& url) {
-    return base::Contains(successful_proxy_lookups_, url.GetOrigin()) ||
-           base::Contains(unsuccessful_proxy_lookups_, url.GetOrigin());
-  }
-
-  bool ProxyFound(const GURL& url) {
-    return base::Contains(successful_proxy_lookups_, url.GetOrigin());
+  bool ProxyFound(const GURL& url,
+                  const net::NetworkIsolationKey& network_isolation_key) {
+    return base::Contains(successful_proxy_lookups_,
+                          ResolveProxyRequestInfo{url::Origin::Create(url),
+                                                  network_isolation_key});
   }
 
  private:
   enum class WaitEvent { kNone, kDns, kProxy };
 
+  struct ResolveProxyRequestInfo {
+    bool operator<(const ResolveProxyRequestInfo& other) const {
+      return std::tie(origin, network_isolation_key) <
+             std::tie(other.origin, other.network_isolation_key);
+    }
+
+    bool operator==(const ResolveProxyRequestInfo& other) const {
+      return std::tie(origin, network_isolation_key) ==
+             std::tie(other.origin, other.network_isolation_key);
+    }
+
+    bool IsEmpty() const {
+      return origin.opaque() && network_isolation_key.IsEmpty();
+    }
+
+    url::Origin origin;
+    net::NetworkIsolationKey network_isolation_key;
+  };
+
+  bool HasProxyBeenLookedUp(const ResolveProxyRequestInfo& resolve_proxy_info) {
+    return base::Contains(successful_proxy_lookups_, resolve_proxy_info) ||
+           base::Contains(unsuccessful_proxy_lookups_, resolve_proxy_info);
+  }
+
   void Wait() {
     base::RunLoop run_loop;
     DCHECK(!run_loop_);
@@ -362,7 +391,7 @@ class TestPreconnectManagerObserver : public PreconnectManager::Observer {
       case WaitEvent::kProxy:
         if (!HasProxyBeenLookedUp(waiting_on_proxy_))
           return;
-        waiting_on_proxy_ = GURL();
+        waiting_on_proxy_ = ResolveProxyRequestInfo();
         break;
     }
     DCHECK(run_loop_);
@@ -378,9 +407,9 @@ class TestPreconnectManagerObserver : public PreconnectManager::Observer {
   std::set<std::string> successful_dns_lookups_;
   std::set<std::string> unsuccessful_dns_lookups_;
 
-  GURL waiting_on_proxy_;
-  std::set<GURL> successful_proxy_lookups_;
-  std::set<GURL> unsuccessful_proxy_lookups_;
+  ResolveProxyRequestInfo waiting_on_proxy_;
+  std::set<ResolveProxyRequestInfo> successful_proxy_lookups_;
+  std::set<ResolveProxyRequestInfo> unsuccessful_proxy_lookups_;
 
   std::set<GURL> preconnect_url_attempts_;
 };
@@ -1367,14 +1396,18 @@ IN_PROC_BROWSER_TEST_F(LoadingPredictorBrowserTestWithProxy,
   GURL url = embedded_test_server()->GetURL(
       "test.com", GetPathWithPortReplacement(kHtmlSubresourcesPath,
                                              embedded_test_server()->port()));
+  url::Origin origin = url::Origin::Create(url);
+  net::NetworkIsolationKey network_isolation_key(origin, origin);
   ui_test_utils::NavigateToURL(browser(), url);
   ResetNetworkState();
   ResetPredictorState();
 
   auto observer = NavigateToURLAsync(url);
   EXPECT_TRUE(observer->WaitForRequestStart());
-  preconnect_manager_observer()->WaitUntilProxyLookedUp(url);
-  EXPECT_TRUE(preconnect_manager_observer()->ProxyFound(url));
+  preconnect_manager_observer()->WaitUntilProxyLookedUp(url,
+                                                        network_isolation_key);
+  EXPECT_TRUE(
+      preconnect_manager_observer()->ProxyFound(url, network_isolation_key));
   // We should preconnect only 2 sockets for the main frame host.
   const size_t expected_connections = 2;
   connection_tracker()->WaitForAcceptedConnections(expected_connections);
@@ -1391,6 +1424,8 @@ IN_PROC_BROWSER_TEST_F(LoadingPredictorBrowserTestWithProxy,
   GURL url = embedded_test_server()->GetURL(
       "test.com", GetPathWithPortReplacement(kHtmlSubresourcesPath,
                                              embedded_test_server()->port()));
+  url::Origin origin = url::Origin::Create(url);
+  net::NetworkIsolationKey network_isolation_key(origin, origin);
   ui_test_utils::NavigateToURL(browser(), url);
   ResetNetworkState();
 
@@ -1398,8 +1433,10 @@ IN_PROC_BROWSER_TEST_F(LoadingPredictorBrowserTestWithProxy,
   EXPECT_TRUE(observer->WaitForRequestStart());
   for (auto* const host : kHtmlSubresourcesHosts) {
     GURL url = embedded_test_server()->GetURL(host, "/");
-    preconnect_manager_observer()->WaitUntilProxyLookedUp(url);
-    EXPECT_TRUE(preconnect_manager_observer()->ProxyFound(url));
+    preconnect_manager_observer()->WaitUntilProxyLookedUp(
+        url, network_isolation_key);
+    EXPECT_TRUE(
+        preconnect_manager_observer()->ProxyFound(url, network_isolation_key));
   }
   // 2 connections to the main frame host + 1 connection per host for others.
   const size_t expected_connections = base::size(kHtmlSubresourcesHosts) + 1;

chrome/browser/predictors/preconnect_manager.cc

@@ -193,6 +193,7 @@ std::unique_ptr<ResolveHostClientImpl> PreconnectManager::PreresolveUrl(
 
 std::unique_ptr<ProxyLookupClientImpl> PreconnectManager::LookupProxyForUrl(
     const GURL& url,
+    const net::NetworkIsolationKey& network_isolation_key,
     ProxyLookupCallback callback) const {
   DCHECK(url.GetOrigin() == url);
   DCHECK(url.SchemeIsHTTPOrHTTPS());
@@ -203,8 +204,8 @@ std::unique_ptr<ProxyLookupClientImpl> PreconnectManager::LookupProxyForUrl(
     return nullptr;
   }
 
-  return std::make_unique<ProxyLookupClientImpl>(url, std::move(callback),
-                                                 network_context);
+  return std::make_unique<ProxyLookupClientImpl>(
+      url, network_isolation_key, std::move(callback), network_context);
 }
 
 void PreconnectManager::TryToLaunchPreresolveJobs() {
@@ -223,8 +224,9 @@ void PreconnectManager::TryToLaunchPreresolveJobs() {
       // configuration is in place, which improves efficiency, and is also
       // important if the unproxied DNS may contain incorrect entries.
       job->proxy_lookup_client = LookupProxyForUrl(
-          job->url, base::BindOnce(&PreconnectManager::OnProxyLookupFinished,
-                                   weak_factory_.GetWeakPtr(), job_id));
+          job->url, job->network_isolation_key,
+          base::BindOnce(&PreconnectManager::OnProxyLookupFinished,
+                         weak_factory_.GetWeakPtr(), job_id));
       if (info)
         ++info->inflight_count;
       ++inflight_preresolves_count_;
@@ -261,8 +263,10 @@ void PreconnectManager::OnProxyLookupFinished(PreresolveJobId job_id,
   PreresolveJob* job = preresolve_jobs_.Lookup(job_id);
   DCHECK(job);
 
-  if (observer_)
-    observer_->OnProxyLookupFinished(job->url, success);
+  if (observer_) {
+    observer_->OnProxyLookupFinished(job->url, job->network_isolation_key,
+                                     success);
+  }
 
   job->proxy_lookup_client = nullptr;
   if (success) {

chrome/browser/predictors/preconnect_manager.h

@@ -132,7 +132,10 @@ class PreconnectManager {
                                  bool allow_credentials) {}
 
     virtual void OnPreresolveFinished(const GURL& url, bool success) {}
-    virtual void OnProxyLookupFinished(const GURL& url, bool success) {}
+    virtual void OnProxyLookupFinished(
+        const GURL& url,
+        const net::NetworkIsolationKey& network_isolation_key,
+        bool success) {}
   };
 
   static const size_t kMaxInflightPreresolves = 3;
@@ -185,6 +188,7 @@ class PreconnectManager {
       ResolveHostCallback callback) const;
   std::unique_ptr<ProxyLookupClientImpl> LookupProxyForUrl(
       const GURL& url,
+      const net::NetworkIsolationKey& network_isolation_key,
       ProxyLookupCallback callback) const;
 
   void TryToLaunchPreresolveJobs();

chrome/browser/predictors/preconnect_manager_unittest.cc

@@ -86,6 +86,7 @@ class MockNetworkContext : public network::TestNetworkContext {
   }
 
   void LookUpProxyForURL(const GURL& url,
+                         const net::NetworkIsolationKey& network_isolation_key,
                          mojo::PendingRemote<network::mojom::ProxyLookupClient>
                              proxy_lookup_client) override {
     EXPECT_TRUE(

chrome/browser/predictors/proxy_lookup_client_impl.cc

@@ -19,12 +19,13 @@ namespace predictors {
 
 ProxyLookupClientImpl::ProxyLookupClientImpl(
     const GURL& url,
+    const net::NetworkIsolationKey& network_isolation_key,
     ProxyLookupCallback callback,
     network::mojom::NetworkContext* network_context)
     : callback_(std::move(callback)) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   network_context->LookUpProxyForURL(
-      url,
+      url, network_isolation_key,
       receiver_.BindNewPipeAndPassRemote(base::CreateSingleThreadTaskRunner(
           {content::BrowserThread::UI,
            content::BrowserTaskType::kPreconnect})));

chrome/browser/predictors/proxy_lookup_client_impl.h

@@ -13,6 +13,10 @@
 
 class GURL;
 
+namespace net {
+class NetworkIsolationKey;
+}
+
 namespace network {
 namespace mojom {
 class NetworkContext;
@@ -30,6 +34,7 @@ class ProxyLookupClientImpl : public network::mojom::ProxyLookupClient {
   // Starts the proxy lookup for |url|. |callback| is called when the proxy
   // lookup is completed or when an error occurs.
   ProxyLookupClientImpl(const GURL& url,
+                        const net::NetworkIsolationKey& network_isolation_key,
                         ProxyLookupCallback callback,
                         network::mojom::NetworkContext* network_context);
   // Cancels the request if it hasn't been completed yet.

content/browser/renderer_host/pepper/pepper_network_proxy_host.cc

@@ -18,6 +18,7 @@
 #include "content/public/browser/storage_partition.h"
 #include "content/public/common/socket_permission_request.h"
 #include "mojo/public/cpp/bindings/pending_remote.h"
+#include "net/base/network_isolation_key.h"
 #include "net/proxy_resolution/proxy_info.h"
 #include "ppapi/c/pp_errors.h"
 #include "ppapi/host/dispatch_host_message.h"
@@ -45,8 +46,9 @@ bool LookUpProxyForURLCallback(
   StoragePartition* storage_partition = BrowserContext::GetStoragePartition(
       site_instance->GetBrowserContext(), site_instance);
 
+  // TODO(https://crbug.com/1021661): Pass in a non-empty NetworkIsolationKey.
   storage_partition->GetNetworkContext()->LookUpProxyForURL(
-      url, std::move(proxy_lookup_client));
+      url, net::NetworkIsolationKey::Todo(), std::move(proxy_lookup_client));
   return true;
 }
 

content/browser/resolve_proxy_msg_helper.cc

@@ -10,6 +10,7 @@
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/render_process_host.h"
 #include "content/public/browser/storage_partition.h"
+#include "net/base/network_isolation_key.h"
 #include "net/proxy_resolution/proxy_info.h"
 #include "services/network/public/mojom/network_context.mojom.h"
 
@@ -83,9 +84,11 @@ bool ResolveProxyMsgHelper::SendRequestToNetworkService(
   // Fail the request if there's no such RenderProcessHost;
   if (!render_process_host)
     return false;
+  // TODO(https://crbug.com/1021661): Pass in a non-empty NetworkIsolationKey.
   render_process_host->GetStoragePartition()
       ->GetNetworkContext()
-      ->LookUpProxyForURL(url, std::move(proxy_lookup_client));
+      ->LookUpProxyForURL(url, net::NetworkIsolationKey::Todo(),
+                          std::move(proxy_lookup_client));
   return true;
 }
 

services/network/network_context.cc

@@ -1099,11 +1099,12 @@ void NetworkContext::CreateProxyResolvingSocketFactory(
 
 void NetworkContext::LookUpProxyForURL(
     const GURL& url,
+    const net::NetworkIsolationKey& network_isolation_key,
     mojo::PendingRemote<mojom::ProxyLookupClient> proxy_lookup_client) {
   DCHECK(proxy_lookup_client);
   std::unique_ptr<ProxyLookupRequest> proxy_lookup_request(
-      std::make_unique<ProxyLookupRequest>(std::move(proxy_lookup_client),
-                                           this));
+      std::make_unique<ProxyLookupRequest>(std::move(proxy_lookup_client), this,
+                                           network_isolation_key));
   ProxyLookupRequest* proxy_lookup_request_ptr = proxy_lookup_request.get();
   proxy_lookup_requests_.insert(std::move(proxy_lookup_request));
   proxy_lookup_request_ptr->Start(url);

services/network/network_context.h

@@ -278,6 +278,7 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) NetworkContext
       mojo::PendingReceiver<mojom::ProxyResolvingSocketFactory> receiver)
       override;
   void LookUpProxyForURL(const GURL& url,
+                         const net::NetworkIsolationKey& network_isolation_key,
                          mojo::PendingRemote<mojom::ProxyLookupClient>
                              proxy_lookup_client) override;
   void ForceReloadProxyConfig(ForceReloadProxyConfigCallback callback) override;

services/network/proxy_lookup_request.cc

@@ -9,7 +9,6 @@
 #include "base/bind.h"
 #include "base/optional.h"
 #include "net/base/net_errors.h"
-#include "net/base/network_isolation_key.h"
 #include "net/http/http_network_session.h"
 #include "net/http/http_transaction_factory.h"
 #include "net/log/net_log_with_source.h"
@@ -22,8 +21,10 @@ namespace network {
 
 ProxyLookupRequest::ProxyLookupRequest(
     mojo::PendingRemote<mojom::ProxyLookupClient> proxy_lookup_client,
-    NetworkContext* network_context)
+    NetworkContext* network_context,
+    const net::NetworkIsolationKey& network_isolation_key)
     : network_context_(network_context),
+      network_isolation_key_(network_isolation_key),
       proxy_lookup_client_(std::move(proxy_lookup_client)) {
   DCHECK(proxy_lookup_client_);
 }
@@ -42,14 +43,13 @@ void ProxyLookupRequest::Start(const GURL& url) {
   // TODO(mmenke): The NetLogWithSource() means nothing is logged. Fix that.
   //
   // TODO(https://crbug.com/1023435): Pass along a NetworkIsolationKey.
-  int result =
-      network_context_->url_request_context()
-          ->proxy_resolution_service()
-          ->ResolveProxy(url, std::string(), net::NetworkIsolationKey::Todo(),
-                         &proxy_info_,
-                         base::BindOnce(&ProxyLookupRequest::OnResolveComplete,
-                                        base::Unretained(this)),
-                         &request_, net::NetLogWithSource());
+  int result = network_context_->url_request_context()
+                   ->proxy_resolution_service()
+                   ->ResolveProxy(
+                       url, std::string(), network_isolation_key_, &proxy_info_,
+                       base::BindOnce(&ProxyLookupRequest::OnResolveComplete,
+                                      base::Unretained(this)),
+                       &request_, net::NetLogWithSource());
   if (result != net::ERR_IO_PENDING)
     OnResolveComplete(result);
 }

services/network/proxy_lookup_request.h

@@ -13,6 +13,7 @@
 #include "base/macros.h"
 #include "mojo/public/cpp/bindings/pending_remote.h"
 #include "mojo/public/cpp/bindings/remote.h"
+#include "net/base/network_isolation_key.h"
 #include "net/proxy_resolution/proxy_info.h"
 #include "net/proxy_resolution/proxy_resolution_service.h"
 #include "services/network/public/mojom/proxy_lookup_client.mojom.h"
@@ -27,7 +28,8 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) ProxyLookupRequest {
  public:
   ProxyLookupRequest(
       mojo::PendingRemote<mojom::ProxyLookupClient> proxy_lookup_client,
-      NetworkContext* network_context);
+      NetworkContext* network_context,
+      const net::NetworkIsolationKey& network_isolation_key);
   ~ProxyLookupRequest();
 
   // Starts looking up what proxy to use for |url|. On completion, will inform
@@ -44,6 +46,7 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) ProxyLookupRequest {
   void DestroySelf();
 
   NetworkContext* const network_context_;
+  const net::NetworkIsolationKey network_isolation_key_;
   mojo::Remote<mojom::ProxyLookupClient> proxy_lookup_client_;
 
   net::ProxyInfo proxy_info_;

services/network/public/mojom/network_context.mojom

@@ -1031,8 +1031,11 @@ interface NetworkContext {
   CreateProxyResolvingSocketFactory(
       pending_receiver<ProxyResolvingSocketFactory> factory);
 
-  // Looks up what proxy to use for a particular URL.
+  // Looks up what proxy to use for a particular URL. |network_isolation_key|
+  // is used to partition the DNS cache in the case a PAC script is used, and
+  // should match the NIK of the frame the lookup is for use with.
   LookUpProxyForURL(url.mojom.Url url,
+                    NetworkIsolationKey network_isolation_key,
                     pending_remote<ProxyLookupClient> proxy_lookup_client);
 
   // Forces refetching the proxy configuration, and applying it.

services/network/test/test_network_context.h

@@ -167,6 +167,7 @@ class TestNetworkContext : public mojom::NetworkContext {
       override {}
   void LookUpProxyForURL(
       const GURL& url,
+      const net::NetworkIsolationKey& network_isolation_key,
       mojo::PendingRemote<::network::mojom::ProxyLookupClient>
           proxy_lookup_client) override {}
   void CreateNetLogExporter(