Added tests for ProxyList::UpdateRetryInfoOnFallback

This CL addresses comments made by eroman@ in
https://codereview.chromium.org/98373005/. In particular,
additional test coverage is included.

BUG=325333

Review URL: https://codereview.chromium.org/117333004

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@241789 0039d316-1c4b-4281-b951-d872f2087c98

4 files changed, 79 insertions, 21 deletions

diff --git a/net/http/http_response_headers_unittest.cc b/net/http/http_response_headers_unittest.cc
index 4be7478..825f5c4 100644
--- a/net/http/http_response_headers_unittest.cc
+++ b/net/http/http_response_headers_unittest.cc
@@ -2013,6 +2013,22 @@ TEST(HttpResponseHeadersTest, GetProxyBypassInfo) {
       86400,
       false,
     },
+    { "HTTP/1.1 200 OK\n"
+      "connection: proxy-bypass\n"
+      "Chrome-Proxy: block=-1\n"
+      "Content-Length: 999\n",
+      false,
+      0,
+      false,
+    },
+    { "HTTP/1.1 200 OK\n"
+      "connection: proxy-bypass\n"
+      "Chrome-Proxy: block=99999999999999999999\n"
+      "Content-Length: 999\n",
+      false,
+      0,
+      false,
+    },
   };
   for (size_t i = 0; i < ARRAYSIZE_UNSAFE(tests); ++i) {
     std::string headers(tests[i].headers);
diff --git a/net/proxy/proxy_list.cc b/net/proxy/proxy_list.cc
index 5328402..4492b7d 100644
--- a/net/proxy/proxy_list.cc
+++ b/net/proxy/proxy_list.cc
@@ -195,9 +195,10 @@ bool ProxyList::Fallback(ProxyRetryInfoMap* proxy_retry_info,
 
 void ProxyList::AddProxyToRetryList(ProxyRetryInfoMap* proxy_retry_info,
                                     base::TimeDelta retry_delay,
-                                    const std::string& proxy_key,
+                                    const ProxyServer& proxy_to_retry,
                                     const BoundNetLog& net_log) const {
   // Mark this proxy as bad.
+  std::string proxy_key = proxy_to_retry.ToURI();
   ProxyRetryInfoMap::iterator iter = proxy_retry_info->find(proxy_key);
   if (iter != proxy_retry_info->end()) {
     // TODO(nsylvain): This is not the first time we get this. We should
@@ -236,21 +237,13 @@ void ProxyList::UpdateRetryInfoOnFallback(
   }
 
   if (!proxies_[0].is_direct()) {
-    std::string key = proxies_[0].ToURI();
-    AddProxyToRetryList(proxy_retry_info, retry_delay, key, net_log);
+    AddProxyToRetryList(proxy_retry_info, retry_delay, proxies_[0], net_log);
 
-    // If additional proxies to bypass are specified, add these to the retry
-    // map as well.
+    // If an additional proxy to bypass is specified, add it to the retry map
+    // as well.
     if (another_proxy_to_bypass.is_valid()) {
-      // Start at index 1 because index 0 is already handled above.
-      for (size_t j = 1; j < proxies_.size(); ++j) {
-        if (proxies_[j].is_direct())
-          break;
-        if (another_proxy_to_bypass == proxies_[j]) {
-          key = proxies_[j].ToURI();
-          AddProxyToRetryList(proxy_retry_info, retry_delay, key, net_log);
-        }
-      }
+      AddProxyToRetryList(proxy_retry_info, retry_delay,
+                          another_proxy_to_bypass, net_log);
     }
   }
 }
diff --git a/net/proxy/proxy_list.h b/net/proxy/proxy_list.h
index e8df0ba..48d3795 100644
--- a/net/proxy/proxy_list.h
+++ b/net/proxy/proxy_list.h
@@ -101,15 +101,11 @@ class NET_EXPORT_PRIVATE ProxyList {
       const BoundNetLog& net_log) const;
 
  private:
-  // Updates |proxy_retry_info| to indicate that the proxy in |proxies_| with
-  // the URI of |proxy_key| is bad. The |proxy_key| must start with the scheme
-  // (only if https) followed by the host and  then an explicit port. For
-  // example, if the proxy origin is https://proxy.chromium.org:443/ the key is
-  // https://proxy.chrome.org:443 whereas if the origin is
-  // http://proxy.chrome.org/, the key is proxy.chrome.org:80.
+  // Updates |proxy_retry_info| to indicate that the |proxy_to_retry| in
+  // |proxies_| is bad.
   void AddProxyToRetryList(ProxyRetryInfoMap* proxy_retry_info,
                            base::TimeDelta retry_delay,
-                           const std::string& proxy_key,
+                           const ProxyServer& proxy_to_retry,
                            const BoundNetLog& net_log) const;
 
   // List of proxies.
diff --git a/net/proxy/proxy_list_unittest.cc b/net/proxy/proxy_list_unittest.cc
index 470a900..33d3dab 100644
--- a/net/proxy/proxy_list_unittest.cc
+++ b/net/proxy/proxy_list_unittest.cc
@@ -4,6 +4,8 @@
 
 #include "net/proxy/proxy_list.h"
 
+#include "net/base/net_log.h"
+#include "net/proxy/proxy_retry_info.h"
 #include "net/proxy/proxy_server.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -178,6 +180,57 @@ TEST(ProxyListTest, DeprioritizeBadProxies) {
   }
 }
 
+TEST(ProxyListTest, UpdateRetryInfoOnFallback) {
+  ProxyRetryInfo proxy_retry_info;
+  // Retrying should put the first proxy on the retry list.
+  {
+    ProxyList list;
+    ProxyRetryInfoMap retry_info_map;
+    BoundNetLog net_log;
+    list.SetFromPacString("PROXY foopy1:80;PROXY foopy2:80;PROXY foopy3:80");
+    list.UpdateRetryInfoOnFallback(&retry_info_map,
+                                   base::TimeDelta::FromSeconds(60),
+                                   ProxyServer(),
+                                   net_log);
+    EXPECT_TRUE(retry_info_map.end() != retry_info_map.find("foopy1:80"));
+    EXPECT_TRUE(retry_info_map.end() == retry_info_map.find("foopy2:80"));
+    EXPECT_TRUE(retry_info_map.end() == retry_info_map.find("foopy3:80"));
+  }
+  // Including another bad proxy should put both the first and the specified
+  // proxy on the retry list.
+  {
+    ProxyList list;
+    ProxyRetryInfoMap retry_info_map;
+    BoundNetLog net_log;
+    ProxyServer proxy_server = ProxyServer::FromURI("foopy3:80",
+                                                    ProxyServer::SCHEME_HTTP);
+    list.SetFromPacString("PROXY foopy1:80;PROXY foopy2:80;PROXY foopy3:80");
+    list.UpdateRetryInfoOnFallback(&retry_info_map,
+                                   base::TimeDelta::FromSeconds(60),
+                                   proxy_server,
+                                   net_log);
+    EXPECT_TRUE(retry_info_map.end() != retry_info_map.find("foopy1:80"));
+    EXPECT_TRUE(retry_info_map.end() == retry_info_map.find("foopy2:80"));
+    EXPECT_TRUE(retry_info_map.end() != retry_info_map.find("foopy3:80"));
+  }
+  // If the first proxy is DIRECT, nothing is added to the retry list, even
+  // if another bad proxy is specified.
+  {
+    ProxyList list;
+    ProxyRetryInfoMap retry_info_map;
+    BoundNetLog net_log;
+    ProxyServer proxy_server = ProxyServer::FromURI("foopy2:80",
+                                                    ProxyServer::SCHEME_HTTP);
+    list.SetFromPacString("DIRECT;PROXY foopy2:80;PROXY foopy3:80");
+    list.UpdateRetryInfoOnFallback(&retry_info_map,
+                                   base::TimeDelta::FromSeconds(60),
+                                   proxy_server,
+                                   net_log);
+    EXPECT_TRUE(retry_info_map.end() == retry_info_map.find("foopy2:80"));
+    EXPECT_TRUE(retry_info_map.end() == retry_info_map.find("foopy3:80"));
+  }
+}
+
 }  // namesapce
 
 }  // namespace net