Remove the implicit fallback to DIRECT when proxies fail. This better matches other browsers, and simplifies the code.

To better understand what this means, here are some examples how the behaviors will differ for the user:

(1) You start chrome with --proxy-server="foobar:80".
The server "foobar:80" is refusing connections.

  Before: Would fallback to direct after failing to connect through foobar:80.
  Now: Will error-out with connection refused after failing to connect through foobar:80.

(2) You start chrome with --proxy-pac-url="file:///foobar.pac".
The server "foobar:80" is unreachable, and foobar.pac reads:
  function FindProxyForURL(url, host) {
    return "PROXY foobar:80";
  }

  Before: Would fallback to direct after failing to connect through foobar:80.
  Now: Will error-out with connection refused after failing to connect through foobar:80.

(3) You start chrome with --proxy-pac-url="file:///foobar.pac".
The server "foobar:80" is unreachable, and foobar.pac reads:
  function FindProxyForURL(url, host) {
    return "PROXY foobar:80; DIRECT";
  }

  *No change, since the fallback to DIRECT is explicit in the PAC script*

BUG=12303

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

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

1 files changed, 42 insertions, 18 deletions

diff --git a/net/proxy/proxy_list.cc b/net/proxy/proxy_list.cc
index e12edf4..924fd15 100644
--- a/net/proxy/proxy_list.cc
+++ b/net/proxy/proxy_list.cc
@@ -61,10 +61,13 @@ void ProxyList::RemoveProxiesWithoutScheme(int scheme_bit_field) {
   }
 }
 
-ProxyServer ProxyList::Get() const {
-  if (!proxies_.empty())
-    return proxies_[0];
-  return ProxyServer(ProxyServer::SCHEME_DIRECT, std::string(), -1);
+bool ProxyList::IsEmpty() const {
+  return proxies_.empty();
+}
+
+const ProxyServer& ProxyList::Get() const {
+  DCHECK(!proxies_.empty());
+  return proxies_[0];
 }
 
 std::string ProxyList::ToPacString() const {
@@ -75,7 +78,7 @@ std::string ProxyList::ToPacString() const {
       proxy_list += ";";
     proxy_list += iter->ToPacString();
   }
-  return proxy_list.empty() ? "DIRECT" : proxy_list;
+  return proxy_list.empty() ? std::string() : proxy_list;
 }
 
 void ProxyList::SetFromPacString(const std::string& pac_string) {
@@ -88,30 +91,51 @@ void ProxyList::SetFromPacString(const std::string& pac_string) {
     if (uri.is_valid())
       proxies_.push_back(uri);
   }
+
+  // If we failed to parse anything from the PAC results list, fallback to
+  // DIRECT (this basically means an error in the PAC script).
+  if (proxies_.empty()) {
+    proxies_.push_back(ProxyServer::Direct());
+  }
 }
 
 bool ProxyList::Fallback(ProxyRetryInfoMap* proxy_retry_info) {
   // 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;
   }
 
-  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;
+  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;
+    }
   }
 
   // Remove this proxy from our list.