Don't mutate |config_| after failing proxy-autodetect.

Otherwise when polling for configuration changes, we think the current configuration is different than the fetched one.
This was a recent regression from r22485.

BUG=http://crbug.com/18526
TEST=ProxyServiceTest.UpdateConfigAfterFailedAutodetect

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

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

3 files changed, 113 insertions, 5 deletions

diff --git a/net/proxy/proxy_service.cc b/net/proxy/proxy_service.cc
index 327e741..1798421 100644
--- a/net/proxy/proxy_service.cc
+++ b/net/proxy/proxy_service.cc
@@ -189,6 +189,7 @@ ProxyService::ProxyService(ProxyConfigService* 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)) {
 }
@@ -299,8 +300,8 @@ int ProxyService::TryToCompleteSynchronously(const GURL& url,
     // Remember that we are trying to use the current proxy configuration.
     result->config_was_tried_ = true;
 
-    if (config_.MayRequirePACResolver()) {
-      // Need to go through ProxyResolver for this.
+    if (should_use_proxy_resolver_ || IsInitializingProxyResolver()) {
+      // May need to go through ProxyResolver for this.
       return ERR_IO_PENDING;
     }
 
@@ -387,14 +388,14 @@ void ProxyService::ResumeAllPendingRequests() {
 void ProxyService::OnInitProxyResolverComplete(int result) {
   DCHECK(init_proxy_resolver_.get());
   DCHECK(config_.MayRequirePACResolver());
+  DCHECK(!should_use_proxy_resolver_);
   init_proxy_resolver_.reset();
 
+  should_use_proxy_resolver_ = result == OK;
+
   if (result != OK) {
     LOG(INFO) << "Failed configuring with PAC script, falling-back to manual "
                  "proxy servers.";
-    config_.auto_detect = false;
-    config_.pac_url = GURL();
-    DCHECK(!config_.MayRequirePACResolver());
   }
 
   // Resume any requests which we had to defer until the PAC script was
@@ -574,6 +575,7 @@ void ProxyService::SetConfig(const ProxyConfig& config) {
   // 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()) {
diff --git a/net/proxy/proxy_service.h b/net/proxy/proxy_service.h
index a7546f1..cc678f2 100644
--- a/net/proxy/proxy_service.h
+++ b/net/proxy/proxy_service.h
@@ -132,6 +132,8 @@ class ProxyService {
 
  private:
   FRIEND_TEST(ProxyServiceTest, IsLocalName);
+  FRIEND_TEST(ProxyServiceTest, UpdateConfigAfterFailedAutodetect);
+  FRIEND_TEST(ProxyServiceTest, UpdateConfigFromPACToDirect);
   friend class PacRequest;
   // TODO(eroman): change this to a std::set. Note that this requires updating
   // some tests in proxy_service_unittest.cc such as:
@@ -231,6 +233,9 @@ class ProxyService {
   // 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_;
+
   // The time when the proxy configuration was last read from the system.
   base::TimeTicks config_last_update_time_;
 
diff --git a/net/proxy/proxy_service_unittest.cc b/net/proxy/proxy_service_unittest.cc
index 0dac40b..3b3eb49 100644
--- a/net/proxy/proxy_service_unittest.cc
+++ b/net/proxy/proxy_service_unittest.cc
@@ -1525,4 +1525,105 @@ TEST(ProxyServiceTest, IsLocalName) {
   }
 }
 
+// Check that after we have done the auto-detect test, and the configuration
+// is updated (with no change), we don't re-try the autodetect test.
+// Regression test for http://crbug.com/18526 -- the configuration was being
+// mutated to cancel out the automatic settings, which meant UpdateConfig()
+// thought it had received a new configuration.
+TEST(ProxyServiceTest, UpdateConfigAfterFailedAutodetect) {
+  ProxyConfig config;
+  config.auto_detect = true;
+
+  MockProxyConfigService* config_service = new MockProxyConfigService(config);
+  MockAsyncProxyResolver* resolver = new MockAsyncProxyResolver;
+  ProxyService service(config_service, resolver);
+
+  // Start 1 requests.
+
+  ProxyInfo info1;
+  TestCompletionCallback callback1;
+  int rv = service.ResolveProxy(
+      GURL("http://www.google.com"), &info1, &callback1, NULL);
+  EXPECT_EQ(ERR_IO_PENDING, rv);
+
+  // Check that nothing has been sent to the proxy resolver yet.
+  ASSERT_EQ(0u, resolver->pending_requests().size());
+
+  // Fail the setting of autodetect script.
+  EXPECT_EQ(GURL(), resolver->pending_set_pac_script_request()->pac_url());
+  resolver->pending_set_pac_script_request()->CompleteNow(ERR_FAILED);
+
+  // Verify that request ran as expected -- should have fallen back to direct.
+  EXPECT_EQ(OK, callback1.WaitForResult());
+  EXPECT_TRUE(info1.is_direct());
+
+  // Force the ProxyService to pull down a new proxy configuration.
+  // (Even though the configuration isn't old/bad).
+  service.UpdateConfig();
+
+  // Start another request -- the effective configuration has not
+  // changed, so we shouldn't re-run the autodetect step.
+  // Rather, it should complete synchronously as direct-connect.
+  ProxyInfo info2;
+  TestCompletionCallback callback2;
+  rv = service.ResolveProxy(
+      GURL("http://www.google.com"), &info2, &callback2, NULL);
+  EXPECT_EQ(OK, rv);
+
+  EXPECT_TRUE(info2.is_direct());
+}
+
+// Test that when going from a configuration that required PAC to one
+// that does NOT, we unset the variable |should_use_proxy_resolver_|.
+TEST(ProxyServiceTest, UpdateConfigFromPACToDirect) {
+  ProxyConfig config;
+  config.auto_detect = true;
+
+  MockProxyConfigService* config_service = new MockProxyConfigService(config);
+  MockAsyncProxyResolver* resolver = new MockAsyncProxyResolver;
+  ProxyService service(config_service, resolver);
+
+  // Start 1 request.
+
+  ProxyInfo info1;
+  TestCompletionCallback callback1;
+  int rv = service.ResolveProxy(
+      GURL("http://www.google.com"), &info1, &callback1, NULL);
+  EXPECT_EQ(ERR_IO_PENDING, rv);
+
+  // Check that nothing has been sent to the proxy resolver yet.
+  ASSERT_EQ(0u, resolver->pending_requests().size());
+
+  // Successfully set the autodetect script.
+  EXPECT_EQ(GURL(), resolver->pending_set_pac_script_request()->pac_url());
+  resolver->pending_set_pac_script_request()->CompleteNow(OK);
+
+  // Complete the pending request.
+  ASSERT_EQ(1u, resolver->pending_requests().size());
+  resolver->pending_requests()[0]->results()->UseNamedProxy("request1:80");
+  resolver->pending_requests()[0]->CompleteNow(OK);
+
+  // Verify that request ran as expected.
+  EXPECT_EQ(OK, callback1.WaitForResult());
+  EXPECT_EQ("request1:80", info1.proxy_server().ToURI());
+
+  // Force the ProxyService to pull down a new proxy configuration.
+  // (Even though the configuration isn't old/bad).
+  //
+  // This new configuration no longer has auto_detect set, so
+  // requests should complete synchronously now as direct-connect.
+  config.auto_detect = false;
+  config_service->config = config;
+  service.UpdateConfig();
+
+  // Start another request -- the effective configuration has changed.
+  ProxyInfo info2;
+  TestCompletionCallback callback2;
+  rv = service.ResolveProxy(
+      GURL("http://www.google.com"), &info2, &callback2, NULL);
+  EXPECT_EQ(OK, rv);
+
+  EXPECT_TRUE(info2.is_direct());
+}
+
 }  // namespace net