Fast-fail WPAD detection.

net/proxy/proxy_script_decider_unittest.cc

 // Copyright (c) 2012 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 <vector>
 
 #include "base/bind.h"
 #include "base/memory/weak_ptr.h"
 #include "base/message_loop/message_loop.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/time/time.h"
 #include "net/base/net_errors.h"
 #include "net/base/net_log.h"
 #include "net/base/net_log_unittest.h"
 #include "net/base/test_completion_callback.h"
+#include "net/dns/mock_host_resolver.h"
 #include "net/proxy/dhcp_proxy_script_fetcher.h"
 #include "net/proxy/proxy_config.h"
 #include "net/proxy/proxy_resolver.h"
 #include "net/proxy/proxy_script_decider.h"
 #include "net/proxy/proxy_script_fetcher.h"
+#include "net/url_request/url_request_context.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace net {
 namespace {
 
 enum Error {
   kFailedDownloading = -100,
   kFailedParsing = ERR_PAC_SCRIPT_FAILED,
 };
 
@@ skipping 54 matching lines @@
     return rules_[0];
   }
 
  private:
   typedef std::vector<Rule> RuleList;
   RuleList rules_;
 };
 
 class RuleBasedProxyScriptFetcher : public ProxyScriptFetcher {
  public:
-  explicit RuleBasedProxyScriptFetcher(const Rules* rules) : rules_(rules) {}
+  explicit RuleBasedProxyScriptFetcher(const Rules* rules)
+      : rules_(rules), request_context_(NULL) {}
 
   // ProxyScriptFetcher implementation.
   virtual int Fetch(const GURL& url,
                     base::string16* text,
                     const CompletionCallback& callback) OVERRIDE {
     const Rules::Rule& rule = rules_->GetRuleByUrl(url);
     int rv = rule.fetch_error;
     EXPECT_NE(ERR_UNEXPECTED, rv);
     if (rv == OK)
       *text = rule.text();
     return rv;
   }
 
   virtual void Cancel() OVERRIDE {}
 
-  virtual URLRequestContext* GetRequestContext() const OVERRIDE { return NULL; }
+  virtual void SetRequestContext(URLRequestContext* context) {

[szym, 2013/09/11 14:06:22]

nit: move this out of here, so that the "ProxyScriptFetcher implementation" is a
single block. I suggest moving it right after the ctor.

[Elly Fong-Jones, 2013/09/11 20:20:37]

Done.

+    request_context_ = context;
+  }
+
+  virtual URLRequestContext* GetRequestContext() const OVERRIDE {
+    return request_context_;
+  }
 
  private:
   const Rules* rules_;
+  URLRequestContext* request_context_;
 };
 
 // Succeed using custom PAC script.
 TEST(ProxyScriptDeciderTest, CustomPacSucceeds) {
   Rules rules;
   RuleBasedProxyScriptFetcher fetcher(&rules);
   DoNothingDhcpProxyScriptFetcher dhcp_fetcher;
 
   ProxyConfig config;
   config.set_pac_url(GURL("http://custom/proxy.pac"));
@@ skipping 110 matching lines @@
   TestCompletionCallback callback;
   ProxyScriptDecider decider(&fetcher, &dhcp_fetcher, NULL);
   EXPECT_EQ(OK, decider.Start(
       config, base::TimeDelta(), true, callback.callback()));
   EXPECT_EQ(rule.text(), decider.script_data()->utf16());
 
   EXPECT_TRUE(decider.effective_config().has_pac_url());
   EXPECT_EQ(rule.url, decider.effective_config().pac_url());
 }
 
+class ProxyScriptDeciderQuickCheckTest : public ::testing::Test {
+ public:
+  ProxyScriptDeciderQuickCheckTest()
+      : rule_(rules_.AddSuccessRule("http://wpad/wpad.dat")),
+        fetcher_(&rules_) { }
+
+  virtual void SetUp() {

[szym, 2013/09/11 14:06:22]

OVERRIDE

[Elly Fong-Jones, 2013/09/11 20:20:37]

Done.

+    request_context_.set_host_resolver(&resolver_);
+    fetcher_.SetRequestContext(&request_context_);
+    config_.set_auto_detect(true);
+    decider_.reset(new ProxyScriptDecider(&fetcher_, &dhcp_fetcher_, NULL));
+  }
+
+  virtual int StartDecider() {

[szym, 2013/09/11 14:06:22]

why virtual?

[Elly Fong-Jones, 2013/09/11 20:20:37]

Habit. Removed.

+    return decider()->Start(config_, base::TimeDelta(), true,
+                            callback_.callback());
+  }
+
+  Rules::Rule* rule() { return &rule_; }
+  ProxyScriptDecider* decider() { return decider_.get(); }
+  MockHostResolver* resolver() { return &resolver_; }
+
+ private:

[szym, 2013/09/11 14:06:22]

I'd suggest making this protected and using directly instead of the three
getters above. The goal is to have less boiler plate.

If you don't want to expose everything, have a separate private section.

[Elly Fong-Jones, 2013/09/11 20:20:37]

Done.

+  URLRequestContext request_context_;
+  MockHostResolver resolver_;
+
+  Rules rules_;
+  Rules::Rule rule_;
+  RuleBasedProxyScriptFetcher fetcher_;
+  DoNothingDhcpProxyScriptFetcher dhcp_fetcher_;
+
+  TestCompletionCallback callback_;
+
+  ProxyConfig config_;
+
+  scoped_ptr<ProxyScriptDecider> decider_;
+};
+
+TEST_F(ProxyScriptDeciderQuickCheckTest, SyncSuccess) {
+  resolver()->set_synchronous_mode(true);
+  resolver()->rules()->AddRule("wpad", "1.2.3.4");
+
+  EXPECT_EQ(OK, StartDecider());
+  EXPECT_EQ(rule()->text(), decider()->script_data()->utf16());
+
+  EXPECT_TRUE(decider()->effective_config().has_pac_url());
+  EXPECT_EQ(rule()->url, decider()->effective_config().pac_url());
+}
+
+TEST_F(ProxyScriptDeciderQuickCheckTest, AsyncSuccess) {
+  resolver()->set_ondemand_mode(true);
+  resolver()->rules()->AddRule("wpad", "1.2.3.4");
+
+  EXPECT_EQ(ERR_IO_PENDING, StartDecider());
+  ASSERT_TRUE(resolver()->has_pending_requests());
+  resolver()->ResolveAllPending();
+  // Let the resolutions complete.
+  base::MessageLoop::current()->RunUntilIdle();

[szym, 2013/09/11 14:06:22]

You should callback_.WaitForResult rather than RunUntilIdle.

BTW MessageLoop::RunUntilIdle is deprecated and new code should be using
base::RunLoop().RunUntilIdle()

[Elly Fong-Jones, 2013/09/11 20:20:37]

Oh man, I never knew about WaitForResult! This also lets me write the failure
test I wanted to do - I tried it before with RunUntilIdle(), but it didn't work.

+  EXPECT_FALSE(resolver()->has_pending_requests());
+  EXPECT_EQ(rule()->text(), decider()->script_data()->utf16());
+  EXPECT_TRUE(decider()->effective_config().has_pac_url());
+  EXPECT_EQ(rule()->url, decider()->effective_config().pac_url());
+}
+
+TEST_F(ProxyScriptDeciderQuickCheckTest, QuickCheckAsyncFail) {
+  resolver()->set_ondemand_mode(true);
+  resolver()->rules()->AddSimulatedFailure("wpad");
+  EXPECT_EQ(ERR_IO_PENDING, StartDecider());
+  // let the timer run

[szym, 2013/09/11 14:06:22]

I don't understand the comment. There's barely any time spent here letting
anything happen.

[Elly Fong-Jones, 2013/09/11 20:20:37]

Done.

+  ASSERT_TRUE(resolver()->has_pending_requests());
+  resolver()->ResolveAllPending();
+  base::MessageLoop::current()->RunUntilIdle();
+  EXPECT_FALSE(decider()->effective_config().has_pac_url());
+}
+

[szym, 2013/09/11 14:06:22]

You need a test which exercises the quick check timeout.

I.e., don't call ResolveAllPending, wait for callback with failure.

[Elly Fong-Jones, 2013/09/11 20:20:37]

Done.

 // Fails at WPAD (downloading), but succeeds in choosing the custom PAC.
 TEST(ProxyScriptDeciderTest, AutodetectFailCustomSuccess1) {
   Rules rules;
   RuleBasedProxyScriptFetcher fetcher(&rules);
   DoNothingDhcpProxyScriptFetcher dhcp_fetcher;
 
   ProxyConfig config;
   config.set_auto_detect(true);
   config.set_pac_url(GURL("http://custom/proxy.pac"));
 
@@ skipping 334 matching lines @@
 
   // Run the message loop to let the DHCP fetch complete and post the results
   // back. Before the fix linked to above, this would try to invoke on
   // the callback object provided by ProxyScriptDecider after it was
   // no longer valid.
   base::MessageLoop::current()->RunUntilIdle();
 }
 
 }  // namespace
 }  // namespace net