Retry proxies which were cached as bad before giving up.

This morphs ProxyList::RemoveBadProxies() into ProxyList::DeprioritizeBadProxies(), such that "bad proxies" are moved to the end of the fallback list rather than removed alltogether.

BUG=31983
TEST=ProxyListTest.DeprioritizeBadProxies

Review URL: http://codereview.chromium.org/542029

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

8 files changed, 101 insertions, 26 deletions

diff --git a/net/base/net_error_list.h b/net/base/net_error_list.h
index 9502018..3bc2c3e 100644
--- a/net/base/net_error_list.h
+++ b/net/base/net_error_list.h
@@ -289,7 +289,7 @@ NET_ERROR(UNRECOGNIZED_FTP_DIRECTORY_LISTING_FORMAT, -334)
 NET_ERROR(INVALID_FLIP_STREAM, -335)
 
 // There are no supported proxies in the provided list.
-NET_ERROR(EMPTY_PROXY_LIST, -336)
+NET_ERROR(NO_SUPPORTED_PROXIES, -336)
 
 // The cache does not have the requested entry.
 NET_ERROR(CACHE_MISS, -400)
diff --git a/net/http/http_network_transaction.cc b/net/http/http_network_transaction.cc
index ea61c79..d3fed93 100644
--- a/net/http/http_network_transaction.cc
+++ b/net/http/http_network_transaction.cc
@@ -611,14 +611,9 @@ int HttpNetworkTransaction::DoResolveProxyComplete(int result) {
   // http://www.chromium.org/developers/design-documents/proxy-settings-fallback
 
   if (proxy_info_.is_empty()) {
-    // No proxies/direct to choose from. This can happen when:
-    //   a. We don't support any of the proxies in the returned list.
-    //   b. The proxy service returned us an empty list.
-    //      1. this can happen if all the proxies were marked as bad already.
-    //
-    // TODO(eroman): in case (b.1) it would be better to just try the bad
-    // proxies again rather than failing without having tried anything!
-    return ERR_EMPTY_PROXY_LIST;
+    // No proxies/direct to choose from. This happens when we don't support any
+    // of the proxies in the returned list.
+    return ERR_NO_SUPPORTED_PROXIES;
   }
 
   if (result != OK) {
diff --git a/net/proxy/proxy_info.h b/net/proxy/proxy_info.h
index d65348e..b18feb8 100644
--- a/net/proxy/proxy_info.h
+++ b/net/proxy/proxy_info.h
@@ -68,9 +68,10 @@ class ProxyInfo {
     return proxy_list_.Fallback(proxy_retry_info);
   }
 
-  // Remove all proxies known to be bad from the proxy list.
-  void RemoveBadProxies(const ProxyRetryInfoMap& proxy_retry_info) {
-    proxy_list_.RemoveBadProxies(proxy_retry_info);
+  // De-prioritizes the proxies that we have cached as not working, by moving
+  // them to the end of the proxy list.
+  void DeprioritizeBadProxies(const ProxyRetryInfoMap& proxy_retry_info) {
+    proxy_list_.DeprioritizeBadProxies(proxy_retry_info);
   }
 
   // Delete any entry which doesn't have one of the specified proxy schemes.
diff --git a/net/proxy/proxy_list.cc b/net/proxy/proxy_list.cc
index 924fd15..b119b13 100644
--- a/net/proxy/proxy_list.cc
+++ b/net/proxy/proxy_list.cc
@@ -31,8 +31,14 @@ void ProxyList::SetSingleProxyServer(const ProxyServer& proxy_server) {
     proxies_.push_back(proxy_server);
 }
 
-void ProxyList::RemoveBadProxies(const ProxyRetryInfoMap& proxy_retry_info) {
-  std::vector<ProxyServer> new_proxy_list;
+void ProxyList::DeprioritizeBadProxies(
+    const ProxyRetryInfoMap& proxy_retry_info) {
+  // Partition the proxy list in two:
+  //   (1) the known bad proxies
+  //   (2) everything else
+  std::vector<ProxyServer> good_proxies;
+  std::vector<ProxyServer> bad_proxies;
+
   std::vector<ProxyServer>::const_iterator iter = proxies_.begin();
   for (; iter != proxies_.end(); ++iter) {
     ProxyRetryInfoMap::const_iterator bad_proxy =
@@ -41,13 +47,16 @@ void ProxyList::RemoveBadProxies(const ProxyRetryInfoMap& proxy_retry_info) {
       // This proxy is bad. Check if it's time to retry.
       if (bad_proxy->second.bad_until >= TimeTicks::Now()) {
         // still invalid.
+        bad_proxies.push_back(*iter);
         continue;
       }
     }
-    new_proxy_list.push_back(*iter);
+    good_proxies.push_back(*iter);
   }
 
-  proxies_ = new_proxy_list;
+  // "proxies_ = good_proxies + bad_proxies"
+  proxies_.swap(good_proxies);
+  proxies_.insert(proxies_.end(), bad_proxies.begin(), bad_proxies.end());
 }
 
 void ProxyList::RemoveProxiesWithoutScheme(int scheme_bit_field) {
diff --git a/net/proxy/proxy_list.h b/net/proxy/proxy_list.h
index f05d4cf..1afd1d9 100644
--- a/net/proxy/proxy_list.h
+++ b/net/proxy/proxy_list.h
@@ -25,8 +25,9 @@ class ProxyList {
   // Set the proxy list to a single entry, |proxy_server|.
   void SetSingleProxyServer(const ProxyServer& proxy_server);
 
-  // Remove all proxies known to be bad from the proxy list.
-  void RemoveBadProxies(const ProxyRetryInfoMap& proxy_retry_info);
+  // De-prioritizes the proxies that we have cached as not working, by moving
+  // them to the end of the fallback list.
+  void DeprioritizeBadProxies(const ProxyRetryInfoMap& proxy_retry_info);
 
   // Delete any entry which doesn't have one of the specified proxy schemes.
   // |scheme_bit_field| is a bunch of ProxyServer::Scheme bitwise ORed together.
diff --git a/net/proxy/proxy_list_unittest.cc b/net/proxy/proxy_list_unittest.cc
index 401461f..397bb7c 100644
--- a/net/proxy/proxy_list_unittest.cc
+++ b/net/proxy/proxy_list_unittest.cc
@@ -5,6 +5,10 @@
 #include "net/proxy/proxy_list.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+namespace net {
+
+namespace {
+
 // Test parsing from a PAC string.
 TEST(ProxyListTest, SetFromPacString) {
   const struct {
@@ -50,7 +54,7 @@ TEST(ProxyListTest, SetFromPacString) {
   };
 
   for (size_t i = 0; i < ARRAYSIZE_UNSAFE(tests); ++i) {
-    net::ProxyList list;
+    ProxyList list;
     list.SetFromPacString(tests[i].pac_input);
     EXPECT_EQ(tests[i].pac_output, list.ToPacString());
     EXPECT_FALSE(list.IsEmpty());
@@ -65,20 +69,77 @@ TEST(ProxyListTest, RemoveProxiesWithoutScheme) {
   } tests[] = {
     {  "PROXY foopy:10 ; SOCKS5 foopy2 ; SOCKS foopy11 ; PROXY foopy3 ; DIRECT",
        // Remove anything that isn't HTTP or DIRECT.
-       net::ProxyServer::SCHEME_DIRECT | net::ProxyServer::SCHEME_HTTP,
+       ProxyServer::SCHEME_DIRECT | ProxyServer::SCHEME_HTTP,
        "PROXY foopy:10;PROXY foopy3:80;DIRECT",
     },
     {  "PROXY foopy:10 ; SOCKS5 foopy2",
        // Remove anything that isn't HTTP or SOCKS5.
-       net::ProxyServer::SCHEME_DIRECT | net::ProxyServer::SCHEME_SOCKS4,
+       ProxyServer::SCHEME_DIRECT | ProxyServer::SCHEME_SOCKS4,
        "",
     },
   };
 
   for (size_t i = 0; i < ARRAYSIZE_UNSAFE(tests); ++i) {
-    net::ProxyList list;
+    ProxyList list;
     list.SetFromPacString(tests[i].pac_input);
     list.RemoveProxiesWithoutScheme(tests[i].filter);
     EXPECT_EQ(tests[i].filtered_pac_output, list.ToPacString());
   }
 }
+
+TEST(ProxyListTest, DeprioritizeBadProxies) {
+  // Retry info that marks a proxy as being bad for a *very* long time (to avoid
+  // the test depending on the current time.)
+  ProxyRetryInfo proxy_retry_info;
+  proxy_retry_info.bad_until =
+      base::TimeTicks::Now() + base::TimeDelta::FromDays(1);
+
+  // Call DeprioritizeBadProxies with an empty map -- should have no effect.
+  {
+    ProxyList list;
+    list.SetFromPacString("PROXY foopy1:80;PROXY foopy2:80;PROXY foopy3:80");
+
+    ProxyRetryInfoMap retry_info_map;
+    list.DeprioritizeBadProxies(retry_info_map);
+    EXPECT_EQ("PROXY foopy1:80;PROXY foopy2:80;PROXY foopy3:80",
+              list.ToPacString());
+  }
+
+  // Call DeprioritizeBadProxies with 2 of the three proxies marked as bad.
+  // These proxies should be retried last.
+  {
+    ProxyList list;
+    list.SetFromPacString("PROXY foopy1:80;PROXY foopy2:80;PROXY foopy3:80");
+
+    ProxyRetryInfoMap retry_info_map;
+    retry_info_map["foopy1:80"] = proxy_retry_info;
+    retry_info_map["foopy3:80"] = proxy_retry_info;
+    retry_info_map["socks5://localhost:1080"] = proxy_retry_info;
+
+    list.DeprioritizeBadProxies(retry_info_map);
+
+    EXPECT_EQ("PROXY foopy2:80;PROXY foopy1:80;PROXY foopy3:80",
+              list.ToPacString());
+  }
+
+  // Call DeprioritizeBadProxies where ALL of the proxies are marked as bad.
+  // This should have no effect on the order.
+  {
+    ProxyList list;
+    list.SetFromPacString("PROXY foopy1:80;PROXY foopy2:80;PROXY foopy3:80");
+
+    ProxyRetryInfoMap retry_info_map;
+    retry_info_map["foopy1:80"] = proxy_retry_info;
+    retry_info_map["foopy2:80"] = proxy_retry_info;
+    retry_info_map["foopy3:80"] = proxy_retry_info;
+
+    list.DeprioritizeBadProxies(retry_info_map);
+
+    EXPECT_EQ("PROXY foopy1:80;PROXY foopy2:80;PROXY foopy3:80",
+              list.ToPacString());
+  }
+}
+
+}  // namesapce
+
+}  // namespace net
diff --git a/net/proxy/proxy_service.cc b/net/proxy/proxy_service.cc
index 8160e64..b90d781 100644
--- a/net/proxy/proxy_service.cc
+++ b/net/proxy/proxy_service.cc
@@ -498,7 +498,7 @@ int ProxyService::DidFinishResolvingProxy(ProxyInfo* result,
 
   // Clean up the results list.
   if (result_code == OK)
-    result->RemoveBadProxies(proxy_retry_info_);
+    result->DeprioritizeBadProxies(proxy_retry_info_);
 
   LoadLog::EndEvent(load_log, LoadLog::TYPE_PROXY_SERVICE);
   return result_code;
diff --git a/net/proxy/proxy_service_unittest.cc b/net/proxy/proxy_service_unittest.cc
index f220ce1..0178dbe 100644
--- a/net/proxy/proxy_service_unittest.cc
+++ b/net/proxy/proxy_service_unittest.cc
@@ -404,7 +404,7 @@ TEST(ProxyServiceTest, ProxyFallback) {
   EXPECT_EQ(url, resolver->pending_requests()[0]->url());
 
   // Set the result in proxy resolver -- the second result is already known
-  // to be bad.
+  // to be bad, so we will not try to use it initially.
   resolver->pending_requests()[0]->results()->UseNamedProxy(
       "foopy3:7070;foopy1:8080;foopy2:9090");
   resolver->pending_requests()[0]->CompleteNow(OK);
@@ -419,10 +419,18 @@ TEST(ProxyServiceTest, ProxyFallback) {
   EXPECT_EQ(OK, rv);
   EXPECT_EQ("foopy2:9090", info.proxy_server().ToURI());
 
-  // Fake another error, the last proxy is gone, the list should now be empty,
-  // so there is nothing left to try.
+  // We fake another error. At this point we have tried all of the
+  // proxy servers we thought were valid; next we try the proxy server
+  // that was in our bad proxies map (foopy1:8080).
   TestCompletionCallback callback5;
   rv = service->ReconsiderProxyAfterError(url, &info, &callback5, NULL, NULL);
+  EXPECT_EQ(OK, rv);
+  EXPECT_EQ("foopy1:8080", info.proxy_server().ToURI());
+
+  // Fake another error, the last proxy is gone, the list should now be empty,
+  // so there is nothing left to try.
+  TestCompletionCallback callback6;
+  rv = service->ReconsiderProxyAfterError(url, &info, &callback6, NULL, NULL);
   EXPECT_EQ(ERR_FAILED, rv);
   EXPECT_FALSE(info.is_direct());
   EXPECT_TRUE(info.is_empty());