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

net/http/http_network_transaction.cc

 // Copyright (c) 2006-2009 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/http/http_network_transaction.h"
 
 #include "base/format_macros.h"
 #include "base/scoped_ptr.h"
 #include "base/compiler_specific.h"
 #include "base/field_trial.h"
@@ skipping 558 matching lines @@
   if (request_->load_flags & LOAD_BYPASS_PROXY) {
     proxy_info_.UseDirect();
     return OK;
   }
 
   return session_->proxy_service()->ResolveProxy(
       request_->url, &proxy_info_, &io_callback_, &pac_request_, load_log_);
 }
 
 int HttpNetworkTransaction::DoResolveProxyComplete(int result) {
-  next_state_ = STATE_INIT_CONNECTION;
+

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

Nit: remove this blank line.

+  pac_request_ = NULL;
 
   // Remove unsupported proxies from the list.
   proxy_info_.RemoveProxiesWithoutScheme(
       ProxyServer::SCHEME_DIRECT | ProxyServer::SCHEME_HTTP |
       ProxyServer::SCHEME_SOCKS4 | ProxyServer::SCHEME_SOCKS5);
 
-  pac_request_ = NULL;
+  // There are four possible outcomes of having run the ProxyService:
+  //   (1) The ProxyService decided we should connect through a proxy.
+  //   (2) The ProxyService decided we should direct-connect.
+  //   (3) The ProxyService decided we should give up, as there are no more
+  //       proxies to try (this is more likely to happen during
+  //       ReconsiderProxyAfterError()).
+  //   (4) The ProxyService failed (which can happen if the PAC script
+  //       we were configured with threw a runtime exception).
+  //
+  // It is important that we fail the connection in case (3) rather than
+  // falling-back to a direct connection, since sending traffic through
+  // a proxy may be integral to the user's privacy/security model.
+  //
+  // For example if a user had configured traffic to go through the TOR
+  // anonymizing proxy to protect their privacy, it would be bad if we
+  // silently fell-back to direct connect if the proxy server were to
+  // become unreachable.
+  //
+  // In case (4) it is less obvious what the right thing to do is. On the
+  // one hand, for consistency it would be natural to hard-fail as well.
+  // However, both Firefox 3.5 and Internet Explorer 8 will silently fall-back
+  // to DIRECT in this case, so we will do the same for compatibility.
+  //
+  // For more information, see:
+  // http://www.chromium.org/developers/design-documents/proxy-settings-fallback
+
+  if (proxy_info_.is_empty()) {
+    // No proxies/direct to choose from. This can happen when:
+    //   a. We don't support any of the proxies in the returned list.
+    //   b. The proxy service returned us an empty list.
+    //      1. this can happen if all the proxies were marked as bad already.
+    //
+    // TODO(eroman): in case (b.1) it would be better to just try the bad
+    // proxies again rather than failing without having tried anything!
+    return ERR_EMPTY_PROXY_LIST;
+  }
 
   if (result != OK) {
     DLOG(ERROR) << "Failed to resolve proxy: " << result;
+    // Fall-back to direct when there were runtime errors in the PAC script,
+    // or some other failure with the settings.
     proxy_info_.UseDirect();
   }
+
+  next_state_ = STATE_INIT_CONNECTION;
   return OK;
 }
 
 int HttpNetworkTransaction::DoInitConnection() {
   DCHECK(!connection_->is_initialized());
+  DCHECK(proxy_info_.proxy_server().is_valid());
 
   next_state_ = STATE_INIT_CONNECTION_COMPLETE;
 
   using_ssl_ = request_->url.SchemeIs("https");
 
   if (proxy_info_.is_direct())
     proxy_mode_ = kDirectConnection;
   else if (proxy_info_.proxy_server().is_socks())
     proxy_mode_ = kSOCKSProxy;
   else if (using_ssl_)
@@ skipping 936 matching lines @@
   int rv = session_->proxy_service()->ReconsiderProxyAfterError(
       request_->url, &proxy_info_, &io_callback_, &pac_request_, load_log_);
   if (rv == OK || rv == ERR_IO_PENDING) {
     // If the error was during connection setup, there is no socket to
     // disconnect.
     if (connection_->socket())
       connection_->socket()->Disconnect();
     connection_->Reset();
     next_state_ = STATE_RESOLVE_PROXY_COMPLETE;
   } else {
+    // If ReconsiderProxyAfterError() failed synchronously, it means

[wtc, 2010/01/07 20:25:38]

Nit: ReconsiderProxyAfterError() =>
session_->proxy_service()->ReconsiderProxyAfterError() or
ProxyService::ReconsiderProxyAfterError() because the current
function is also named ReconsiderProxyAfterError().

+    // there was nothing left to fall-back to, so fail the transaction
+    // with the last connection error we got.

[wtc, 2010/01/07 20:25:38]

Nit: "last" => "original".

There is another reason we must report the original
connection error: session_->proxy_service()->ReconsiderProxyAfterError()
returns ERR_FAILED on failure.  If we return ERR_FAILED,
it would be bad (loss of error information).

With your comment (and perhaps my comment above), this isn't
that confusing.  So I don't know what you meant by
"confusing contract".

+    // TODO(eroman): This is a confusing contract, make it more obvious.
     rv = error;
   }
 
   return rv;
 }
 
 bool HttpNetworkTransaction::ShouldApplyProxyAuth() const {
   return (proxy_mode_ == kHTTPProxy) || establishing_tunnel_;
 }
 
@@ skipping 257 matching lines @@
   AuthChallengeInfo* auth_info = new AuthChallengeInfo;
   auth_info->is_proxy = target == HttpAuth::AUTH_PROXY;
   auth_info->host_and_port = ASCIIToWide(GetHostAndPort(auth_origin));
   auth_info->scheme = ASCIIToWide(auth_handler_[target]->scheme());
   // TODO(eroman): decode realm according to RFC 2047.
   auth_info->realm = ASCIIToWide(auth_handler_[target]->realm());
   response_.auth_challenge = auth_info;
 }
 
 }  // namespace net