Receiving Connection: Proxy-Bypass induces proxy fallback.

net/proxy/proxy_list.cc

Index: net/proxy/proxy_list.cc
diff --git a/net/proxy/proxy_list.cc b/net/proxy/proxy_list.cc
index 3a63c6cc0eb705ae13e8c72fc351edad370727ec..2057d50b4327321eafa2e2e0dd42ac63dcbb90f0 100644
--- a/net/proxy/proxy_list.cc
+++ b/net/proxy/proxy_list.cc
@@ -67,6 +67,26 @@ void ProxyList::DeprioritizeBadProxies(
   proxies_.insert(proxies_.end(), bad_proxies.begin(), bad_proxies.end());
 }
 
+bool ProxyList::HasUntriedProxies(
+    const ProxyRetryInfoMap& proxy_retry_info) const {
+  std::vector<ProxyServer>::const_iterator iter = proxies_.begin();
+  for (; iter != proxies_.end(); ++iter) {
+    ProxyRetryInfoMap::const_iterator bad_proxy =
+        proxy_retry_info.find(iter->ToURI());
+    if (bad_proxy != proxy_retry_info.end()) {
+      // This proxy is bad. Check if it's time to retry.
+      if (bad_proxy->second.bad_until >= TimeTicks::Now()) {
+        continue;
+      }
+    }
+    // Either we've found the entry in the retry map and it's expired or we
+    // didn't find a corresponding entry in the retry map. In either case, we
+    // have a proxy to try.
+    return true;

[eroman, 2012/10/12 18:30:58]

Note |proxies_| can also contain entries for "DIRECT" (as it represents a PAC
fallback list).

So it is possible for this function to return true due to hitting a DIRECT
pseudo-proxy.

This isn't "wrong", but it is subtle enough that I would like you to include
tests for it (both in http_network_transaction_unittest, and in
proxy_list_unittest).

The case I am thinking of, is when connecting through DIRECT also returns a
proxy-bypass. I believe your code will work as-is, since
http_network_transaction is explicitly testing for "was_fetched_via_proxy" so it
shouldn't loop.

[Michael Piatek, 2012/10/15 17:10:16]

Done.

+  }
+  return false;
+}
+
 void ProxyList::RemoveProxiesWithoutScheme(int scheme_bit_field) {
   for (std::vector<ProxyServer>::iterator it = proxies_.begin();
        it != proxies_.end(); ) {
@@ -126,8 +146,6 @@ std::string ProxyList::ToPacString() const {
 
 bool ProxyList::Fallback(ProxyRetryInfoMap* proxy_retry_info,
                          const BoundNetLog& net_log) {
-  // Number of minutes to wait before retrying a bad proxy server.
-  const TimeDelta kProxyRetryDelay = TimeDelta::FromMinutes(5);
 
   // TODO(eroman): It would be good if instead of removing failed proxies
   // from the list, we simply annotated them with the error code they failed
@@ -147,6 +165,22 @@ bool ProxyList::Fallback(ProxyRetryInfoMap* proxy_retry_info,
     NOTREACHED();
     return false;
   }
+  UpdateRetryInfoOnFallback(proxy_retry_info, net_log);
+
+  // Remove this proxy from our list.
+  proxies_.erase(proxies_.begin());
+  return !proxies_.empty();
+}
+
+void ProxyList::UpdateRetryInfoOnFallback(
+    ProxyRetryInfoMap* proxy_retry_info, const BoundNetLog& net_log) const {
+  // Number of minutes to wait before retrying a bad proxy server.
+  const TimeDelta kProxyRetryDelay = TimeDelta::FromMinutes(5);
+
+  if (proxies_.empty()) {
+    NOTREACHED();
+    return;
+  }
 
   if (!proxies_[0].is_direct()) {
     std::string key = proxies_[0].ToURI();
@@ -165,11 +199,6 @@ bool ProxyList::Fallback(ProxyRetryInfoMap* proxy_retry_info,
     net_log.AddEvent(NetLog::TYPE_PROXY_LIST_FALLBACK,
                      NetLog::StringCallback("bad_proxy", &key));
   }
-
-  // Remove this proxy from our list.
-  proxies_.erase(proxies_.begin());
-
-  return !proxies_.empty();
 }
 
 }  // namespace net