Remove the implicit fallback to DIRECT when proxies fail. This better matches other browsers, and simplifies the code.

To better understand what this means, here are some examples how the behaviors will differ for the user:

(1) You start chrome with --proxy-server="foobar:80".
The server "foobar:80" is refusing connections.

  Before: Would fallback to direct after failing to connect through foobar:80.
  Now: Will error-out with connection refused after failing to connect through foobar:80.

(2) You start chrome with --proxy-pac-url="file:///foobar.pac".
The server "foobar:80" is unreachable, and foobar.pac reads:
  function FindProxyForURL(url, host) {
    return "PROXY foobar:80";
  }

  Before: Would fallback to direct after failing to connect through foobar:80.
  Now: Will error-out with connection refused after failing to connect through foobar:80.

(3) You start chrome with --proxy-pac-url="file:///foobar.pac".
The server "foobar:80" is unreachable, and foobar.pac reads:
  function FindProxyForURL(url, host) {
    return "PROXY foobar:80; DIRECT";
  }

  *No change, since the fallback to DIRECT is explicit in the PAC script*

BUG=12303

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

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

9 files changed, 258 insertions, 147 deletions

diff --git a/net/proxy/proxy_info.cc b/net/proxy/proxy_info.cc
index 98ca42f..36e9426 100644
--- a/net/proxy/proxy_info.cc
+++ b/net/proxy/proxy_info.cc
@@ -6,9 +6,7 @@
 
 namespace net {
 
-ProxyInfo::ProxyInfo()
-    : config_id_(ProxyConfig::INVALID_ID),
-      config_was_tried_(false) {
+ProxyInfo::ProxyInfo() : config_id_(ProxyConfig::INVALID_ID) {
 }
 
 void ProxyInfo::Use(const ProxyInfo& other) {
@@ -16,7 +14,7 @@ void ProxyInfo::Use(const ProxyInfo& other) {
 }
 
 void ProxyInfo::UseDirect() {
-  proxy_list_.Set(std::string());
+  proxy_list_.SetSingleProxyServer(ProxyServer::Direct());
 }
 
 void ProxyInfo::UseNamedProxy(const std::string& proxy_uri_list) {
diff --git a/net/proxy/proxy_info.h b/net/proxy/proxy_info.h
index 5a386ae..d3097a1 100644
--- a/net/proxy/proxy_info.h
+++ b/net/proxy/proxy_info.h
@@ -43,9 +43,20 @@ class ProxyInfo {
   }
 
   // Returns true if this proxy info specifies a direct connection.
-  bool is_direct() const { return proxy_list_.Get().is_direct(); }
+  bool is_direct() const {
+    // We don't implicitly fallback to DIRECT unless it was added to the list.
+    if (is_empty())
+      return false;
+    return proxy_list_.Get().is_direct();
+  }
+
+  // Returns true if this proxy info has no proxies left to try.
+  bool is_empty() const {
+    return proxy_list_.IsEmpty();
+  }
 
-  // Returns the first valid proxy server.
+  // Returns the first valid proxy server. is_empty() must be false to be able
+  // to call this function.
   ProxyServer proxy_server() const { return proxy_list_.Get(); }
 
   // See description in ProxyList::ToPacString().
@@ -70,17 +81,12 @@ class ProxyInfo {
  private:
   friend class ProxyService;
 
-  // If proxy_list_ is set to empty, then a "direct" connection is indicated.
+  // The ordered list of proxy servers (including DIRECT attempts) remaining to
+  // try. If proxy_list_ is empty, then there is nothing left to fall back to.
   ProxyList proxy_list_;
 
   // This value identifies the proxy config used to initialize this object.
   ProxyConfig::ID config_id_;
-
-  // This flag is false when the proxy configuration was known to be bad when
-  // this proxy info was initialized.  In such cases, we know that if this
-  // proxy info does not yield a connection that we might want to reconsider
-  // the proxy config given by config_id_.
-  bool config_was_tried_;
 };
 
 }  // namespace net
diff --git a/net/proxy/proxy_list.cc b/net/proxy/proxy_list.cc
index e12edf4..924fd15 100644
--- a/net/proxy/proxy_list.cc
+++ b/net/proxy/proxy_list.cc
@@ -61,10 +61,13 @@ void ProxyList::RemoveProxiesWithoutScheme(int scheme_bit_field) {
   }
 }
 
-ProxyServer ProxyList::Get() const {
-  if (!proxies_.empty())
-    return proxies_[0];
-  return ProxyServer(ProxyServer::SCHEME_DIRECT, std::string(), -1);
+bool ProxyList::IsEmpty() const {
+  return proxies_.empty();
+}
+
+const ProxyServer& ProxyList::Get() const {
+  DCHECK(!proxies_.empty());
+  return proxies_[0];
 }
 
 std::string ProxyList::ToPacString() const {
@@ -75,7 +78,7 @@ std::string ProxyList::ToPacString() const {
       proxy_list += ";";
     proxy_list += iter->ToPacString();
   }
-  return proxy_list.empty() ? "DIRECT" : proxy_list;
+  return proxy_list.empty() ? std::string() : proxy_list;
 }
 
 void ProxyList::SetFromPacString(const std::string& pac_string) {
@@ -88,30 +91,51 @@ void ProxyList::SetFromPacString(const std::string& pac_string) {
     if (uri.is_valid())
       proxies_.push_back(uri);
   }
+
+  // If we failed to parse anything from the PAC results list, fallback to
+  // DIRECT (this basically means an error in the PAC script).
+  if (proxies_.empty()) {
+    proxies_.push_back(ProxyServer::Direct());
+  }
 }
 
 bool ProxyList::Fallback(ProxyRetryInfoMap* proxy_retry_info) {
   // Number of minutes to wait before retrying a bad proxy server.
   const TimeDelta kProxyRetryDelay = TimeDelta::FromMinutes(5);
 
+  // TODO(eroman): It would be good if instead of removing failed proxies
+  // from the list, we simply annotated them with the error code they failed
+  // with. Of course, ProxyService::ReconsiderProxyAfterError() would need to
+  // be given this information by the network transaction.
+  //
+  // The advantage of this approach is when the network transaction
+  // fails, we could output the full list of proxies that were attempted, and
+  // why each one of those failed (as opposed to just the last failure).
+  //
+  // And also, before failing the transaction wholesale, we could go back and
+  // retry the "bad proxies" which we never tried to begin with.
+  // (RemoveBadProxies would annotate them as 'expected bad' rather then delete
+  // them from the list, so we would know what they were).
+
   if (proxies_.empty()) {
     NOTREACHED();
     return false;
   }
 
-  std::string key = proxies_[0].ToURI();
-
-  // Mark this proxy as bad.
-  ProxyRetryInfoMap::iterator iter = proxy_retry_info->find(key);
-  if (iter != proxy_retry_info->end()) {
-    // TODO(nsylvain): This is not the first time we get this. We should
-    // double the retry time. Bug 997660.
-    iter->second.bad_until = TimeTicks::Now() + iter->second.current_delay;
-  } else {
-    ProxyRetryInfo retry_info;
-    retry_info.current_delay = kProxyRetryDelay;
-    retry_info.bad_until = TimeTicks().Now() + retry_info.current_delay;
-    (*proxy_retry_info)[key] = retry_info;
+  if (!proxies_[0].is_direct()) {
+    std::string key = proxies_[0].ToURI();
+    // Mark this proxy as bad.
+    ProxyRetryInfoMap::iterator iter = proxy_retry_info->find(key);
+    if (iter != proxy_retry_info->end()) {
+      // TODO(nsylvain): This is not the first time we get this. We should
+      // double the retry time. Bug 997660.
+      iter->second.bad_until = TimeTicks::Now() + iter->second.current_delay;
+    } else {
+      ProxyRetryInfo retry_info;
+      retry_info.current_delay = kProxyRetryDelay;
+      retry_info.bad_until = TimeTicks().Now() + retry_info.current_delay;
+      (*proxy_retry_info)[key] = retry_info;
+    }
   }
 
   // Remove this proxy from our list.
diff --git a/net/proxy/proxy_list.h b/net/proxy/proxy_list.h
index 884dec9..f05d4cf 100644
--- a/net/proxy/proxy_list.h
+++ b/net/proxy/proxy_list.h
@@ -32,8 +32,12 @@ class ProxyList {
   // |scheme_bit_field| is a bunch of ProxyServer::Scheme bitwise ORed together.
   void RemoveProxiesWithoutScheme(int scheme_bit_field);
 
-  // Returns the first valid proxy server in the list.
-  ProxyServer Get() const;
+  // Returns true if there is nothing left in the ProxyList.
+  bool IsEmpty() const;
+
+  // Returns the first proxy server in the list. It is only valid to call
+  // this if !IsEmpty().
+  const ProxyServer& Get() const;
 
   // Set the list by parsing the pac result |pac_string|.
   // Some examples for |pac_string|:
diff --git a/net/proxy/proxy_list_unittest.cc b/net/proxy/proxy_list_unittest.cc
index c52381e..401461f 100644
--- a/net/proxy/proxy_list_unittest.cc
+++ b/net/proxy/proxy_list_unittest.cc
@@ -24,9 +24,20 @@ TEST(ProxyListTest, SetFromPacString) {
     {  "proxy foopy1 ; SOCKS foopy2",
        "PROXY foopy1:80;SOCKS foopy2:1080",
     },
+    // Try putting DIRECT first.
+    {  "DIRECT ; proxy foopy1 ; DIRECT ; SOCKS5 foopy2;DIRECT ",
+       "DIRECT;PROXY foopy1:80;DIRECT;SOCKS5 foopy2:1080;DIRECT",
+    },
+    // Try putting DIRECT consecutively.
+    {  "DIRECT ; proxy foopy1:80; DIRECT ; DIRECT",
+       "DIRECT;PROXY foopy1:80;DIRECT;DIRECT",
+    },
 
     // Invalid inputs (parts which aren't understood get
     // silently discarded):
+    //
+    // If the proxy list string parsed to empty, automatically fall-back to
+    // DIRECT.
     {  "PROXY-foopy:10",
        "DIRECT",
     },
@@ -42,6 +53,7 @@ TEST(ProxyListTest, SetFromPacString) {
     net::ProxyList list;
     list.SetFromPacString(tests[i].pac_input);
     EXPECT_EQ(tests[i].pac_output, list.ToPacString());
+    EXPECT_FALSE(list.IsEmpty());
   }
 }
 
@@ -56,10 +68,10 @@ TEST(ProxyListTest, RemoveProxiesWithoutScheme) {
        net::ProxyServer::SCHEME_DIRECT | net::ProxyServer::SCHEME_HTTP,
        "PROXY foopy:10;PROXY foopy3:80;DIRECT",
     },
-    {  "PROXY foopy:10 | SOCKS5 foopy2",
+    {  "PROXY foopy:10 ; SOCKS5 foopy2",
        // Remove anything that isn't HTTP or SOCKS5.
        net::ProxyServer::SCHEME_DIRECT | net::ProxyServer::SCHEME_SOCKS4,
-       "DIRECT",
+       "",
     },
   };
 
diff --git a/net/proxy/proxy_server.h b/net/proxy/proxy_server.h
index 67feccc..d079396 100644
--- a/net/proxy/proxy_server.h
+++ b/net/proxy/proxy_server.h
@@ -107,6 +107,11 @@ class ProxyServer {
   static ProxyServer FromPacString(std::string::const_iterator pac_string_begin,
                                    std::string::const_iterator pac_string_end);
 
+  // Returns a ProxyServer representing DIRECT connections.
+  static ProxyServer Direct() {
+    return ProxyServer(SCHEME_DIRECT, std::string(), -1);
+  }
+
 #if defined(OS_MACOSX)
   // Utility function to pull out a host/port pair from a dictionary and return
   // it as a ProxyServer object. Pass in a dictionary that has a  value for the
diff --git a/net/proxy/proxy_service.cc b/net/proxy/proxy_service.cc
index 071415d..9ac6fcd 100644
--- a/net/proxy/proxy_service.cc
+++ b/net/proxy/proxy_service.cc
@@ -158,9 +158,6 @@ class ProxyService::PacRequest
     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 @@ ProxyService::ProxyService(ProxyConfigService* config_service,
     : 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 @@ int ProxyService::TryToCompleteSynchronously(const GURL& url,
 
   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 @@ int ProxyService::ReconsiderProxyAfterError(const GURL& url,
     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 @@ int ProxyService::ReconsiderProxyAfterError(const GURL& url,
     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;
 }
 
 void ProxyService::CancelPacRequest(PacRequest* req) {
@@ -544,18 +509,6 @@ void ProxyService::PurgeMemory() {
     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 @@ void ProxyService::SetConfig(const ProxyConfig& config) {
   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
diff --git a/net/proxy/proxy_service.h b/net/proxy/proxy_service.h
index 715a9c2..ed02b03 100644
--- a/net/proxy/proxy_service.h
+++ b/net/proxy/proxy_service.h
@@ -222,12 +222,6 @@ class ProxyService : public base::RefCountedThreadSafe<ProxyService> {
   // Removes |req| from the list of pending requests.
   void RemovePendingRequest(PacRequest* req);
 
-  // Called to indicate that a PacRequest completed.  The |config_id| parameter
-  // indicates the proxy configuration that was queried.  |result_code| is OK
-  // if the PAC file could be downloaded and executed.  Otherwise, it is an
-  // error code, indicating a bad proxy configuration.
-  void DidCompletePacRequest(int config_id, int result_code);
-
   // Returns true if the URL passed in should not go through the proxy server.
   // 1. If the proxy settings say to bypass local names, and |IsLocalName(url)|.
   // 2. The URL matches one of the entities in the proxy bypass list.
@@ -253,9 +247,6 @@ class ProxyService : public base::RefCountedThreadSafe<ProxyService> {
   // Increasing ID to give to the next ProxyConfig that we set.
   int next_config_id_;
 
-  // Indicates that the configuration is bad and should be ignored.
-  bool config_is_bad_;
-
   // Indicates whether the ProxyResolver should be sent requests.
   bool should_use_proxy_resolver_;
 
diff --git a/net/proxy/proxy_service_unittest.cc b/net/proxy/proxy_service_unittest.cc
index 6308cc5..ecf28c9 100644
--- a/net/proxy/proxy_service_unittest.cc
+++ b/net/proxy/proxy_service_unittest.cc
@@ -185,7 +185,7 @@ TEST(ProxyServiceTest, PAC_NoIdentityOrHash) {
   // ProxyService will cancel the outstanding request.
 }
 
-TEST(ProxyServiceTest, PAC_FailoverToDirect) {
+TEST(ProxyServiceTest, PAC_FailoverWithoutDirect) {
   MockProxyConfigService* config_service =
       new MockProxyConfigService("http://foopy/proxy.pac");
   MockAsyncProxyResolver* resolver = new MockAsyncProxyResolver;
@@ -215,18 +215,94 @@ TEST(ProxyServiceTest, PAC_FailoverToDirect) {
   EXPECT_FALSE(info.is_direct());
   EXPECT_EQ("foopy:8080", info.proxy_server().ToURI());
 
-  // Now, imagine that connecting to foopy:8080 fails.
+  // Now, imagine that connecting to foopy:8080 fails: there is nothing
+  // left to fallback to, since our proxy list was NOT terminated by
+  // DIRECT.
   TestCompletionCallback callback2;
   rv = service->ReconsiderProxyAfterError(url, &info, &callback2, NULL, NULL);
+  // ReconsiderProxyAfterError returns error indicating nothing left.
+  EXPECT_EQ(ERR_FAILED, rv);
+  EXPECT_TRUE(info.is_empty());
+}
+
+// The proxy list could potentially contain the DIRECT fallback choice
+// in a location other than the very end of the list, and could even
+// specify it multiple times.
+//
+// This is not a typical usage, but we will obey it.
+// (If we wanted to disallow this type of input, the right place to
+// enforce it would be in parsing the PAC result string).
+//
+// This test will use the PAC result string:
+//
+//   "DIRECT ; PROXY foobar:10 ; DIRECT ; PROXY foobar:20"
+//
+// For which we expect it to try DIRECT, then foobar:10, then DIRECT again,
+// then foobar:20, and then give up and error.
+//
+// The important check of this test is to make sure that DIRECT is not somehow
+// cached as being a bad proxy.
+TEST(ProxyServiceTest, PAC_FailoverAfterDirect) {
+  MockProxyConfigService* config_service =
+      new MockProxyConfigService("http://foopy/proxy.pac");
+  MockAsyncProxyResolver* resolver = new MockAsyncProxyResolver;
+
+  scoped_refptr<ProxyService> service(
+      new ProxyService(config_service, resolver));
+
+  GURL url("http://www.google.com/");
+
+  ProxyInfo info;
+  TestCompletionCallback callback1;
+  int rv = service->ResolveProxy(url, &info, &callback1, NULL, NULL);
+  EXPECT_EQ(ERR_IO_PENDING, rv);
+
+  EXPECT_EQ(GURL("http://foopy/proxy.pac"),
+            resolver->pending_set_pac_script_request()->pac_url());
+  resolver->pending_set_pac_script_request()->CompleteNow(OK);
+
+  ASSERT_EQ(1u, resolver->pending_requests().size());
+  EXPECT_EQ(url, resolver->pending_requests()[0]->url());
+
+  // Set the result in proxy resolver.
+  resolver->pending_requests()[0]->results()->UsePacString(
+      "DIRECT ; PROXY foobar:10 ; DIRECT ; PROXY foobar:20");
+  resolver->pending_requests()[0]->CompleteNow(OK);
+
+  EXPECT_EQ(OK, callback1.WaitForResult());
+  EXPECT_TRUE(info.is_direct());
+
+  // Fallback 1.
+  TestCompletionCallback callback2;
+  rv = service->ReconsiderProxyAfterError(url, &info, &callback2, NULL, NULL);
+  EXPECT_EQ(OK, rv);
+  EXPECT_FALSE(info.is_direct());
+  EXPECT_EQ("foobar:10", info.proxy_server().ToURI());
+
+  // Fallback 2.
+  TestCompletionCallback callback3;
+  rv = service->ReconsiderProxyAfterError(url, &info, &callback3, NULL, NULL);
   EXPECT_EQ(OK, rv);
   EXPECT_TRUE(info.is_direct());
+
+  // Fallback 3.
+  TestCompletionCallback callback4;
+  rv = service->ReconsiderProxyAfterError(url, &info, &callback4, NULL, NULL);
+  EXPECT_EQ(OK, rv);
+  EXPECT_FALSE(info.is_direct());
+  EXPECT_EQ("foobar:20", info.proxy_server().ToURI());
+
+  // Fallback 4 -- Nothing to fall back to!
+  TestCompletionCallback callback5;
+  rv = service->ReconsiderProxyAfterError(url, &info, &callback5, NULL, NULL);
+  EXPECT_EQ(ERR_FAILED, rv);
+  EXPECT_TRUE(info.is_empty());
 }
 
 TEST(ProxyServiceTest, ProxyResolverFails) {
-  // Test what happens when the ProxyResolver fails (this could represent
-  // a failure to download the PAC script in the case of ProxyResolvers which
-  // do the fetch internally.)
-  // TODO(eroman): change this comment.
+  // Test what happens when the ProxyResolver fails. The download and setting
+  // of the PAC script have already succeeded, so this corresponds with a
+  // javascript runtime error while calling FindProxyForURL().
 
   MockProxyConfigService* config_service =
       new MockProxyConfigService("http://foopy/proxy.pac");
@@ -255,28 +331,21 @@ TEST(ProxyServiceTest, ProxyResolverFails) {
 
   EXPECT_EQ(ERR_FAILED, callback1.WaitForResult());
 
-  // The second resolve request will automatically select direct connect,
-  // because it has cached the configuration as being bad.
+  // The second resolve request will try to run through the proxy resolver,
+  // regardless of whether the first request failed in it.
   TestCompletionCallback callback2;
   rv = service->ResolveProxy(url, &info, &callback2, NULL, NULL);
-  EXPECT_EQ(OK, rv);
-  EXPECT_TRUE(info.is_direct());
-  EXPECT_TRUE(resolver->pending_requests().empty());
-
-  // But, if that fails, then we should give the proxy config another shot
-  // since we have never tried it with this URL before.
-  TestCompletionCallback callback3;
-  rv = service->ReconsiderProxyAfterError(url, &info, &callback3, NULL, NULL);
   EXPECT_EQ(ERR_IO_PENDING, rv);
 
   ASSERT_EQ(1u, resolver->pending_requests().size());
   EXPECT_EQ(url, resolver->pending_requests()[0]->url());
 
-    // Set the result in proxy resolver.
+  // This time we will have the resolver succeed (perhaps the PAC script has
+  // a dependency on the current time).
   resolver->pending_requests()[0]->results()->UseNamedProxy("foopy_valid:8080");
   resolver->pending_requests()[0]->CompleteNow(OK);
 
-  EXPECT_EQ(OK, callback3.WaitForResult());
+  EXPECT_EQ(OK, callback2.WaitForResult());
   EXPECT_FALSE(info.is_direct());
   EXPECT_EQ("foopy_valid:8080", info.proxy_server().ToURI());
 }
@@ -349,20 +418,77 @@ TEST(ProxyServiceTest, ProxyFallback) {
   EXPECT_EQ(OK, rv);
   EXPECT_EQ("foopy2:9090", info.proxy_server().ToURI());
 
-  // Fake another error, the last proxy is gone, the list should now be empty.
+  // Fake another error, the last proxy is gone, the list should now be empty,
+  // so there is nothing left to try.
   TestCompletionCallback callback5;
   rv = service->ReconsiderProxyAfterError(url, &info, &callback5, NULL, NULL);
-  EXPECT_EQ(OK, rv);  // We try direct.
-  EXPECT_TRUE(info.is_direct());
-
-  // If it fails again, we don't have anything else to try.
-  TestCompletionCallback callback6;
-  rv = service->ReconsiderProxyAfterError(url, &info, &callback6, NULL, NULL);
   EXPECT_EQ(ERR_FAILED, rv);
+  EXPECT_FALSE(info.is_direct());
+  EXPECT_TRUE(info.is_empty());
 
   // TODO(nsylvain): Test that the proxy can be retried after the delay.
 }
 
+// This test is similar to ProxyFallback, but this time we have an explicit
+// fallback choice to DIRECT.
+TEST(ProxyServiceTest, ProxyFallbackToDirect) {
+  MockProxyConfigService* config_service =
+      new MockProxyConfigService("http://foopy/proxy.pac");
+
+  MockAsyncProxyResolver* resolver = new MockAsyncProxyResolver;
+
+  scoped_refptr<ProxyService> service(
+      new ProxyService(config_service, resolver));
+
+  GURL url("http://www.google.com/");
+
+  // Get the proxy information.
+  ProxyInfo info;
+  TestCompletionCallback callback1;
+  int rv = service->ResolveProxy(url, &info, &callback1, NULL, NULL);
+  EXPECT_EQ(ERR_IO_PENDING, rv);
+
+  EXPECT_EQ(GURL("http://foopy/proxy.pac"),
+            resolver->pending_set_pac_script_request()->pac_url());
+  resolver->pending_set_pac_script_request()->CompleteNow(OK);
+
+  ASSERT_EQ(1u, resolver->pending_requests().size());
+  EXPECT_EQ(url, resolver->pending_requests()[0]->url());
+
+  // Set the result in proxy resolver.
+  resolver->pending_requests()[0]->results()->UsePacString(
+      "PROXY foopy1:8080; PROXY foopy2:9090; DIRECT");
+  resolver->pending_requests()[0]->CompleteNow(OK);
+
+  // Get the first result.
+  EXPECT_EQ(OK, callback1.WaitForResult());
+  EXPECT_FALSE(info.is_direct());
+  EXPECT_EQ("foopy1:8080", info.proxy_server().ToURI());
+
+  // Fake an error on the proxy.
+  TestCompletionCallback callback2;
+  rv = service->ReconsiderProxyAfterError(url, &info, &callback2, NULL, NULL);
+  EXPECT_EQ(OK, rv);
+
+  // Now we get back the second proxy.
+  EXPECT_EQ("foopy2:9090", info.proxy_server().ToURI());
+
+  // Fake an error on this proxy as well.
+  TestCompletionCallback callback3;
+  rv = service->ReconsiderProxyAfterError(url, &info, &callback3, NULL, NULL);
+  EXPECT_EQ(OK, rv);
+
+  // Finally, we get back DIRECT.
+  EXPECT_TRUE(info.is_direct());
+
+  // Now we tell the proxy service that even DIRECT failed.
+  TestCompletionCallback callback4;
+  rv = service->ReconsiderProxyAfterError(url, &info, &callback4, NULL, NULL);
+  // There was nothing left to try after DIRECT, so we are out of
+  // choices.
+  EXPECT_EQ(ERR_FAILED, rv);
+}
+
 TEST(ProxyServiceTest, ProxyFallback_NewSettings) {
   // Test proxy failover when new settings are available.
 
@@ -504,28 +630,20 @@ TEST(ProxyServiceTest, ProxyFallback_BadConfig) {
   ASSERT_EQ(1u, resolver->pending_requests().size());
   EXPECT_EQ(url, resolver->pending_requests()[0]->url());
 
+  // This simulates a javascript runtime error in the PAC script.
   resolver->pending_requests()[0]->CompleteNow(ERR_FAILED);
 
-  // No proxy servers are returned. It's a direct connection.
+  // No proxy servers are returned, since we failed.
   EXPECT_EQ(ERR_FAILED, callback3.WaitForResult());
-  EXPECT_TRUE(info2.is_direct());
-
-  // The PAC will now be fixed and will return a proxy server.
-  // It should also clear the list of bad proxies.
+  EXPECT_FALSE(info2.is_direct());
+  EXPECT_TRUE(info2.is_empty());
 
-  // Try to resolve, it will still return "direct" because we have no reason
-  // to check the config since everything works.
+  // 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
+  // "just work" the next time we call it.
   ProxyInfo info3;
   TestCompletionCallback callback4;
-  rv = service->ResolveProxy(url, &info3, &callback4, NULL, NULL);
-  EXPECT_EQ(OK, rv);
-  EXPECT_TRUE(info3.is_direct());
-
-  // But if the direct connection fails, we check if the ProxyInfo tried to
-  // resolve the proxy before, and if not (like in this case), we give the
-  // PAC another try.
-  TestCompletionCallback callback5;
-  rv = service->ReconsiderProxyAfterError(url, &info3, &callback5, NULL, NULL);
+  rv = service->ReconsiderProxyAfterError(url, &info3, &callback4, NULL, NULL);
   EXPECT_EQ(ERR_IO_PENDING, rv);
 
   ASSERT_EQ(1u, resolver->pending_requests().size());
@@ -535,8 +653,9 @@ TEST(ProxyServiceTest, ProxyFallback_BadConfig) {
       "foopy1:8080;foopy2:9090");
   resolver->pending_requests()[0]->CompleteNow(OK);
 
-  // The first proxy is still there since the list of bad proxies got cleared.
-  EXPECT_EQ(OK, callback5.WaitForResult());
+  // The first proxy is not there since the it was added to the bad proxies
+  // list by the earlier ReconsiderProxyAfterError().
+  EXPECT_EQ(OK, callback4.WaitForResult());
   EXPECT_FALSE(info3.is_direct());
   EXPECT_EQ("foopy1:8080", info3.proxy_server().ToURI());
 }