Remove partial support for proxy chains in PAC strings

For the moment, the plan is to not support proxy chains in PAC strings.
This CL rewrites a bunch of uses of `{ProxyInfo,ProxyList}.ToPacString`
to use `ToDebugString` (which *can* represent a chain), and removes
TODO's for supporting parsing PAC strings into chains.

Three spots that call ToPacString to return the result of proxy
resolution externally are updated to fail if that result contains a
multi-proxy chain.

Bug: 1491092
Change-Id: I30cdef9d3293312b0c52218f0e0baba639bcb76d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5034180
Reviewed-by: Adam Rice <ricea@chromium.org>
Reviewed-by: Roland Bock <rbock@google.com>
Commit-Queue: Dustin Mitchell <djmitche@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Reviewed-by: Ryo Hashimoto <hashimoto@chromium.org>
Reviewed-by: Derek Schuff <dschuff@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1228039}

android_webview/browser/aw_pac_processor.cc

@@ -444,6 +444,10 @@ jboolean AwPacProcessor::SetProxyScript(JNIEnv* env,
 bool AwPacProcessor::MakeProxyRequest(std::string url, std::string* result) {
   MakeProxyRequestJob job(this, url);
   if (job.ExecSync()) {
+    if (job.proxy_info().ContainsMultiProxyChain()) {
+      // Multi-proxy chains cannot be represented as a PAC string.
+      return false;
+    }
     *result = job.proxy_info().ToPacString();
     return true;
   } else {

chrome/browser/ash/dbus/proxy_resolution_service_provider.cc

@@ -82,6 +82,10 @@ class ProxyLookupRequest : public network::mojom::ProxyLookupClient {
     if (!proxy_info) {
       error = net::ErrorToString(net_error);
       result = kProxyInfoOnFailure;
+    } else if (proxy_info->ContainsMultiProxyChain()) {
+      // Multi-proxy chains cannot be represented as a PAC string.
+      error = net::ErrorToString(net::ERR_MANDATORY_PROXY_CONFIGURATION_FAILED);
+      result = kProxyInfoOnFailure;
     } else {
       result = proxy_info->ToPacString();
       if (proxy_info->is_http()) {

chrome/browser/lacros/net/network_settings_translation_unittest.cc

@@ -211,11 +211,11 @@ TEST(NetworkSettingsTranslationTest, CrosapiProxyToNetProxyManual) {
 
   auto actual = CrosapiProxyToNetProxy(std::move(ptr));
 
-  EXPECT_EQ(actual.value().proxy_rules().proxies_for_http.ToPacString(),
+  EXPECT_EQ(actual.value().proxy_rules().proxies_for_http.ToDebugString(),
             "PROXY proxy:80");
-  EXPECT_EQ(actual.value().proxy_rules().proxies_for_https.ToPacString(),
+  EXPECT_EQ(actual.value().proxy_rules().proxies_for_https.ToDebugString(),
             "HTTPS secure_proxy:81;PROXY secure_proxy2:82");
-  EXPECT_EQ(actual.value().proxy_rules().fallback_proxies.ToPacString(),
+  EXPECT_EQ(actual.value().proxy_rules().fallback_proxies.ToDebugString(),
             "SOCKS socks_proxy:82");
   EXPECT_EQ(actual.value().proxy_rules().bypass_rules.ToString(),
             "localhost;google.com;");

content/browser/renderer_host/pepper/pepper_network_proxy_host.cc

@@ -175,6 +175,9 @@ void PepperNetworkProxyHost::OnResolveProxyCompleted(
     // really no action a plugin can take, so there's no need to distinguish
     // which error occurred.
     context.params.set_result(PP_ERROR_FAILED);
+  } else if (proxy_info->ContainsMultiProxyChain()) {
+    // Multi-proxy chains cannot be represented as a PAC string.
+    context.params.set_result(PP_ERROR_FAILED);
   } else {
     pac_string = proxy_info->ToPacString();
   }

content/browser/renderer_host/pepper/pepper_proxy_lookup_helper_unittest.cc

@@ -159,7 +159,7 @@ TEST_F(PepperProxyLookupHelperTest, Success) {
   ClaimProxyLookupClient()->OnProxyLookupComplete(net::OK, proxy_info_response);
   WaitForLookupCompletion();
   ASSERT_TRUE(proxy_info());
-  EXPECT_EQ("PROXY result:80", proxy_info()->ToPacString());
+  EXPECT_EQ("PROXY result:80", proxy_info()->ToDebugString());
 }
 
 // Basic failure case - an error is passed to the PepperProxyLookupHelper

net/base/proxy_string_util.cc

@@ -99,8 +99,8 @@ std::string ConstructHostPortString(std::string_view hostname, uint16_t port) {
 }  // namespace
 
 ProxyChain PacResultElementToProxyChain(std::string_view pac_result_element) {
-  // TODO(https://crbug.com/1491092): Support parsing multi-hop proxy chains
-  // from PAC scripts.
+  // Proxy chains are not supported in PAC strings, so this is just parsed
+  // as a single server.
   return ProxyChain(PacResultElementToProxyServer(pac_result_element));
 }
 

net/base/proxy_string_util.h

@@ -19,10 +19,7 @@ namespace net {
 // element separated out from the complete semicolon-delimited PAC result
 // string.
 //
-// TODO(https://crbug.com/1491092): Update this once we add support for parsing
-// multi-hop proxies into a ProxyChain. Also, remove the ProxyServer functions
-// once we've updated the code to use ProxyChains instead (assuming these are no
-// longer needed).
+// Note that PAC strings cannot currently specify multi-proxy chains.
 //
 // PAC result elements have the format:
 // <scheme>" "<host>[":"<port>]

net/base/proxy_string_util_unittest.cc

@@ -216,9 +216,6 @@ TEST(ProxySpecificationUtilTest, PacResultElementToProxyServer) {
     EXPECT_TRUE(server.is_valid());
     EXPECT_EQ(test.expected_uri, ProxyServerToProxyUri(server));
 
-    // TODO(https://crbug.com/1491092): Split this into a new test when
-    // `PacResultElementToProxyChain()` does more than just wrap
-    // `PacResultElementToProxyServer()`.
     ProxyChain chain = PacResultElementToProxyChain(test.input_pac);
     EXPECT_TRUE(chain.IsValid());
     if (!chain.is_direct()) {
@@ -245,9 +242,6 @@ TEST(ProxySpecificationUtilTest, InvalidPacResultElementToProxyServer) {
     ProxyServer server = PacResultElementToProxyServer(test);
     EXPECT_FALSE(server.is_valid());
 
-    // TODO(https://crbug.com/1491092): Split this into a new test when
-    // `PacResultElementToProxyChain()` does more than just wrap
-    // `PacResultElementToProxyServer()`.
     ProxyChain chain = PacResultElementToProxyChain(test);
     EXPECT_FALSE(chain.IsValid());
   }

net/proxy_resolution/configured_proxy_resolution_service.cc

@@ -314,7 +314,7 @@ base::Value::Dict NetLogBadProxyListParams(
 // Returns NetLog parameters on a successful proxy resolution.
 base::Value::Dict NetLogFinishedResolvingProxyParams(const ProxyInfo* result) {
   base::Value::Dict dict;
-  dict.Set("pac_string", result->ToPacString());
+  dict.Set("proxy_info", result->ToDebugString());
   return dict;
 }
 

net/proxy_resolution/multi_threaded_proxy_resolver_unittest.cc

@@ -288,7 +288,7 @@ TEST_F(MultiThreadedProxyResolverTest, SingleThread_Basic) {
   // Wait for request 0 to finish.
   rv = callback0.WaitForResult();
   EXPECT_EQ(0, rv);
-  EXPECT_EQ("PROXY request0:80", results0.ToPacString());
+  EXPECT_EQ("PROXY request0:80", results0.ToDebugString());
 
   // The mock proxy resolver should have written 1 log entry. And
   // on completion, this should have been copied into |log0|.
@@ -327,15 +327,15 @@ TEST_F(MultiThreadedProxyResolverTest, SingleThread_Basic) {
 
   rv = callback1.WaitForResult();
   EXPECT_EQ(1, rv);
-  EXPECT_EQ("PROXY request1:80", results1.ToPacString());
+  EXPECT_EQ("PROXY request1:80", results1.ToDebugString());
 
   rv = callback2.WaitForResult();
   EXPECT_EQ(2, rv);
-  EXPECT_EQ("PROXY request2:80", results2.ToPacString());
+  EXPECT_EQ("PROXY request2:80", results2.ToDebugString());
 
   rv = callback3.WaitForResult();
   EXPECT_EQ(3, rv);
-  EXPECT_EQ("PROXY request3:80", results3.ToPacString());
+  EXPECT_EQ("PROXY request3:80", results3.ToDebugString());
 }
 
 // Tests that the NetLog is updated to include the time the request was waiting
@@ -391,7 +391,7 @@ TEST_F(MultiThreadedProxyResolverTest,
   // The NetLog has 1 entry that came from the MultiThreadedProxyResolver, and
   // 1 entry from the mock proxy resolver.
   EXPECT_EQ(0, callback0.WaitForResult());
-  EXPECT_EQ("PROXY request0:80", results0.ToPacString());
+  EXPECT_EQ("PROXY request0:80", results0.ToDebugString());
 
   auto entries0 =
       net_log_observer.GetEntriesForSource(log_with_source0.source());
@@ -401,7 +401,7 @@ TEST_F(MultiThreadedProxyResolverTest,
 
   // Check that request 1 completed as expected.
   EXPECT_EQ(1, callback1.WaitForResult());
-  EXPECT_EQ("PROXY request1:80", results1.ToPacString());
+  EXPECT_EQ("PROXY request1:80", results1.ToDebugString());
 
   auto entries1 =
       net_log_observer.GetEntriesForSource(log_with_source1.source());
@@ -414,7 +414,7 @@ TEST_F(MultiThreadedProxyResolverTest,
 
   // Check that request 2 completed as expected.
   EXPECT_EQ(2, callback2.WaitForResult());
-  EXPECT_EQ("PROXY request2:80", results2.ToPacString());
+  EXPECT_EQ("PROXY request2:80", results2.ToDebugString());
 
   auto entries2 =
       net_log_observer.GetEntriesForSource(log_with_source2.source());
@@ -484,13 +484,13 @@ TEST_F(MultiThreadedProxyResolverTest, SingleThread_CancelRequest) {
 
   rv = callback1.WaitForResult();
   EXPECT_EQ(1, rv);
-  EXPECT_EQ("PROXY request1:80", results1.ToPacString());
+  EXPECT_EQ("PROXY request1:80", results1.ToDebugString());
 
   rv = callback3.WaitForResult();
   // Note that since request2 was cancelled before reaching the resolver,
   // the request count is 2 and not 3 here.
   EXPECT_EQ(2, rv);
-  EXPECT_EQ("PROXY request3:80", results3.ToPacString());
+  EXPECT_EQ("PROXY request3:80", results3.ToDebugString());
 
   // Requests 0 and 2 which were cancelled, hence their completion callbacks
   // were never summoned.
@@ -619,7 +619,7 @@ TEST_F(MultiThreadedProxyResolverTest, ThreeThreads_Basic) {
   // Wait for request 0 to finish.
   rv = callback[0].WaitForResult();
   EXPECT_EQ(0, rv);
-  EXPECT_EQ("PROXY request0:80", results[0].ToPacString());
+  EXPECT_EQ("PROXY request0:80", results[0].ToDebugString());
   ASSERT_EQ(1u, factory().resolvers().size());
   EXPECT_EQ(1, factory().resolvers()[0]->request_count());
 

net/proxy_resolution/proxy_config_service_android_unittest.cc

@@ -118,7 +118,7 @@ class ProxyConfigServiceAndroidTestBase : public TestWithTaskEnvironment {
     EXPECT_EQ(ProxyConfigService::CONFIG_VALID, availability);
     ProxyInfo proxy_info;
     proxy_config.value().proxy_rules().Apply(GURL(url), &proxy_info);
-    EXPECT_EQ(expected, proxy_info.ToPacString());
+    EXPECT_EQ(expected, proxy_info.ToDebugString());
   }
 
   void SetProxyOverride(

net/proxy_resolution/proxy_config_unittest.cc

@@ -15,11 +15,11 @@ namespace net {
 namespace {
 
 void ExpectProxyServerEquals(const char* expectation,
-                             const ProxyList& proxy_servers) {
+                             const ProxyList& proxy_list) {
   if (expectation == nullptr) {
-    EXPECT_TRUE(proxy_servers.IsEmpty());
+    EXPECT_TRUE(proxy_list.IsEmpty());
   } else {
-    EXPECT_EQ(expectation, proxy_servers.ToPacString());
+    EXPECT_EQ(expectation, proxy_list.ToDebugString());
   }
 }
 
@@ -491,7 +491,7 @@ class ProxyConfigWebSocketTest : public ::testing::Test {
     rules_.ParseFromString(rules);
   }
   void Apply(const GURL& gurl) { rules_.Apply(gurl, &info_); }
-  std::string ToPacString() const { return info_.ToPacString(); }
+  std::string ToDebugString() const { return info_.ToDebugString(); }
 
   static GURL WsUrl() { return GURL(kWsUrl); }
   static GURL WssUrl() { return GURL(kWssUrl); }
@@ -504,7 +504,7 @@ class ProxyConfigWebSocketTest : public ::testing::Test {
 TEST_F(ProxyConfigWebSocketTest, UsesProxy) {
   ParseFromString("proxy:3128");
   Apply(WsUrl());
-  EXPECT_EQ("PROXY proxy:3128", ToPacString());
+  EXPECT_EQ("PROXY proxy:3128", ToDebugString());
 }
 
 // See RFC6455 Section 4.1. item 3, "_Proxy Usage_". Note that this favors a
@@ -513,7 +513,7 @@ TEST_F(ProxyConfigWebSocketTest, PrefersSocksV4) {
   ParseFromString(
       "http=proxy:3128 ; https=sslproxy:3128 ; socks=socksproxy:1080");
   Apply(WsUrl());
-  EXPECT_EQ("SOCKS socksproxy:1080", ToPacString());
+  EXPECT_EQ("SOCKS socksproxy:1080", ToDebugString());
 }
 
 // See RFC6455 Section 4.1. item 3, "_Proxy Usage_".
@@ -521,13 +521,13 @@ TEST_F(ProxyConfigWebSocketTest, PrefersSocksV5) {
   ParseFromString(
       "http=proxy:3128 ; https=sslproxy:3128 ; socks=socks5://socksproxy:1080");
   Apply(WsUrl());
-  EXPECT_EQ("SOCKS5 socksproxy:1080", ToPacString());
+  EXPECT_EQ("SOCKS5 socksproxy:1080", ToDebugString());
 }
 
 TEST_F(ProxyConfigWebSocketTest, PrefersHttpsToHttp) {
   ParseFromString("http=proxy:3128 ; https=sslproxy:3128");
   Apply(WssUrl());
-  EXPECT_EQ("PROXY sslproxy:3128", ToPacString());
+  EXPECT_EQ("PROXY sslproxy:3128", ToDebugString());
 }
 
 // Tests when a proxy-per-url-scheme configuration was used, and proxies are
@@ -538,9 +538,9 @@ TEST_F(ProxyConfigWebSocketTest, PrefersNonSocksFallbackOverHttps) {
   // The notation for "socks=" is abused to set the "fallback proxy".
   ParseFromString(
       "http=proxy:3128 ; https=sslproxy:3128; socks=https://httpsproxy");
-  EXPECT_EQ("HTTPS httpsproxy:443", rules_.fallback_proxies.ToPacString());
+  EXPECT_EQ("HTTPS httpsproxy:443", rules_.fallback_proxies.ToDebugString());
   Apply(WssUrl());
-  EXPECT_EQ("HTTPS httpsproxy:443", ToPacString());
+  EXPECT_EQ("HTTPS httpsproxy:443", ToDebugString());
 }
 
 // Tests when a proxy-per-url-scheme configuration was used, and the fallback
@@ -549,41 +549,41 @@ TEST_F(ProxyConfigWebSocketTest, PrefersNonSocksFallbackOverHttps) {
 TEST_F(ProxyConfigWebSocketTest, UsesNonSocksFallbackProxy) {
   // The notation for "socks=" is abused to set the "fallback proxy".
   ParseFromString("ftp=ftpproxy:3128; socks=https://httpsproxy");
-  EXPECT_EQ("HTTPS httpsproxy:443", rules_.fallback_proxies.ToPacString());
+  EXPECT_EQ("HTTPS httpsproxy:443", rules_.fallback_proxies.ToDebugString());
   Apply(WssUrl());
-  EXPECT_EQ("HTTPS httpsproxy:443", ToPacString());
+  EXPECT_EQ("HTTPS httpsproxy:443", ToDebugString());
 }
 
 TEST_F(ProxyConfigWebSocketTest, PrefersHttpsEvenForWs) {
   ParseFromString("http=proxy:3128 ; https=sslproxy:3128");
   Apply(WsUrl());
-  EXPECT_EQ("PROXY sslproxy:3128", ToPacString());
+  EXPECT_EQ("PROXY sslproxy:3128", ToDebugString());
 }
 
 TEST_F(ProxyConfigWebSocketTest, PrefersHttpToDirect) {
   ParseFromString("http=proxy:3128");
   Apply(WssUrl());
-  EXPECT_EQ("PROXY proxy:3128", ToPacString());
+  EXPECT_EQ("PROXY proxy:3128", ToDebugString());
 }
 
 TEST_F(ProxyConfigWebSocketTest, IgnoresFtpProxy) {
   ParseFromString("ftp=ftpproxy:3128");
   Apply(WssUrl());
-  EXPECT_EQ("DIRECT", ToPacString());
+  EXPECT_EQ("DIRECT", ToDebugString());
 }
 
 TEST_F(ProxyConfigWebSocketTest, ObeysBypassRules) {
   ParseFromString("http=proxy:3128 ; https=sslproxy:3128");
   rules_.bypass_rules.AddRuleFromString(".chromium.org");
   Apply(GURL("wss://codereview.chromium.org/feed"));
-  EXPECT_EQ("DIRECT", ToPacString());
+  EXPECT_EQ("DIRECT", ToDebugString());
 }
 
 TEST_F(ProxyConfigWebSocketTest, ObeysLocalBypass) {
   ParseFromString("http=proxy:3128 ; https=sslproxy:3128");
   rules_.bypass_rules.AddRuleFromString("<local>");
   Apply(GURL("ws://localhost/feed"));
-  EXPECT_EQ("DIRECT", ToPacString());
+  EXPECT_EQ("DIRECT", ToDebugString());
 }
 
 }  // namespace

net/proxy_resolution/proxy_info.cc

@@ -57,6 +57,14 @@ void ProxyInfo::OverrideProxyList(const ProxyList& proxy_list) {
   proxy_list_ = proxy_list;
 }
 
+bool ProxyInfo::ContainsMultiProxyChain() const {
+  auto& proxy_chains = proxy_list_.AllChains();
+  return std::any_of(proxy_chains.begin(), proxy_chains.end(),
+                     [](const ProxyChain& proxy_chain) {
+                       return proxy_chain.is_multi_proxy();
+                     });
+}
+
 std::string ProxyInfo::ToPacString() const {
   return proxy_list_.ToPacString();
 }
@@ -127,6 +135,10 @@ bool ProxyInfo::is_socks() const {
   return proxy_chain().GetProxyServer(/*chain_index=*/0).is_socks();
 }
 
+std::string ProxyInfo::ToDebugString() const {
+  return proxy_list_.ToDebugString();
+}
+
 bool ProxyInfo::Fallback(int net_error, const NetLogWithSource& net_log) {
   return proxy_list_.Fallback(&proxy_retry_info_, net_error, net_log);
 }

net/proxy_resolution/proxy_info.h

@@ -76,6 +76,9 @@ class NET_EXPORT ProxyInfo {
     return is_direct() && proxy_list_.size() == 1 && proxy_retry_info_.empty();
   }
 
+  // Returns true if any of the contained ProxyChains are multi-proxy.
+  bool ContainsMultiProxyChain() const;
+
   // Returns true if the first valid proxy server is an https proxy.
   // TODO(https://crbug.com/1491092): Remove this method in favor of checking
   // the corresponding property of the relevant proxy server from the next
@@ -137,6 +140,9 @@ class NET_EXPORT ProxyInfo {
   // See description in ProxyList::ToPacString().
   std::string ToPacString() const;
 
+  // See description in ProxyList::ToDebugString().
+  std::string ToDebugString() const;
+
   // Marks the current proxy as bad. |net_error| should contain the network
   // error encountered when this proxy was tried, if any. If this fallback
   // is not because of a network error, then |OK| should be passed in (eg. for

net/proxy_resolution/proxy_info_unittest.cc

@@ -38,7 +38,7 @@ TEST(ProxyInfoTest, ProxyInfoIsDirectOnly) {
   EXPECT_FALSE(info.is_direct());
   EXPECT_FALSE(info.is_direct_only());
   EXPECT_EQ(2u, info.proxy_list().size());
-  EXPECT_EQ("PROXY myproxy:80;DIRECT", info.proxy_list().ToPacString());
+  EXPECT_EQ("PROXY myproxy:80;DIRECT", info.proxy_list().ToDebugString());
   // After falling back to direct, we shouldn't consider it DIRECT only.
   EXPECT_TRUE(info.Fallback(ERR_PROXY_CONNECTION_FAILED, NetLogWithSource()));
   EXPECT_TRUE(info.is_direct());
@@ -52,10 +52,10 @@ TEST(ProxyInfoTest, UseVsOverrideProxyList) {
   ProxyList proxy_list;
   proxy_list.Set("http://foo.com");
   info.OverrideProxyList(proxy_list);
-  EXPECT_EQ("PROXY foo.com:80", info.proxy_list().ToPacString());
+  EXPECT_EQ("PROXY foo.com:80", info.proxy_list().ToDebugString());
   proxy_list.Set("http://bar.com");
   info.UseProxyList(proxy_list);
-  EXPECT_EQ("PROXY bar.com:80", info.proxy_list().ToPacString());
+  EXPECT_EQ("PROXY bar.com:80", info.proxy_list().ToDebugString());
 }
 
 TEST(ProxyInfoTest, IsForIpProtection) {
@@ -72,7 +72,7 @@ TEST(ProxyListTest, UseProxyChain) {
   ProxyChain proxy_chain =
       ProxyChain::FromSchemeHostAndPort(ProxyServer::SCHEME_HTTP, "foo", 80);
   info.UseProxyChain(proxy_chain);
-  EXPECT_EQ("PROXY foo:80", info.proxy_list().ToPacString());
+  EXPECT_EQ("PROXY foo:80", info.proxy_list().ToDebugString());
 }
 
 }  // namespace net

net/proxy_resolution/proxy_list.cc

@@ -154,7 +154,6 @@ void ProxyList::SetFromPacString(const std::string& pac_string) {
   Clear();
   base::StringTokenizer entry_tok(pac_string, ";");
   while (entry_tok.GetNext()) {
-    // TODO(crbug.com/1491092): Parse multi-proxy chains.
     ProxyServer proxy_server =
         PacResultElementToProxyServer(entry_tok.token_piece());
     if (proxy_server.is_valid()) {
@@ -170,14 +169,25 @@ void ProxyList::SetFromPacString(const std::string& pac_string) {
 }
 
 std::string ProxyList::ToPacString() const {
+  std::string proxy_list;
+  auto iter = proxy_chains_.begin();
+  for (; iter != proxy_chains_.end(); ++iter) {
+    if (!proxy_list.empty()) {
+      proxy_list += ";";
+    }
+    CHECK(!iter->is_multi_proxy());
+    proxy_list += ProxyServerToPacResultElement(iter->proxy_server());
+  }
+  return proxy_list.empty() ? std::string() : proxy_list;
+}
+
+std::string ProxyList::ToDebugString() const {
   std::string proxy_list;
   auto iter = proxy_chains_.begin();
   for (; iter != proxy_chains_.end(); ++iter) {
     if (!proxy_list.empty())
       proxy_list += ";";
     if (iter->is_multi_proxy()) {
-      // TODO(crbug.com/1491092): Figure out how to represent a multi-proxy
-      // chain.
       proxy_list += iter->ToDebugString();
     } else {
       proxy_list += ProxyServerToPacResultElement(iter->proxy_server());

net/proxy_resolution/proxy_list.h

@@ -106,9 +106,16 @@ class NET_EXPORT_PRIVATE ProxyList {
   void SetFromPacString(const std::string& pac_string);
 
   // Returns a PAC-style semicolon-separated list of valid proxy servers.
-  // For example: "PROXY xxx.xxx.xxx.xxx:xx; SOCKS yyy.yyy.yyy:yy".
+  // For example: "PROXY xxx.xxx.xxx.xxx:xx; SOCKS yyy.yyy.yyy:yy". This is
+  // only valid if the list contains no multi-proxy chains, as those cannot
+  // be represented in PAC syntax.
   std::string ToPacString() const;
 
+  // Returns a semicolon-separated list of proxy chain debug representations.
+  // For single-proxy chains, this is just the PAC representation of the proxy;
+  // otherwise the chain is displayed in `[..]`.
+  std::string ToDebugString() const;
+
   // Returns a serialized value for the list.
   base::Value ToValue() const;
 

net/proxy_resolution/proxy_list_unittest.cc

@@ -25,7 +25,7 @@ namespace {
 TEST(ProxyListTest, SetFromPacString) {
   const struct {
     const char* pac_input;
-    const char* pac_output;
+    const char* debug_output;
   } tests[] = {
     // Valid inputs:
     {  "PROXY foopy:10",
@@ -68,7 +68,7 @@ TEST(ProxyListTest, SetFromPacString) {
   for (const auto& test : tests) {
     ProxyList list;
     list.SetFromPacString(test.pac_input);
-    EXPECT_EQ(test.pac_output, list.ToPacString());
+    EXPECT_EQ(test.debug_output, list.ToDebugString());
     EXPECT_FALSE(list.IsEmpty());
   }
 }
@@ -77,7 +77,7 @@ TEST(ProxyListTest, RemoveProxiesWithoutScheme) {
   const struct {
     const char* pac_input;
     int filter;
-    const char* filtered_pac_output;
+    const char* filtered_debug_output;
   } tests[] = {
     {  "PROXY foopy:10 ; SOCKS5 foopy2 ; SOCKS foopy11 ; PROXY foopy3 ; DIRECT",
        // Remove anything that isn't HTTP or DIRECT.
@@ -95,7 +95,7 @@ TEST(ProxyListTest, RemoveProxiesWithoutScheme) {
     ProxyList list;
     list.SetFromPacString(test.pac_input);
     list.RemoveProxiesWithoutScheme(test.filter);
-    EXPECT_EQ(test.filtered_pac_output, list.ToPacString());
+    EXPECT_EQ(test.filtered_debug_output, list.ToDebugString());
   }
 }
 
@@ -146,7 +146,7 @@ TEST(ProxyListTest, DeprioritizeBadProxyChains) {
     ProxyRetryInfoMap retry_info_map;
     list.DeprioritizeBadProxyChains(retry_info_map);
     EXPECT_EQ("PROXY foopy1:80;PROXY foopy2:80;PROXY foopy3:80",
-              list.ToPacString());
+              list.ToDebugString());
   }
 
   // Call DeprioritizeBadProxyChains with 2 of the three chains marked as bad.
@@ -167,7 +167,7 @@ TEST(ProxyListTest, DeprioritizeBadProxyChains) {
     list.DeprioritizeBadProxyChains(retry_info_map);
 
     EXPECT_EQ("PROXY foopy2:80;PROXY foopy1:80;PROXY foopy3:80",
-              list.ToPacString());
+              list.ToDebugString());
   }
 
   // Call DeprioritizeBadProxyChains where ALL of the chains are marked as bad.
@@ -187,7 +187,7 @@ TEST(ProxyListTest, DeprioritizeBadProxyChains) {
     list.DeprioritizeBadProxyChains(retry_info_map);
 
     EXPECT_EQ("PROXY foopy1:80;PROXY foopy2:80;PROXY foopy3:80",
-              list.ToPacString());
+              list.ToDebugString());
   }
 
   // Call DeprioritizeBadProxyChains with 2 of the three chains marked as bad.
@@ -211,8 +211,7 @@ TEST(ProxyListTest, DeprioritizeBadProxyChains) {
 
     list.DeprioritizeBadProxyChains(retry_info_map);
 
-    EXPECT_EQ("PROXY foopy2:80;PROXY foopy1:80",
-              list.ToPacString());
+    EXPECT_EQ("PROXY foopy2:80;PROXY foopy1:80", list.ToDebugString());
   }
 }
 
@@ -374,6 +373,22 @@ TEST(ProxyListTest, UpdateRetryInfoOnFallback) {
 }
 
 TEST(ProxyListTest, ToPacString) {
+  ProxyList list;
+  list.AddProxyChain(ProxyChain::FromSchemeHostAndPort(
+      ProxyServer::Scheme::SCHEME_HTTPS, "foo", 443));
+  EXPECT_EQ(list.ToPacString(), "HTTPS foo:443");
+
+  // ToPacString should fail for proxy chains.
+  list.AddProxyChain(ProxyChain({
+      ProxyServer::FromSchemeHostAndPort(ProxyServer::Scheme::SCHEME_HTTPS,
+                                         "foo-a", 443),
+      ProxyServer::FromSchemeHostAndPort(ProxyServer::Scheme::SCHEME_HTTPS,
+                                         "foo-b", 443),
+  }));
+  EXPECT_DEATH_IF_SUPPORTED(list.ToPacString(), "");
+}
+
+TEST(ProxyListTest, ToDebugString) {
   ProxyList list;
   list.AddProxyChain(ProxyChain::FromSchemeHostAndPort(
       ProxyServer::Scheme::SCHEME_HTTPS, "foo", 443));
@@ -384,9 +399,7 @@ TEST(ProxyListTest, ToPacString) {
                                          "foo-b", 443),
   }));
 
-  // TODO(crbug.com/1491092): This is not actually valid PAC syntax. We should
-  // decide if we're OK with that.
-  EXPECT_EQ(list.ToPacString(),
+  EXPECT_EQ(list.ToDebugString(),
             "HTTPS foo:443;[https://foo-a:443, https://foo-b:443]");
 }
 

net/websockets/websocket_end_to_end_test.cc

@@ -533,7 +533,7 @@ TEST_F(WebSocketEndToEndTest, MAYBE_ProxyPacUsed) {
   const auto& info = proxy_delegate_->resolved_proxy_info();
   EXPECT_EQ(ws_url, info.url);
   EXPECT_TRUE(info.proxy_info.is_http());
-  EXPECT_EQ(info.proxy_info.ToPacString(),
+  EXPECT_EQ(info.proxy_info.ToDebugString(),
             base::StrCat({"PROXY ", proxy_server.host_port_pair().ToString()}));
 }
 

services/network/network_context_unittest.cc

@@ -2968,8 +2968,8 @@ TEST_F(NetworkContextTest, ProxyConfig) {
         network_context.get());
     http_proxy_lookup_client.WaitForResult();
     ASSERT_TRUE(http_proxy_lookup_client.proxy_info());
-    EXPECT_EQ(initial_proxy_config_set.http_proxy_info.ToPacString(),
-              http_proxy_lookup_client.proxy_info()->ToPacString());
+    EXPECT_EQ(initial_proxy_config_set.http_proxy_info.ToDebugString(),
+              http_proxy_lookup_client.proxy_info()->ToDebugString());
 
     TestProxyLookupClient ftp_proxy_lookup_client;
     ftp_proxy_lookup_client.StartLookUpProxyForURL(
@@ -2977,8 +2977,8 @@ TEST_F(NetworkContextTest, ProxyConfig) {
         network_context.get());
     ftp_proxy_lookup_client.WaitForResult();
     ASSERT_TRUE(ftp_proxy_lookup_client.proxy_info());
-    EXPECT_EQ(initial_proxy_config_set.ftp_proxy_info.ToPacString(),
-              ftp_proxy_lookup_client.proxy_info()->ToPacString());
+    EXPECT_EQ(initial_proxy_config_set.ftp_proxy_info.ToDebugString(),
+              ftp_proxy_lookup_client.proxy_info()->ToDebugString());
 
     EXPECT_TRUE(proxy_resolution_service->config());
     EXPECT_TRUE(proxy_resolution_service->config()->value().Equals(
@@ -2998,8 +2998,8 @@ TEST_F(NetworkContextTest, ProxyConfig) {
           network_context.get());
       http_proxy_lookup_client2.WaitForResult();
       ASSERT_TRUE(http_proxy_lookup_client2.proxy_info());
-      EXPECT_EQ(proxy_config_set.http_proxy_info.ToPacString(),
-                http_proxy_lookup_client2.proxy_info()->ToPacString());
+      EXPECT_EQ(proxy_config_set.http_proxy_info.ToDebugString(),
+                http_proxy_lookup_client2.proxy_info()->ToDebugString());
 
       TestProxyLookupClient ftp_proxy_lookup_client2;
       ftp_proxy_lookup_client2.StartLookUpProxyForURL(
@@ -3007,8 +3007,8 @@ TEST_F(NetworkContextTest, ProxyConfig) {
           network_context.get());
       ftp_proxy_lookup_client2.WaitForResult();
       ASSERT_TRUE(ftp_proxy_lookup_client2.proxy_info());
-      EXPECT_EQ(proxy_config_set.ftp_proxy_info.ToPacString(),
-                ftp_proxy_lookup_client2.proxy_info()->ToPacString());
+      EXPECT_EQ(proxy_config_set.ftp_proxy_info.ToDebugString(),
+                ftp_proxy_lookup_client2.proxy_info()->ToDebugString());
 
       EXPECT_TRUE(proxy_resolution_service->config());
       EXPECT_TRUE(proxy_resolution_service->config()->value().Equals(
@@ -3085,11 +3085,11 @@ TEST_F(NetworkContextTest, NoInitialProxyConfig) {
   http_proxy_lookup_client.WaitForResult();
   ASSERT_TRUE(http_proxy_lookup_client.proxy_info());
   EXPECT_EQ("PROXY foopy:80",
-            http_proxy_lookup_client.proxy_info()->ToPacString());
+            http_proxy_lookup_client.proxy_info()->ToDebugString());
 
   ftp_proxy_lookup_client.WaitForResult();
   ASSERT_TRUE(ftp_proxy_lookup_client.proxy_info());
-  EXPECT_EQ("DIRECT", ftp_proxy_lookup_client.proxy_info()->ToPacString());
+  EXPECT_EQ("DIRECT", ftp_proxy_lookup_client.proxy_info()->ToDebugString());
 
   EXPECT_EQ(0u, network_context->pending_proxy_lookup_requests_for_testing());
 }

services/network/network_service_proxy_delegate.cc

@@ -184,7 +184,7 @@ void NetworkServiceProxyDelegate::OnResolveProxy(
 
     if (VLOG_IS_ON(3)) {
       dvlog(base::StrCat({"setting proxy list (before deprioritization) to ",
-                          proxy_list.ToPacString()}));
+                          proxy_list.ToDebugString()}));
     }
     result->set_is_for_ip_protection(true);
     result->OverrideProxyList(

services/network/network_service_proxy_delegate_unittest.cc

@@ -721,7 +721,7 @@ TEST_F(NetworkServiceProxyDelegateTest,
   expected_proxy_list.AddProxyServer(
       net::PacResultElementToProxyServer("PROXY weird"));
   EXPECT_TRUE(result.proxy_list().Equals(expected_proxy_list))
-      << "Got: " << result.proxy_list().ToPacString();
+      << "Got: " << result.proxy_list().ToDebugString();
   EXPECT_TRUE(result.is_for_ip_protection());
 }
 
@@ -758,7 +758,7 @@ TEST_F(NetworkServiceProxyDelegateTest,
   // Proxy server is not added.
   expected_proxy_list.AddProxyServer(net::ProxyServer::Direct());
   EXPECT_TRUE(result.proxy_list().Equals(expected_proxy_list))
-      << "Got: " << result.proxy_list().ToPacString();
+      << "Got: " << result.proxy_list().ToDebugString();
   EXPECT_TRUE(result.is_for_ip_protection());
 }
 

services/network/proxy_resolver_factory_mojo.cc

@@ -278,7 +278,7 @@ void ProxyResolverMojo::Job::ReportResult(int32_t error,
 
   if (error == net::OK) {
     *results_ = proxy_info;
-    DVLOG(1) << "Servers: " << results_->ToPacString();
+    DVLOG(1) << "Servers: " << results_->ToDebugString();
   }
 
   CompleteRequest(error);

services/network/proxy_resolver_factory_mojo_unittest.cc

@@ -785,7 +785,7 @@ TEST_F(ProxyResolverFactoryMojoTest, GetProxyForURL) {
   EXPECT_THAT(request->Resolve(), IsError(net::ERR_IO_PENDING));
   EXPECT_THAT(request->WaitForResult(), IsOk());
 
-  EXPECT_EQ("DIRECT", request->results().ToPacString());
+  EXPECT_EQ("DIRECT", request->results().ToDebugString());
   CheckCapturedNetLogEntries(url.spec(),
                              net_log_observer_.GetEntriesForSource(
                                  request->net_log_with_source().source()));
@@ -803,7 +803,7 @@ TEST_F(ProxyResolverFactoryMojoTest, GetProxyForURL_MultipleResults) {
   EXPECT_THAT(request->Resolve(), IsError(net::ERR_IO_PENDING));
   EXPECT_THAT(request->WaitForResult(), IsOk());
 
-  EXPECT_EQ(kPacString, request->results().ToPacString());
+  EXPECT_EQ(kPacString, request->results().ToDebugString());
 }
 
 TEST_F(ProxyResolverFactoryMojoTest, GetProxyForURL_Error) {
@@ -849,8 +849,8 @@ TEST_F(ProxyResolverFactoryMojoTest, GetProxyForURL_MultipleRequests) {
   EXPECT_THAT(request1->WaitForResult(), IsOk());
   EXPECT_THAT(request2->WaitForResult(), IsOk());
 
-  EXPECT_EQ("DIRECT", request1->results().ToPacString());
-  EXPECT_EQ("HTTPS foo:443", request2->results().ToPacString());
+  EXPECT_EQ("DIRECT", request1->results().ToDebugString());
+  EXPECT_EQ("HTTPS foo:443", request2->results().ToDebugString());
 }
 
 TEST_F(ProxyResolverFactoryMojoTest, GetProxyForURL_Disconnect) {

services/network/proxy_service_mojo_unittest.cc

@@ -161,7 +161,7 @@ TEST_F(ProxyServiceMojoTest, Basic) {
   fetcher_->NotifyFetchCompletion(net::OK, kSimplePacScript);
 
   EXPECT_THAT(callback.WaitForResult(), IsOk());
-  EXPECT_EQ("PROXY foo:1234", info.ToPacString());
+  EXPECT_EQ("PROXY foo:1234", info.ToDebugString());
   EXPECT_EQ(0u, mock_host_resolver_.num_resolve());
   proxy_resolution_service_.reset();
 }
@@ -184,7 +184,7 @@ TEST_F(ProxyServiceMojoTest, DnsResolution) {
   fetcher_->NotifyFetchCompletion(net::OK, kDnsResolvePacScript);
 
   EXPECT_THAT(callback.WaitForResult(), IsOk());
-  EXPECT_EQ("QUIC bar:4321", info.ToPacString());
+  EXPECT_EQ("QUIC bar:4321", info.ToDebugString());
   EXPECT_EQ(1u, mock_host_resolver_.num_resolve());
   proxy_resolution_service_.reset();
 }
@@ -211,7 +211,7 @@ TEST_F(ProxyServiceMojoTest, Error) {
       TestNetworkDelegate::PAC_SCRIPT_ERROR);
 
   EXPECT_THAT(callback.WaitForResult(), IsOk());
-  EXPECT_EQ("DIRECT", info.ToPacString());
+  EXPECT_EQ("DIRECT", info.ToDebugString());
   EXPECT_EQ(0u, mock_host_resolver_.num_resolve());
   CheckCapturedNetLogEntries(
       net_log_observer_.GetEntriesForSource(net_log_with_source.source()));
@@ -238,7 +238,7 @@ TEST_F(ProxyServiceMojoTest, ErrorOnInitialization) {
       TestNetworkDelegate::PAC_SCRIPT_ERROR);
 
   EXPECT_THAT(callback.WaitForResult(), IsOk());
-  EXPECT_EQ("DIRECT", info.ToPacString());
+  EXPECT_EQ("DIRECT", info.ToDebugString());
   EXPECT_EQ(0u, mock_host_resolver_.num_resolve());
 
   CheckCapturedNetLogEntries(net_log_observer_.GetEntries());

services/proxy_resolver_win/windows_system_proxy_resolver_impl_unittest.cc

@@ -428,7 +428,7 @@ class WindowsSystemProxyResolverImplTest : public testing::Test {
     std::wstring proxy;
     if (!proxy_config.proxy_rules().single_proxies.IsEmpty()) {
       proxy = base::UTF8ToWide(
-          proxy_config.proxy_rules().single_proxies.ToPacString());
+          proxy_config.proxy_rules().single_proxies.ToDebugString());
     }
 
     std::wstring proxy_bypass;