Receiving Connection: Proxy-Bypass induces proxy fallback.

net/proxy/proxy_list.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/proxy/proxy_list.h"
 
 #include "base/callback.h"
 #include "base/logging.h"
 #include "base/string_tokenizer.h"
 #include "base/time.h"
@@ skipping 49 matching lines @@
       }
     }
     good_proxies.push_back(*iter);
   }
 
   // "proxies_ = good_proxies + bad_proxies"
   proxies_.swap(good_proxies);
   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(); ) {
     if (!(scheme_bit_field & it->scheme())) {
       it = proxies_.erase(it);
       continue;
     }
     ++it;
   }
 }
@@ skipping 39 matching lines @@
   for (; iter != proxies_.end(); ++iter) {
     if (!proxy_list.empty())
       proxy_list += ";";
     proxy_list += iter->ToPacString();
   }
   return proxy_list.empty() ? std::string() : proxy_list;
 }
 
 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
   // with. Of course, ProxyService::ReconsiderProxyAfterError() would need to
   // be given this information by the network transaction.
   //
   // The advantage of this approach is when the network transaction
   // fails, we could output the full list of proxies that were attempted, and
   // why each one of those failed (as opposed to just the last failure).
   //
   // And also, before failing the transaction wholesale, we could go back and
   // retry the "bad proxies" which we never tried to begin with.
   // (RemoveBadProxies would annotate them as 'expected bad' rather then delete
   // them from the list, so we would know what they were).
 
   if (proxies_.empty()) {
     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();
     // Mark this proxy as bad.
     ProxyRetryInfoMap::iterator iter = proxy_retry_info->find(key);
     if (iter != proxy_retry_info->end()) {
       // TODO(nsylvain): This is not the first time we get this. We should
       // double the retry time. Bug 997660.
       iter->second.bad_until = TimeTicks::Now() + iter->second.current_delay;
     } else {
       ProxyRetryInfo retry_info;
       retry_info.current_delay = kProxyRetryDelay;
       retry_info.bad_until = TimeTicks().Now() + retry_info.current_delay;
       (*proxy_retry_info)[key] = 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