Remove the implicit fallback to DIRECT when proxies fail. This better matches...

net/proxy/proxy_service.cc

Index: net/proxy/proxy_service.cc
===================================================================
--- net/proxy/proxy_service.cc	(revision 35522)
+++ net/proxy/proxy_service.cc	(working copy)
@@ -158,9 +158,6 @@
     resolve_job_ = NULL;
     config_id_ = ProxyConfig::INVALID_ID;
 
-    // Notify the service of the completion.
-    service_->DidCompletePacRequest(results_->config_id_, result_code);
-
     // Clean up the results list.
     if (result_code == OK)
       results_->RemoveBadProxies(service_->proxy_retry_info_);
@@ -211,7 +208,6 @@
     : config_service_(config_service),
       resolver_(resolver),
       next_config_id_(1),
-      config_is_bad_(false),
       should_use_proxy_resolver_(false),
       ALLOW_THIS_IN_INITIALIZER_LIST(init_proxy_resolver_callback_(
           this, &ProxyService::OnInitProxyResolverComplete)) {
@@ -318,25 +314,14 @@
 
   DCHECK(config_.id() != ProxyConfig::INVALID_ID);
 
-  // Fallback to a "direct" (no proxy) connection if the current configuration
-  // is known to be bad.
-  if (config_is_bad_) {
-    // Reset this flag to false in case the ProxyInfo object is being
-    // re-used by the caller.
-    result->config_was_tried_ = false;
-  } else {
-    // Remember that we are trying to use the current proxy configuration.
-    result->config_was_tried_ = true;
+  if (should_use_proxy_resolver_ || IsInitializingProxyResolver()) {
+    // May need to go through ProxyResolver for this.
+    return ERR_IO_PENDING;
+  }
 
-    if (should_use_proxy_resolver_ || IsInitializingProxyResolver()) {
-      // May need to go through ProxyResolver for this.
-      return ERR_IO_PENDING;
-    }
-
-    if (!config_.proxy_rules.empty()) {
-      ApplyProxyRules(url, config_.proxy_rules, result);
-      return OK;
-    }
+  if (!config_.proxy_rules.empty()) {
+    ApplyProxyRules(url, config_.proxy_rules, result);
+    return OK;
   }
 
   // otherwise, we have no proxy config
@@ -455,12 +440,6 @@
     if (result->config_id_ != config_.id()) {
       // A new configuration!
       re_resolve = true;
-    } else if (!result->config_was_tried_) {
-      // We never tried the proxy configuration since we thought it was bad,
-      // but because we failed to establish a connection, let's try the proxy
-      // configuration again to see if it will work now.
-      config_is_bad_ = false;
-      re_resolve = true;
     }
   }
   if (re_resolve) {
@@ -470,27 +449,13 @@
     return ResolveProxy(url, result, callback, pac_request, load_log);
   }
 
-  // We don't have new proxy settings to try, fallback to the next proxy
+  // We don't have new proxy settings to try, try to fallback to the next proxy
   // in the list.
-  bool was_direct = result->is_direct();
-  if (!was_direct && result->Fallback(&proxy_retry_info_))
-    return OK;
+  bool did_fallback = result->Fallback(&proxy_retry_info_);
 
-  // TODO(eroman): Hmm, this doesn't seem right. For starters just because
-  // auto_detect is true doesn't mean we are actually using it.
-  if (!config_.auto_detect && !config_.proxy_rules.empty()) {
-    // If auto detect is on, then we should try a DIRECT connection
-    // as the attempt to reach the proxy failed.
-    return ERR_FAILED;
-  }
-
-  // If we already tried a direct connection, then just give up.
-  if (was_direct)
-    return ERR_FAILED;
-
-  // Try going direct.
-  result->UseDirect();
-  return OK;
+  // Return synchronous failure if there is nothing left to fall-back to.
+  // TODO(eroman): This is a yucky API, clean it up.
+  return did_fallback ? OK : ERR_FAILED;

[wtc, 2010/01/07 20:12:27]

I wonder if this ERR_FAILED error code may leak through
the call chain and end up in an error page.

[eroman, 2010/01/07 21:08:19]

This shouldn't leak since HttpNetworkTransaction checks for synchronous failure.
But as the TODO says, the API of the method should be changed so this is an
explicit result of the call.

[wtc, 2010/01/07 21:22:09]

I see.  Now I think an async failure (with ERR_FAILED
result code) will leak into HttpNetworkTransaction,
because HttpNetworkTransaction does not save the original
connection error code in a member variable.  We should
file a bug about this.

[wtc, 2010/01/07 21:53:56]

eroman went though the async failure case with me, and
showed me that HttpNetworkTransaction::DoProxyResolveComplete
will fall back to DIRECT rather than failing with the
async failure result code of ProxyResolver.

 }
 
 void ProxyService::CancelPacRequest(PacRequest* req) {
@@ -544,18 +509,6 @@
     resolver_->PurgeMemory();
 }
 
-void ProxyService::DidCompletePacRequest(int config_id, int result_code) {
-  // If we get an error that indicates a bad PAC config, then we should
-  // remember that, and not try the PAC config again for a while.
-
-  // Our config may have already changed.
-  if (result_code == OK || config_id != config_.id())
-    return;
-
-  // Remember that this configuration doesn't work.
-  config_is_bad_ = true;
-}
-
 // static
 ProxyConfigService* ProxyService::CreateSystemProxyConfigService(
     MessageLoop* io_loop, MessageLoop* file_loop) {
@@ -638,7 +591,6 @@
   config_.set_id(next_config_id_++);
 
   // Reset state associated with latest config.
-  config_is_bad_ = false;
   proxy_retry_info_.clear();
 
   // Cancel any PAC fetching / ProxyResolver::SetPacScript() which was