Add unittest for r37566 (on PAC failure, should fallback to DIRECT).

To make it more testable, I had to move the fallback code in question from HttpNetworkTransaction to ProxyService.
Although I think this is a better fit for that code anway, so it should be an overall readability improvement.

BUG=32316
TEST=ProxyServiceTest.ProxyFallback_BadConfig, ProxyServiceTest.ProxyResolverFails

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

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

4 files changed, 26 insertions, 51 deletions

diff --git a/net/http/http_network_transaction.cc b/net/http/http_network_transaction.cc
index f970384..300482f 100644
--- a/net/http/http_network_transaction.cc
+++ b/net/http/http_network_transaction.cc
@@ -579,44 +579,14 @@ int HttpNetworkTransaction::DoResolveProxyComplete(int result) {
 
   pac_request_ = NULL;
 
-  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();
-  }
+  if (result != OK)
+    return result;
 
   // Remove unsupported proxies from the list.
   proxy_info_.RemoveProxiesWithoutScheme(
       ProxyServer::SCHEME_DIRECT | ProxyServer::SCHEME_HTTP |
       ProxyServer::SCHEME_SOCKS4 | ProxyServer::SCHEME_SOCKS5);
 
-  // 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.
diff --git a/net/http/http_network_transaction_unittest.cc b/net/http/http_network_transaction_unittest.cc
index 4a71ab1..3b977bd 100644
--- a/net/http/http_network_transaction_unittest.cc
+++ b/net/http/http_network_transaction_unittest.cc
@@ -31,17 +31,8 @@
 
 //-----------------------------------------------------------------------------
 
-// TODO(eroman): Add a regression test for http://crbug.com/32316 -- when the
-// proxy service returns an error, we should fallback to DIRECT instead of
-// failing with ERR_NO_SUPPORTED_PROXIES.
-
 namespace net {
 
-// Create a proxy service which fails on all requests (falls back to direct).
-ProxyService* CreateNullProxyService() {
-  return ProxyService::CreateNull();
-}
-
 // Helper to manage the lifetimes of the dependencies for a
 // HttpNetworkTransaction.
 class SessionDependencies {
@@ -49,7 +40,7 @@ class SessionDependencies {
   // Default set of dependencies -- "null" proxy service.
   SessionDependencies()
       : host_resolver(new MockHostResolver),
-        proxy_service(CreateNullProxyService()),
+        proxy_service(ProxyService::CreateNull()),
         ssl_config_service(new SSLConfigServiceDefaults),
         flip_session_pool(new FlipSessionPool) {}
 
diff --git a/net/proxy/proxy_service.cc b/net/proxy/proxy_service.cc
index fb607ec..90fd297 100644
--- a/net/proxy/proxy_service.cc
+++ b/net/proxy/proxy_service.cc
@@ -492,13 +492,23 @@ int ProxyService::DidFinishResolvingProxy(ProxyInfo* result,
           load_log,
           std::string("Resolved proxy list: ") + result->ToPacString());
     }
+    result->DeprioritizeBadProxies(proxy_retry_info_);
   } else {
+    LoadLog::AddStringLiteral(
+        load_log,
+        "Got an error from proxy resolver, falling-back to DIRECT.");
     LoadLog::AddErrorCode(load_log, result_code);
-  }
 
-  // Clean up the results list.
-  if (result_code == OK)
-    result->DeprioritizeBadProxies(proxy_retry_info_);
+    // Fall-back to direct when the proxy resolver fails. This corresponds
+    // with a javascript runtime error in the PAC script.
+    //
+    // This implicit fall-back to direct matches Firefox 3.5 and
+    // Internet Explorer 8. For more information, see:
+    //
+    // http://www.chromium.org/developers/design-documents/proxy-settings-fallback
+    result->UseDirect();
+    result_code = OK;
+  }
 
   LoadLog::EndEvent(load_log, LoadLog::TYPE_PROXY_SERVICE);
   return result_code;
diff --git a/net/proxy/proxy_service_unittest.cc b/net/proxy/proxy_service_unittest.cc
index 2e57e51..b6804ce 100644
--- a/net/proxy/proxy_service_unittest.cc
+++ b/net/proxy/proxy_service_unittest.cc
@@ -330,7 +330,10 @@ TEST(ProxyServiceTest, ProxyResolverFails) {
   // Fail the first resolve request in MockAsyncProxyResolver.
   resolver->pending_requests()[0]->CompleteNow(ERR_FAILED);
 
-  EXPECT_EQ(ERR_FAILED, callback1.WaitForResult());
+  // Although the proxy resolver failed the request, ProxyService implicitly
+  // falls-back to DIRECT.
+  EXPECT_EQ(OK, callback1.WaitForResult());
+  EXPECT_TRUE(info.is_direct());
 
   // The second resolve request will try to run through the proxy resolver,
   // regardless of whether the first request failed in it.
@@ -642,10 +645,11 @@ TEST(ProxyServiceTest, ProxyFallback_BadConfig) {
   // This simulates a javascript runtime error in the PAC script.
   resolver->pending_requests()[0]->CompleteNow(ERR_FAILED);
 
-  // No proxy servers are returned, since we failed.
-  EXPECT_EQ(ERR_FAILED, callback3.WaitForResult());
-  EXPECT_FALSE(info2.is_direct());
-  EXPECT_TRUE(info2.is_empty());
+  // Although the resolver failed, the ProxyService will implicitly fall-back
+  // to a DIRECT connection.
+  EXPECT_EQ(OK, callback3.WaitForResult());
+  EXPECT_TRUE(info2.is_direct());
+  EXPECT_FALSE(info2.is_empty());
 
   // The PAC script will work properly next time and successfully return a
   // proxy list. Since we have not marked the configuration as bad, it should