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

net/proxy/proxy_service.cc

 // Copyright (c) 2006-2008 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_service.h"
 
 #include <algorithm>
 
 #include "base/compiler_specific.h"
 #include "base/logging.h"
@@ skipping 140 matching lines @@
     DCHECK(!was_cancelled());
 
     // Make a note in the results which configuration was in use at the
     // time of the resolve.
     results_->config_id_ = config_id_;
 
     // Reset the state associated with in-progress-resolve.
     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_);
 
     LoadLog::EndEvent(load_log_, LoadLog::TYPE_PROXY_SERVICE);
 
     return result_code;
   }
 
   LoadLog* load_log() const { return load_log_; }
@@ skipping 30 matching lines @@
   scoped_refptr<LoadLog> load_log_;
 };
 
 // ProxyService ---------------------------------------------------------------
 
 ProxyService::ProxyService(ProxyConfigService* config_service,
                            ProxyResolver* resolver)
     : 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)) {
 }
 
 // static
 ProxyService* ProxyService::Create(
     ProxyConfigService* proxy_config_service,
     bool use_v8_resolver,
     URLRequestContext* url_request_context,
@@ skipping 86 matching lines @@
     *pac_request = req.get();
   return rv;  // ERR_IO_PENDING
 }
 
 int ProxyService::TryToCompleteSynchronously(const GURL& url,
                                              ProxyInfo* result) {
   result->config_id_ = config_.id();
 
   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
   result->UseDirect();
   return OK;
 }
 
 void ProxyService::ApplyProxyRules(const GURL& url,
                                    const ProxyConfig::ProxyRules& proxy_rules,
                                    ProxyInfo* result) {
@@ skipping 98 matching lines @@
   // Check to see if we have a new config since ResolveProxy was called.  We
   // want to re-run ResolveProxy in two cases: 1) we have a new config, or 2) a
   // direct connection failed and we never tried the current config.
 
   bool re_resolve = result->config_id_ != config_.id();
   if (!re_resolve) {
     UpdateConfig(load_log);
     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) {
     // If we have a new config or the config was never tried, we delete the
     // list of bad proxies and we try again.
     proxy_retry_info_.clear();
     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) {
   DCHECK(req);
   req->Cancel();
   RemovePendingRequest(req);
 }
 
 bool ProxyService::ContainsPendingRequest(PacRequest* req) {
   PendingRequests::iterator it = std::find(
@@ skipping 33 matching lines @@
     ProxyConfigService* new_proxy_config_service) {
   config_service_.reset(new_proxy_config_service);
   UpdateConfig(NULL);
 }
 
 void ProxyService::PurgeMemory() {
   if (resolver_.get())
     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) {
 #if defined(OS_WIN)
   return new ProxyConfigServiceWin();
 #elif defined(OS_MACOSX)
   return new ProxyConfigServiceMac();
 #elif defined(OS_LINUX)
   ProxyConfigServiceLinux* linux_config_service
       = new ProxyConfigServiceLinux();
@@ skipping 62 matching lines @@
   SetConfig(latest);
 }
 
 void ProxyService::SetConfig(const ProxyConfig& config) {
   config_ = config;
 
   // Increment the ID to reflect that the config has changed.
   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
   // in progress for the previous configuration.
   init_proxy_resolver_.reset();
   should_use_proxy_resolver_ = false;
 
   // Start downloading + testing the PAC scripts for this new configuration.
   if (config_.MayRequirePACResolver()) {
     // Since InitProxyResolver will be playing around with the proxy resolver
@@ skipping 175 matching lines @@
     OnCompletion(result_);
   }
 }
 
 void SyncProxyServiceHelper::OnCompletion(int rv) {
   result_ = rv;
   event_.Signal();
 }
 
 }  // namespace net