Keep previously configured DRP servers in network service

This matches the logic in http://crrev.com/c/1045025. A couple of the
most recent configs are kept around in case the config changes in the
middle of a request, which will allow the headers to be correct in that
case.

Also adds a browsertest for basic config changes.

Bug: 721403
Change-Id: I788dcf52efa9a86031fd33cbc77d1be45022b19c
Reviewed-on: https://chromium-review.googlesource.com/c/1292510
Commit-Queue: Clark DuVall <cduvall@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: rajendrant <rajendrant@chromium.org>
Cr-Commit-Position: refs/heads/master@{#601701}

services/network/network_service_proxy_delegate.cc

@@ -16,6 +16,9 @@ namespace {
 // alternate proxy list.
 constexpr size_t kMaxCacheSize = 15;
 
+// The maximum number of previous configs to keep.
+constexpr size_t kMaxPreviousConfigs = 2;
+
 void GetAlternativeProxy(const GURL& url,
                          const net::ProxyRetryInfoMap& proxy_retry_info,
                          net::ProxyInfo* result) {
@@ -55,12 +58,19 @@ bool ApplyProxyConfigToProxyInfo(const net::ProxyConfig::ProxyRules& rules,
 bool CheckProxyList(const net::ProxyList& proxy_list,
                     const net::ProxyServer& target_proxy) {
   for (const auto& proxy : proxy_list.GetAll()) {
-    if (proxy.host_port_pair().Equals(target_proxy.host_port_pair()))
+    if (!proxy.is_direct() &&
+        proxy.host_port_pair().Equals(target_proxy.host_port_pair())) {
       return true;
+    }
   }
   return false;
 }
 
+// Whether the custom proxy can proxy |url|.
+bool IsURLValidForProxy(const GURL& url) {
+  return url.SchemeIs(url::kHttpScheme) && !net::IsLocalhost(url);
+}
+
 }  // namespace
 
 NetworkServiceProxyDelegate::NetworkServiceProxyDelegate(
@@ -96,9 +106,7 @@ void NetworkServiceProxyDelegate::OnBeforeSendHeaders(
     if (url_loader) {
       headers->MergeFrom(url_loader->custom_proxy_post_cache_headers());
     }
-    // TODO(crbug.com/721403): This check may be incorrect if a new proxy config
-    // is set between OnBeforeStartTransaction and here.
-  } else if (MayProxyURL(request->url())) {
+  } else if (MayHaveProxiedURL(request->url())) {
     for (const auto& kv : proxy_config_->pre_cache_headers.GetHeaderVector()) {
       headers->RemoveHeader(kv.key);
     }
@@ -138,6 +146,11 @@ void NetworkServiceProxyDelegate::OnCustomProxyConfigUpdated(
     mojom::CustomProxyConfigPtr proxy_config) {
   DCHECK(proxy_config->rules.empty() ||
          !proxy_config->rules.proxies_for_http.IsEmpty());
+  if (proxy_config_) {
+    previous_proxy_configs_.push_front(std::move(proxy_config_));
+    if (previous_proxy_configs_.size() > kMaxPreviousConfigs)
+      previous_proxy_configs_.pop_back();
+  }
   proxy_config_ = std::move(proxy_config);
 }
 
@@ -146,12 +159,34 @@ bool NetworkServiceProxyDelegate::IsInProxyConfig(
   if (!proxy_server.is_valid() || proxy_server.is_direct())
     return false;
 
-  return CheckProxyList(proxy_config_->rules.proxies_for_http, proxy_server);
+  if (CheckProxyList(proxy_config_->rules.proxies_for_http, proxy_server))
+    return true;
+
+  for (const auto& config : previous_proxy_configs_) {
+    if (CheckProxyList(config->rules.proxies_for_http, proxy_server))
+      return true;
+  }
+
+  return false;
 }
 
 bool NetworkServiceProxyDelegate::MayProxyURL(const GURL& url) const {
-  return url.SchemeIs(url::kHttpScheme) && !proxy_config_->rules.empty() &&
-         !net::IsLocalhost(url);
+  return IsURLValidForProxy(url) && !proxy_config_->rules.empty();
+}
+
+bool NetworkServiceProxyDelegate::MayHaveProxiedURL(const GURL& url) const {
+  if (!IsURLValidForProxy(url))
+    return false;
+
+  if (!proxy_config_->rules.empty())
+    return true;
+
+  for (const auto& config : previous_proxy_configs_) {
+    if (!config->rules.empty())
+      return true;
+  }
+
+  return false;
 }
 
 bool NetworkServiceProxyDelegate::EligibleForProxy(

services/network/network_service_proxy_delegate.h

@@ -5,6 +5,8 @@
 #ifndef SERVICES_NETWORK_NETWORK_SERVICE_PROXY_DELEGATE_H_
 #define SERVICES_NETWORK_NETWORK_SERVICE_PROXY_DELEGATE_H_
 
+#include <deque>
+
 #include "base/component_export.h"
 #include "base/containers/mru_cache.h"
 #include "base/macros.h"
@@ -51,6 +53,10 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) NetworkServiceProxyDelegate
   // Whether the current config may proxy |url|.
   bool MayProxyURL(const GURL& url) const;
 
+  // Whether the current config may have proxied |url| with the current config
+  // or a previous config.
+  bool MayHaveProxiedURL(const GURL& url) const;
+
   // Whether the |url| with current |proxy_info| is eligible to be proxied.
   bool EligibleForProxy(const net::ProxyInfo& proxy_info,
                         const GURL& url,
@@ -68,6 +74,11 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) NetworkServiceProxyDelegate
 
   base::MRUCache<std::string, bool> should_use_alternate_proxy_list_cache_;
 
+  // We keep track of a limited number of previous configs so we can determine
+  // if a request used a custom proxy if the config happened to change during
+  // the request.
+  std::deque<mojom::CustomProxyConfigPtr> previous_proxy_configs_;
+
   DISALLOW_COPY_AND_ASSIGN(NetworkServiceProxyDelegate);
 };
 

services/network/network_service_proxy_delegate_unittest.cc

@@ -29,11 +29,9 @@ class NetworkServiceProxyDelegateTest : public testing::Test {
  protected:
   std::unique_ptr<NetworkServiceProxyDelegate> CreateDelegate(
       mojom::CustomProxyConfigPtr config) {
-    mojom::CustomProxyConfigClientPtr client;
     auto delegate = std::make_unique<NetworkServiceProxyDelegate>(
-        mojo::MakeRequest(&client));
-    client->OnCustomProxyConfigUpdated(std::move(config));
-    scoped_task_environment_.RunUntilIdle();
+        mojo::MakeRequest(&client_));
+    SetConfig(std::move(config));
     return delegate;
   }
 
@@ -41,7 +39,13 @@ class NetworkServiceProxyDelegateTest : public testing::Test {
     return context_->CreateRequest(url, net::DEFAULT_PRIORITY, nullptr);
   }
 
+  void SetConfig(mojom::CustomProxyConfigPtr config) {
+    client_->OnCustomProxyConfigUpdated(std::move(config));
+    scoped_task_environment_.RunUntilIdle();
+  }
+
  private:
+  mojom::CustomProxyConfigClientPtr client_;
   std::unique_ptr<net::TestURLRequestContext> context_;
   base::test::ScopedTaskEnvironment scoped_task_environment_;
 };
@@ -75,6 +79,19 @@ TEST_F(NetworkServiceProxyDelegateTest,
   EXPECT_TRUE(headers.IsEmpty());
 }
 
+TEST_F(NetworkServiceProxyDelegateTest,
+       DoesNotAddHeadersBeforeCacheWithEmptyConfig) {
+  auto config = mojom::CustomProxyConfig::New();
+  config->pre_cache_headers.SetHeader("foo", "bar");
+  auto delegate = CreateDelegate(std::move(config));
+
+  net::HttpRequestHeaders headers;
+  auto request = CreateRequest(GURL(kHttpUrl));
+  delegate->OnBeforeStartTransaction(request.get(), &headers);
+
+  EXPECT_TRUE(headers.IsEmpty());
+}
+
 TEST_F(NetworkServiceProxyDelegateTest, DoesNotAddHeadersBeforeCacheForHttps) {
   auto config = mojom::CustomProxyConfig::New();
   config->rules.ParseFromString("http=proxy");
@@ -190,6 +207,49 @@ TEST_F(NetworkServiceProxyDelegateTest, KeepsPreCacheHeadersWhenProxyInConfig) {
   EXPECT_EQ(value, "bar");
 }
 
+TEST_F(NetworkServiceProxyDelegateTest, KeepsHeadersWhenConfigUpdated) {
+  auto config = mojom::CustomProxyConfig::New();
+  config->rules.ParseFromString("http=proxy");
+  config->pre_cache_headers.SetHeader("foo", "bar");
+  auto delegate = CreateDelegate(config->Clone());
+
+  // Update config with new proxy.
+  config->rules.ParseFromString("http=other");
+  SetConfig(std::move(config));
+
+  net::HttpRequestHeaders headers;
+  headers.SetHeader("foo", "bar");
+  auto request = CreateRequest(GURL(kHttpUrl));
+  net::ProxyInfo info;
+  info.UsePacString("PROXY proxy");
+  delegate->OnBeforeSendHeaders(request.get(), info, &headers);
+
+  std::string value;
+  EXPECT_TRUE(headers.GetHeader("foo", &value));
+  EXPECT_EQ(value, "bar");
+}
+
+TEST_F(NetworkServiceProxyDelegateTest,
+       RemovesPreCacheHeadersWhenConfigUpdatedToBeEmpty) {
+  auto config = mojom::CustomProxyConfig::New();
+  config->rules.ParseFromString("http=proxy");
+  config->pre_cache_headers.SetHeader("foo", "bar");
+  auto delegate = CreateDelegate(config->Clone());
+
+  // Update config with empty proxy rules.
+  config->rules = net::ProxyConfig::ProxyRules();
+  SetConfig(std::move(config));
+
+  net::HttpRequestHeaders headers;
+  headers.SetHeader("foo", "bar");
+  auto request = CreateRequest(GURL(kHttpUrl));
+  net::ProxyInfo info;
+  info.UseDirect();
+  delegate->OnBeforeSendHeaders(request.get(), info, &headers);
+
+  EXPECT_TRUE(headers.IsEmpty());
+}
+
 TEST_F(NetworkServiceProxyDelegateTest, OnResolveProxySuccessHttpProxy) {
   auto config = mojom::CustomProxyConfig::New();
   config->rules.ParseFromString("http=foo");