[net/dns] Serve requests from HOSTS file if possible.

net/base/host_resolver_impl_unittest.cc

Index: net/base/host_resolver_impl_unittest.cc
diff --git a/net/base/host_resolver_impl_unittest.cc b/net/base/host_resolver_impl_unittest.cc
index b99dbf7b064d8ad818ce8bc13d555039eba9ddff..e49d4f295e85058f09be828548e6d7d8c686f0b8 100644
--- a/net/base/host_resolver_impl_unittest.cc
+++ b/net/base/host_resolver_impl_unittest.cc
@@ -28,15 +28,18 @@
 #include "net/base/net_util.h"
 #include "net/base/sys_addrinfo.h"
 #include "net/base/test_completion_callback.h"
+#include "net/dns/dns_client.h"
+#include "net/dns/dns_test_util.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace net {
+namespace {
 
 using base::TimeDelta;
 using base::TimeTicks;
 
-static const size_t kMaxJobs = 10u;
-static const size_t kMaxRetryAttempts = 4u;
+const size_t kMaxJobs = 10u;
+const size_t kMaxRetryAttempts = 4u;
 
 PrioritizedDispatcher::Limits DefaultLimits() {
   PrioritizedDispatcher::Limits limits(NUM_PRIORITIES, kMaxJobs);
@@ -58,6 +61,17 @@ HostResolverImpl* CreateHostResolverImpl(HostResolverProc* resolver_proc) {
       NULL);
 }
 
+HostResolverImpl* CreateHostResolverImplWithDns(

[mmenke, 2012/03/13 15:56:27]

nit:  Suggest "CreateHostResolverImplWithDnsConfig" or
"CreateHostResolverImplWithDirectDns".

+    HostResolverProc* resolver_proc,
+    scoped_ptr<DnsConfigService> config_service) {
+  return new HostResolverImpl(
+      HostCache::CreateDefaultCache(),
+      DefaultLimits(),
+      DefaultParams(resolver_proc),
+      config_service.Pass(),
+      NULL);
+}
+
 // This HostResolverImpl will only allow 1 outstanding resolve at a time.
 HostResolverImpl* CreateSerialHostResolverImpl(
     HostResolverProc* resolver_proc) {
@@ -425,6 +439,20 @@ class ResolveRequest {
     EXPECT_EQ(ERR_IO_PENDING, err);
   }
 
+  ResolveRequest(HostResolver* resolver,

[mmenke, 2012/03/13 15:56:27]

Think you should comment that this is only for synchronous requests that should
be served from the cache or HOSTS file.

+                 const std::string& hostname,
+                 int port)
+      : info_(HostPortPair(hostname, port)),
+        resolver_(resolver),
+        delegate_(NULL)  {
+    // Start the request.
+    result_ = resolver->Resolve(
+        info_, &addrlist_,
+        base::Bind(&ResolveRequest::OnLookupFinished, base::Unretained(this)),
+        &req_, BoundNetLog());
+    EXPECT_NE(ERR_IO_PENDING, result_);
+  }
+
   void Cancel() {
     resolver_->CancelRequest(req_);
   }
@@ -451,6 +479,9 @@ class ResolveRequest {
 
  private:
   void OnLookupFinished(int result) {
+    EXPECT_TRUE(delegate_ != NULL);
+    if (delegate_ == NULL)
+      return;
     result_ = result;
     delegate_->OnCompleted(this);
   }
@@ -870,7 +901,8 @@ TEST_F(HostResolverImplTest, CancelMultipleRequests) {
   MessageLoop::current()->Run();
 }
 
-// Helper class used by HostResolverImplTest.CanceledRequestsReleaseJobSlots.
+// Delegate which allows to wait for specific number of requests to complete.
+// Used by HostResolverImplTest.CanceledRequestsReleaseJobSlots and .DnsTask.
 class CountingDelegate : public ResolveRequest::Delegate {
  public:
   CountingDelegate() : num_completions_(0) {}
@@ -882,6 +914,11 @@ class CountingDelegate : public ResolveRequest::Delegate {
 
   unsigned num_completions() const { return num_completions_; }
 
+  void WaitUntil(unsigned completions) {

[mmenke, 2012/03/13 15:56:27]

optional nit:  Suggest "WaitFor" or "WaitForCompletion".

+    while (num_completions_ < completions)
+      MessageLoop::current()->Run();

[mmenke, 2012/03/13 15:56:27]

Rather than repeatedly entering message loops again, it's preferred just to quit
the message loop once when num_completions_ == completions.  It tends to make
for less flaky tests, or so the theory goes.

[szym, 2012/03/13 18:14:29]

I think it's the other way around: because anyone can Quit the MessageLoop, I
will have to have while statement around Run() anyway. It's true that
OnCompleted could be smarter and not Quit the loop until there's enough
completions, although that would require adding new fields and arguments.

[mmenke, 2012/03/13 18:27:50]

You don't want anyone else exiting the message loop.  That's actually one of the
reasons for having a single call to quit.  If someone else exits the loop, you
know there's a bug somewhere, assuming you add an EXPECT statement here as well.

+  }
+
  private:
   unsigned num_completions_;
 };
@@ -918,8 +955,7 @@ TEST_F(HostResolverImplTest, CanceledRequestsReleaseJobSlots) {
 
   resolver_proc->SignalAll();
 
-  while (delegate.num_completions() < 2)
-    MessageLoop::current()->Run();
+  delegate.WaitUntil(2);
 
   EXPECT_EQ(0u, host_resolver->num_running_jobs_for_tests());
 }
@@ -1750,6 +1786,101 @@ TEST_F(HostResolverImplTest, MultipleAttempts) {
   EXPECT_EQ(resolver_proc->resolved_attempt_number(), kAttemptNumberToResolve);
 }
 
+DnsConfig CreateValidDnsConfig() {
+  IPAddressNumber dns_ip;
+  bool rv = ParseIPLiteralToNumber("192.168.1.0", &dns_ip);
+  EXPECT_TRUE(rv);
+
+  DnsConfig config;
+  config.nameservers.push_back(IPEndPoint(dns_ip,
+                                          dns_protocol::kDefaultPort));
+  EXPECT_TRUE(config.IsValid());
+  return config;
+}
+
+// Test successful and fallback resolutions in HostResolverImpl::DnsTask.
+TEST_F(HostResolverImplTest, DnsTask) {
+  scoped_refptr<RuleBasedHostResolverProc> resolver_proc(
+      new RuleBasedHostResolverProc(NULL));
+  scoped_ptr<HostResolverImpl> host_resolver(CreateHostResolverImpl(
+      resolver_proc));
+
+  resolver_proc->AddRule("er_succeed", "192.168.1.101");
+  resolver_proc->AddRule("nx_succeed", "192.168.1.102");
+  resolver_proc->AddSimulatedFailure("ok_fail");
+  resolver_proc->AddSimulatedFailure("er_fail");
+  resolver_proc->AddSimulatedFailure("nx_fail");
+
+  CountingDelegate delegate;
+
+  // Initially there is no config, so client should not be invoked.
+  ResolveRequest req1(host_resolver.get(), "ok_fail", 80, &delegate);
+
+  delegate.WaitUntil(1);
+  EXPECT_EQ(ERR_NAME_NOT_RESOLVED, req1.result());
+
+  host_resolver->set_dns_client_for_tests(
+      CreateMockDnsClient(CreateValidDnsConfig()));
+
+  ResolveRequest req2(host_resolver.get(), "ok_fail", 80, &delegate);
+  ResolveRequest req3(host_resolver.get(), "er_fail", 80, &delegate);
+  ResolveRequest req4(host_resolver.get(), "nx_fail", 80, &delegate);
+  ResolveRequest req5(host_resolver.get(), "er_succeed", 80, &delegate);
+  ResolveRequest req6(host_resolver.get(), "nx_succeed", 80, &delegate);
+
+  delegate.WaitUntil(6);
+  EXPECT_EQ(OK, req2.result());

[mmenke, 2012/03/13 15:56:27]

Should the OK checks be ASSERTs to prevent crashing on regressions?

+  // Resolved by MockDnsClient.
+  EXPECT_EQ("127.0.0.1", NetAddressToString(req2.addrlist().head()));
+  EXPECT_EQ(ERR_NAME_NOT_RESOLVED, req3.result());
+  EXPECT_EQ(ERR_NAME_NOT_RESOLVED, req4.result());
+  EXPECT_EQ(OK, req5.result());
+  EXPECT_EQ("192.168.1.101", NetAddressToString(req5.addrlist().head()));
+  EXPECT_EQ(OK, req6.result());
+  EXPECT_EQ("192.168.1.102", NetAddressToString(req6.addrlist().head()));
+}
+
+TEST_F(HostResolverImplTest, ServeFromHosts) {
+  scoped_refptr<RuleBasedHostResolverProc> resolver_proc(
+      new RuleBasedHostResolverProc(NULL));
+  MockDnsConfigService* config_service = new MockDnsConfigService();
+  scoped_ptr<HostResolverImpl> host_resolver(CreateHostResolverImplWithDns(
+      resolver_proc,
+      scoped_ptr<DnsConfigService>(config_service)));
+
+  resolver_proc->AddSimulatedFailure("*");
+
+  DnsConfig config = CreateValidDnsConfig();
+  host_resolver->set_dns_client_for_tests(CreateMockDnsClient(config));
+
+  CountingDelegate delegate;
+
+  ResolveRequest req1(host_resolver.get(), "er_ipv4", 80, &delegate);
+  delegate.WaitUntil(1);
+  EXPECT_EQ(ERR_NAME_NOT_RESOLVED, req1.result());
+
+  IPAddressNumber local_ipv4, local_ipv6;
+  ASSERT_TRUE(ParseIPLiteralToNumber("127.0.0.1", &local_ipv4));
+  ASSERT_TRUE(ParseIPLiteralToNumber("::1", &local_ipv6));
+
+  DnsHosts hosts;
+  hosts[DnsHostsKey("er_ipv4", ADDRESS_FAMILY_IPV4)] = local_ipv4;
+  hosts[DnsHostsKey("er_ipv6", ADDRESS_FAMILY_IPV6)] = local_ipv6;

[mmenke, 2012/03/13 15:56:27]

Suggest you add two or three ADDRESS_FAMILY_UNSPEC here as well - one with ipv6
address only (like "er_ipv6"), and one with ipv4 and ipv6 addresses.  Maybe also
one with ipv4 only.

[szym, 2012/03/13 18:14:29]

DnsHosts will not have keys with ADDRESS_FAMILY_UNSPEC, unless we update
dns_hosts.cc to do that (which would increase the size of DnsHosts between 1.5x
and 2x).

[mmenke, 2012/03/13 18:27:50]

Sorry, I was a little confused...  You should have a hostname with ipv6 and ipv4
IPs, and make sure you only get the IPv4 one, as per
HostResolverImpl::ServeFromHosts.

May also be a good idea to run a request for a ADDRESS_FAMILY_IPV4 address only
(Instead of UNSPECIFIED), just for the sake of completeness.

[szym, 2012/03/13 19:04:00]

I don't want to test the IPv4 preference unless we specify that explicitly (and
therefore close the respective bug). A test that requires such behavior would
indicate that this is "how it's supposed to be" which would be detrimental in
future development.

+
+  config_service->ChangeConfig(config);
+  config_service->ChangeHosts(hosts);
+
+  ResolveRequest req2(host_resolver.get(), "er_ipv4", 80);
+  EXPECT_EQ(OK, req2.result());
+  EXPECT_EQ("127.0.0.1", NetAddressToString(req2.addrlist().head()));
+  ResolveRequest req3(host_resolver.get(), "er_ipv6", 80);
+  EXPECT_EQ(OK, req3.result());
+  EXPECT_EQ("::1", NetAddressToString(req3.addrlist().head()));
+}
+
+// TODO(szym): Test AbortAllInProgressJobs due to DnsConfig change.
+
 // TODO(cbentzel): Test a mix of requests with different HostResolverFlags.
 
+}  // namespace
 }  // namespace net