Merge 37191 - Merge 34903, 34928, 35008, 35549, 36054 to the 249s branch....

net/http/http_network_transaction.cc

Index: net/http/http_network_transaction.cc
===================================================================
--- net/http/http_network_transaction.cc	(revision 37198)
+++ net/http/http_network_transaction.cc	(working copy)
@@ -513,24 +513,60 @@
 }
 
 int HttpNetworkTransaction::DoResolveProxyComplete(int result) {
-  next_state_ = STATE_INIT_CONNECTION;
 
+  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 happens when we don't support any
+    // of the proxies in the returned list.
+    return ERR_NO_SUPPORTED_PROXIES;
+  }
+
   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;
 
@@ -644,7 +680,7 @@
   req_info.set_referrer(request_->referrer);
 
   if (proxy_info_.proxy_server().scheme() == ProxyServer::SCHEME_SOCKS5)
-    s = new SOCKS5ClientSocket(s, req_info, session_->host_resolver());
+    s = new SOCKS5ClientSocket(s, req_info);
   else
     s = new SOCKSClientSocket(s, req_info, session_->host_resolver());
   connection_.set_socket(s);
@@ -1414,6 +1450,10 @@
     connection_.Reset();
     next_state_ = STATE_RESOLVE_PROXY_COMPLETE;
   } else {
+    // If ReconsiderProxyAfterError() failed synchronously, it means
+    // there was nothing left to fall-back to, so fail the transaction
+    // with the last connection error we got.
+    // TODO(eroman): This is a confusing contract, make it more obvious.
     rv = error;
   }