Remove "Default Address Family" behavior from the HostResolver.

net/dns/host_resolver_impl_unittest.cc

Index: net/dns/host_resolver_impl_unittest.cc
diff --git a/net/dns/host_resolver_impl_unittest.cc b/net/dns/host_resolver_impl_unittest.cc
index 9166350121612230cce81d872c16a364793492ec..2b5c6e3a876728a3fe081b32514108a824813e49 100644
--- a/net/dns/host_resolver_impl_unittest.cc
+++ b/net/dns/host_resolver_impl_unittest.cc
@@ -416,6 +416,17 @@ class LookupAttemptHostResolverProc : public HostResolverProc {
   base::ConditionVariable all_done_;
 };
 
+class TestHostResolverImpl : public HostResolverImpl {

[pauljensen, 2015/06/03 16:26:06]

How about adding a comment mentioning that use of this class prevents IPv6
probing?

[cbentzel, 2015/06/03 22:58:08]

Done.

+ public:
+  TestHostResolverImpl(const Options& options, NetLog* net_log)
+      : HostResolverImpl(options, net_log) {}
+
+  ~TestHostResolverImpl() override {}
+
+ private:
+  bool IsIPv6Reachable(const BoundNetLog& net_log) override { return true; }
+};
+
 }  // namespace
 
 class HostResolverImplTest : public testing::Test {
@@ -474,7 +485,7 @@ class HostResolverImplTest : public testing::Test {
       const HostResolverImpl::ProcTaskParams& params) {
     HostResolverImpl::Options options = DefaultOptions();
     options.max_concurrent_resolves = max_concurrent_resolves;
-    resolver_.reset(new HostResolverImpl(options, NULL));
+    resolver_.reset(new TestHostResolverImpl(options, NULL));
     resolver_->set_proc_params_for_test(params);
   }
 
@@ -854,7 +865,7 @@ TEST_F(HostResolverImplTest, StartWithinCallback) {
   // Turn off caching for this host resolver.
   HostResolver::Options options = DefaultOptions();
   options.enable_caching = false;
-  resolver_.reset(new HostResolverImpl(options, NULL));
+  resolver_.reset(new TestHostResolverImpl(options, NULL));
   resolver_->set_proc_params_for_test(DefaultParams(proc_.get()));
 
   for (size_t i = 0; i < 4; ++i) {
@@ -1176,100 +1187,6 @@ TEST_F(HostResolverImplTest, QueueOverflow) {
   }
 }
 
-// Tests that after changing the default AddressFamily to IPV4, requests
-// with UNSPECIFIED address family map to IPV4.
-TEST_F(HostResolverImplTest, SetDefaultAddressFamily_IPv4) {
-  CreateSerialResolver();  // To guarantee order of resolutions.
-
-  proc_->AddRule("h1", ADDRESS_FAMILY_IPV4, "1.0.0.1");
-  proc_->AddRule("h1", ADDRESS_FAMILY_IPV6, "::2");
-
-  resolver_->SetDefaultAddressFamily(ADDRESS_FAMILY_IPV4);
-
-  CreateRequest("h1", 80, MEDIUM, ADDRESS_FAMILY_UNSPECIFIED);
-  CreateRequest("h1", 80, MEDIUM, ADDRESS_FAMILY_IPV4);
-  CreateRequest("h1", 80, MEDIUM, ADDRESS_FAMILY_IPV6);
-
-  // Start all of the requests.
-  for (size_t i = 0; i < requests_.size(); ++i) {
-    EXPECT_EQ(ERR_IO_PENDING, requests_[i]->Resolve()) << i;
-  }
-
-  proc_->SignalMultiple(requests_.size());
-
-  // Wait for all the requests to complete.
-  for (size_t i = 0u; i < requests_.size(); ++i) {
-    EXPECT_EQ(OK, requests_[i]->WaitForResult()) << i;
-  }
-
-  // Since the requests all had the same priority and we limited the thread
-  // count to 1, they should have completed in the same order as they were
-  // requested. Moreover, request0 and request1 will have been serviced by
-  // the same job.
-
-  MockHostResolverProc::CaptureList capture_list = proc_->GetCaptureList();
-  ASSERT_EQ(2u, capture_list.size());
-
-  EXPECT_EQ("h1", capture_list[0].hostname);
-  EXPECT_EQ(ADDRESS_FAMILY_IPV4, capture_list[0].address_family);
-
-  EXPECT_EQ("h1", capture_list[1].hostname);
-  EXPECT_EQ(ADDRESS_FAMILY_IPV6, capture_list[1].address_family);
-
-  // Now check that the correct resolved IP addresses were returned.
-  EXPECT_TRUE(requests_[0]->HasOneAddress("1.0.0.1", 80));
-  EXPECT_TRUE(requests_[1]->HasOneAddress("1.0.0.1", 80));
-  EXPECT_TRUE(requests_[2]->HasOneAddress("::2", 80));
-}
-
-// This is the exact same test as SetDefaultAddressFamily_IPv4, except the
-// default family is set to IPv6 and the family of requests is flipped where
-// specified.
-TEST_F(HostResolverImplTest, SetDefaultAddressFamily_IPv6) {
-  CreateSerialResolver();  // To guarantee order of resolutions.
-
-  // Don't use IPv6 replacements here since some systems don't support it.
-  proc_->AddRule("h1", ADDRESS_FAMILY_IPV4, "1.0.0.1");
-  proc_->AddRule("h1", ADDRESS_FAMILY_IPV6, "::2");
-
-  resolver_->SetDefaultAddressFamily(ADDRESS_FAMILY_IPV6);
-
-  CreateRequest("h1", 80, MEDIUM, ADDRESS_FAMILY_UNSPECIFIED);
-  CreateRequest("h1", 80, MEDIUM, ADDRESS_FAMILY_IPV6);
-  CreateRequest("h1", 80, MEDIUM, ADDRESS_FAMILY_IPV4);
-
-  // Start all of the requests.
-  for (size_t i = 0; i < requests_.size(); ++i) {
-    EXPECT_EQ(ERR_IO_PENDING, requests_[i]->Resolve()) << i;
-  }
-
-  proc_->SignalMultiple(requests_.size());
-
-  // Wait for all the requests to complete.
-  for (size_t i = 0u; i < requests_.size(); ++i) {
-    EXPECT_EQ(OK, requests_[i]->WaitForResult()) << i;
-  }
-
-  // Since the requests all had the same priority and we limited the thread
-  // count to 1, they should have completed in the same order as they were
-  // requested. Moreover, request0 and request1 will have been serviced by
-  // the same job.
-
-  MockHostResolverProc::CaptureList capture_list = proc_->GetCaptureList();
-  ASSERT_EQ(2u, capture_list.size());
-
-  EXPECT_EQ("h1", capture_list[0].hostname);
-  EXPECT_EQ(ADDRESS_FAMILY_IPV6, capture_list[0].address_family);
-
-  EXPECT_EQ("h1", capture_list[1].hostname);
-  EXPECT_EQ(ADDRESS_FAMILY_IPV4, capture_list[1].address_family);
-
-  // Now check that the correct resolved IP addresses were returned.
-  EXPECT_TRUE(requests_[0]->HasOneAddress("::2", 80));
-  EXPECT_TRUE(requests_[1]->HasOneAddress("::2", 80));
-  EXPECT_TRUE(requests_[2]->HasOneAddress("1.0.0.1", 80));
-}
-
 // Make sure that the address family parameter is respected when raw IPs are
 // passed in.
 TEST_F(HostResolverImplTest, AddressFamilyWithRawIPs) {
@@ -1336,7 +1253,7 @@ TEST_F(HostResolverImplTest, MultipleAttempts) {
   // (500ms * 3).
   params.unresponsive_delay = base::TimeDelta::FromMilliseconds(500);
 
-  resolver_.reset(new HostResolverImpl(DefaultOptions(), NULL));
+  resolver_.reset(new TestHostResolverImpl(DefaultOptions(), NULL));
   resolver_->set_proc_params_for_test(params);
 
   // Resolve "host1".
@@ -1403,6 +1320,8 @@ TEST_F(HostResolverImplTest, NameCollision127_0_53_53) {
 }
 
 TEST_F(HostResolverImplTest, IsIPv6Reachable) {
+  // We need to use the real resolver here.
+  resolver_.reset(new HostResolverImpl(DefaultOptions(), nullptr));
   // Verify that two consecutive calls return the same value.
   TestNetLog net_log;
   BoundNetLog bound_net_log = BoundNetLog::Make(&net_log, NetLog::SOURCE_NONE);
@@ -1492,10 +1411,8 @@ class HostResolverImplDnsTest : public HostResolverImplTest {
       const HostResolverImpl::ProcTaskParams& params) override {
     HostResolverImpl::Options options = DefaultOptions();
     options.max_concurrent_resolves = max_concurrent_resolves;
-    resolver_.reset(new HostResolverImpl(options, NULL));
+    resolver_.reset(new TestHostResolverImpl(options, NULL));
     resolver_->set_proc_params_for_test(params);
-    // Disable IPv6 support probing.
-    resolver_->SetDefaultAddressFamily(ADDRESS_FAMILY_UNSPECIFIED);
     dns_client_ = new MockDnsClient(DnsConfig(), dns_rules_);
     resolver_->SetDnsClient(scoped_ptr<DnsClient>(dns_client_));
   }
@@ -1525,8 +1442,6 @@ class HostResolverImplDnsTest : public HostResolverImplTest {
 
 // Test successful and fallback resolutions in HostResolverImpl::DnsTask.
 TEST_F(HostResolverImplDnsTest, DnsTask) {
-  resolver_->SetDefaultAddressFamily(ADDRESS_FAMILY_IPV4);
-
   proc_->AddRuleForAllFamilies("nx_succeed", "192.168.1.102");
   // All other hostnames will fail in proc_.
 
@@ -1538,9 +1453,12 @@ TEST_F(HostResolverImplDnsTest, DnsTask) {
 
   ChangeDnsConfig(CreateValidDnsConfig());
 
-  EXPECT_EQ(ERR_IO_PENDING, CreateRequest("ok_fail", 80)->Resolve());
-  EXPECT_EQ(ERR_IO_PENDING, CreateRequest("nx_fail", 80)->Resolve());
-  EXPECT_EQ(ERR_IO_PENDING, CreateRequest("nx_succeed", 80)->Resolve());
+  EXPECT_EQ(ERR_IO_PENDING,
+            CreateRequest("ok_fail", 80, IDLE, ADDRESS_FAMILY_IPV4)->Resolve());

[pauljensen, 2015/06/03 16:26:06]

Any particular reason you changed the priority from MEDIUM to IDLE?

[cbentzel, 2015/06/03 22:58:08]

Fewer characters to type. I'll change to MEDIUM since it obviously raised a
question.

+  EXPECT_EQ(ERR_IO_PENDING,
+            CreateRequest("nx_fail", 80, IDLE, ADDRESS_FAMILY_IPV4)->Resolve());
+  EXPECT_EQ(ERR_IO_PENDING, CreateRequest("nx_succeed", 80, IDLE,
+                                          ADDRESS_FAMILY_IPV4)->Resolve());
 
   proc_->SignalMultiple(requests_.size());
 
@@ -1559,7 +1477,6 @@ TEST_F(HostResolverImplDnsTest, DnsTask) {
 // Test successful and failing resolutions in HostResolverImpl::DnsTask when
 // fallback to ProcTask is disabled.
 TEST_F(HostResolverImplDnsTest, NoFallbackToProcTask) {
-  resolver_->SetDefaultAddressFamily(ADDRESS_FAMILY_IPV4);
   set_fallback_to_proctask(false);
 
   proc_->AddRuleForAllFamilies("nx_succeed", "192.168.1.102");
@@ -1579,8 +1496,10 @@ TEST_F(HostResolverImplDnsTest, NoFallbackToProcTask) {
 
   ChangeDnsConfig(CreateValidDnsConfig());
 
-  EXPECT_EQ(ERR_IO_PENDING, CreateRequest("ok_abort", 80)->Resolve());
-  EXPECT_EQ(ERR_IO_PENDING, CreateRequest("nx_abort", 80)->Resolve());
+  EXPECT_EQ(ERR_IO_PENDING, CreateRequest("ok_abort", 80, IDLE,
+                                          ADDRESS_FAMILY_IPV4)->Resolve());
+  EXPECT_EQ(ERR_IO_PENDING, CreateRequest("nx_abort", 80, IDLE,
+                                          ADDRESS_FAMILY_IPV4)->Resolve());
 
   // Simulate the case when the preference or policy has disabled the DNS client
   // causing AbortDnsTasks.
@@ -1590,8 +1509,10 @@ TEST_F(HostResolverImplDnsTest, NoFallbackToProcTask) {
 
   // First request is resolved by MockDnsClient, others should fail due to
   // disabled fallback to ProcTask.
-  EXPECT_EQ(ERR_IO_PENDING, CreateRequest("ok_fail", 80)->Resolve());
-  EXPECT_EQ(ERR_IO_PENDING, CreateRequest("nx_fail", 80)->Resolve());
+  EXPECT_EQ(ERR_IO_PENDING,
+            CreateRequest("ok_fail", 80, IDLE, ADDRESS_FAMILY_IPV4)->Resolve());
+  EXPECT_EQ(ERR_IO_PENDING,
+            CreateRequest("nx_fail", 80, IDLE, ADDRESS_FAMILY_IPV4)->Resolve());
   proc_->SignalMultiple(requests_.size());
 
   // Aborted due to Network Change.
@@ -1606,7 +1527,6 @@ TEST_F(HostResolverImplDnsTest, NoFallbackToProcTask) {
 
 // Test behavior of OnDnsTaskFailure when Job is aborted.
 TEST_F(HostResolverImplDnsTest, OnDnsTaskFailureAbortedJob) {
-  resolver_->SetDefaultAddressFamily(ADDRESS_FAMILY_IPV4);
   ChangeDnsConfig(CreateValidDnsConfig());
   EXPECT_EQ(ERR_IO_PENDING, CreateRequest("nx_abort", 80)->Resolve());
   // Abort all jobs here.
@@ -1618,7 +1538,6 @@ TEST_F(HostResolverImplDnsTest, OnDnsTaskFailureAbortedJob) {
   EXPECT_EQ(ERR_IO_PENDING, requests_[0]->result());
 
   // Repeat test with Fallback to ProcTask disabled
-  resolver_->SetDefaultAddressFamily(ADDRESS_FAMILY_IPV4);
   set_fallback_to_proctask(false);
   ChangeDnsConfig(CreateValidDnsConfig());
   EXPECT_EQ(ERR_IO_PENDING, CreateRequest("nx_abort", 80)->Resolve());
@@ -1823,12 +1742,11 @@ TEST_F(HostResolverImplDnsTest, DontDisableDnsClientOnSporadicFailure) {
 TEST_F(HostResolverImplDnsTest, DualFamilyLocalhost) {
   // Use regular SystemHostResolverCall!
   scoped_refptr<HostResolverProc> proc(new SystemHostResolverProc());
-  resolver_.reset(new HostResolverImpl(DefaultOptions(), NULL));
+  resolver_.reset(new TestHostResolverImpl(DefaultOptions(), NULL));
   resolver_->set_proc_params_for_test(DefaultParams(proc.get()));
 
   resolver_->SetDnsClient(
       scoped_ptr<DnsClient>(new MockDnsClient(DnsConfig(), dns_rules_)));
-  resolver_->SetDefaultAddressFamily(ADDRESS_FAMILY_IPV4);

[pauljensen, 2015/06/03 16:26:06]

By removing this, have you stopped this test from testing the intended behavior?
 I think the behavior being tested relies on a restricted request, at least
according to this comment:
https://code.google.com/p/chromium/codesearch#chromium/src/net/dns/host_resol...

[cbentzel, 2015/06/03 22:58:08]

Great catch. 

I noticed on my machine that localhost only resolved to 127.0.0.1 and most of
the test is skipped. 

I actually want to understand why this test uses SystemHostResolverProc rather
than mocking it out so it will actually run on all platforms. I'll try to get in
touch with szym@ about it.

[cbentzel, 2015/06/04 13:06:42]

OK, I looked at this some more.

The old version of the test acted as if the requests were AF_UNSPECIFIED, but
the HostResolverImpl would choose an effective key of AF_IPV4 and set the host
resolver flag that IPv6 was disabled - and made sure that localhost resolutions
would still give AAAA in this case if the system host resolver did as well.

The first request would go to the system getaddrinfo call.
The second request would be handled out of HOSTS.

By treating IPv6 probes as failing with the TestHostResolverImpl, the second
request is actually handled identically with the new version of the code from
the old version.

But the first version does not go through this check because we treat
"use_local_ipv6_" as true since there is no DnsConfig. So in that case we'd
still keep the AF_UNSPECIFIED behavior on a dual host machine.

Trying to figure out a good workaround for that without introducing runtime
logic.
 

 
   // Get the expected output.
   AddressList addrlist;
@@ -1882,10 +1800,10 @@ TEST_F(HostResolverImplDnsTest, DualFamilyLocalhost) {
 
 // Cancel a request with a single DNS transaction active.
 TEST_F(HostResolverImplDnsTest, CancelWithOneTransactionActive) {
-  resolver_->SetDefaultAddressFamily(ADDRESS_FAMILY_IPV4);
   ChangeDnsConfig(CreateValidDnsConfig());
 
-  EXPECT_EQ(ERR_IO_PENDING, CreateRequest("ok", 80)->Resolve());
+  EXPECT_EQ(ERR_IO_PENDING,
+            CreateRequest("ok", 80, IDLE, ADDRESS_FAMILY_IPV4)->Resolve());
   EXPECT_EQ(1u, num_running_dispatcher_jobs());
   requests_[0]->Cancel();
 
@@ -1895,7 +1813,6 @@ TEST_F(HostResolverImplDnsTest, CancelWithOneTransactionActive) {
 // Cancel a request with a single DNS transaction active and another pending.
 TEST_F(HostResolverImplDnsTest, CancelWithOneTransactionActiveOnePending) {
   CreateSerialResolver();
-  resolver_->SetDefaultAddressFamily(ADDRESS_FAMILY_UNSPECIFIED);
   ChangeDnsConfig(CreateValidDnsConfig());
 
   EXPECT_EQ(ERR_IO_PENDING, CreateRequest("ok", 80)->Resolve());
@@ -1907,7 +1824,6 @@ TEST_F(HostResolverImplDnsTest, CancelWithOneTransactionActiveOnePending) {
 
 // Cancel a request with two DNS transactions active.
 TEST_F(HostResolverImplDnsTest, CancelWithTwoTransactionsActive) {
-  resolver_->SetDefaultAddressFamily(ADDRESS_FAMILY_UNSPECIFIED);
   ChangeDnsConfig(CreateValidDnsConfig());
 
   EXPECT_EQ(ERR_IO_PENDING, CreateRequest("ok", 80)->Resolve());
@@ -1922,7 +1838,6 @@ TEST_F(HostResolverImplDnsTest, DeleteWithActiveTransactions) {
   // At most 10 Jobs active at once.
   CreateResolverWithLimitsAndParams(10u, DefaultParams(proc_.get()));
 
-  resolver_->SetDefaultAddressFamily(ADDRESS_FAMILY_UNSPECIFIED);
   ChangeDnsConfig(CreateValidDnsConfig());
 
   // First active job is an IPv4 request.
@@ -1943,7 +1858,6 @@ TEST_F(HostResolverImplDnsTest, DeleteWithActiveTransactions) {
 
 // Cancel a request with only the IPv6 transaction active.
 TEST_F(HostResolverImplDnsTest, CancelWithIPv6TransactionActive) {
-  resolver_->SetDefaultAddressFamily(ADDRESS_FAMILY_UNSPECIFIED);
   ChangeDnsConfig(CreateValidDnsConfig());
 
   EXPECT_EQ(ERR_IO_PENDING, CreateRequest("6slow_ok", 80)->Resolve());
@@ -1960,7 +1874,6 @@ TEST_F(HostResolverImplDnsTest, CancelWithIPv6TransactionActive) {
 // Cancel a request with only the IPv4 transaction pending.
 TEST_F(HostResolverImplDnsTest, CancelWithIPv4TransactionPending) {
   set_fallback_to_proctask(false);
-  resolver_->SetDefaultAddressFamily(ADDRESS_FAMILY_UNSPECIFIED);
   ChangeDnsConfig(CreateValidDnsConfig());
 
   EXPECT_EQ(ERR_IO_PENDING, CreateRequest("4slow_ok", 80)->Resolve());
@@ -1976,7 +1889,6 @@ TEST_F(HostResolverImplDnsTest, CancelWithIPv4TransactionPending) {
 // Test cases where AAAA completes first.
 TEST_F(HostResolverImplDnsTest, AAAACompletesFirst) {
   set_fallback_to_proctask(false);
-  resolver_->SetDefaultAddressFamily(ADDRESS_FAMILY_UNSPECIFIED);
   ChangeDnsConfig(CreateValidDnsConfig());
 
   EXPECT_EQ(ERR_IO_PENDING, CreateRequest("4slow_ok", 80)->Resolve());
@@ -2014,7 +1926,6 @@ TEST_F(HostResolverImplDnsTest, AAAACompletesFirst) {
 TEST_F(HostResolverImplDnsTest, SerialResolver) {
   CreateSerialResolver();
   set_fallback_to_proctask(false);
-  resolver_->SetDefaultAddressFamily(ADDRESS_FAMILY_UNSPECIFIED);
   ChangeDnsConfig(CreateValidDnsConfig());
 
   EXPECT_EQ(ERR_IO_PENDING, CreateRequest("ok", 80)->Resolve());
@@ -2033,7 +1944,6 @@ TEST_F(HostResolverImplDnsTest, SerialResolver) {
 TEST_F(HostResolverImplDnsTest, AAAAStartsAfterOtherJobFinishes) {
   CreateResolverWithLimitsAndParams(2u, DefaultParams(proc_.get()));
   set_fallback_to_proctask(false);
-  resolver_->SetDefaultAddressFamily(ADDRESS_FAMILY_UNSPECIFIED);
   ChangeDnsConfig(CreateValidDnsConfig());
 
   EXPECT_EQ(ERR_IO_PENDING, CreateRequest("ok", 80, MEDIUM,
@@ -2092,7 +2002,6 @@ TEST_F(HostResolverImplDnsTest, InvalidDnsConfigWithPendingRequests) {
   // prioritized dispatcher slot.
   CreateResolverWithLimitsAndParams(3u, DefaultParams(proc_.get()));
 
-  resolver_->SetDefaultAddressFamily(ADDRESS_FAMILY_UNSPECIFIED);
   ChangeDnsConfig(CreateValidDnsConfig());
 
   proc_->AddRuleForAllFamilies("slow_nx1", "192.168.0.1");
@@ -2133,7 +2042,6 @@ TEST_F(HostResolverImplDnsTest,
   for (size_t limit = 1u; limit < 6u; ++limit) {
     CreateResolverWithLimitsAndParams(limit, DefaultParams(proc_.get()));
 
-    resolver_->SetDefaultAddressFamily(ADDRESS_FAMILY_UNSPECIFIED);
     ChangeDnsConfig(CreateValidDnsConfig());
 
     // Queue up enough failures to disable DnsTasks.  These will all fall back
@@ -2180,7 +2088,6 @@ TEST_F(HostResolverImplDnsTest, ManuallyDisableDnsClientWithPendingRequests) {
   // prioritized dispatcher slot.
   CreateResolverWithLimitsAndParams(3u, DefaultParams(proc_.get()));
 
-  resolver_->SetDefaultAddressFamily(ADDRESS_FAMILY_UNSPECIFIED);
   ChangeDnsConfig(CreateValidDnsConfig());
 
   proc_->AddRuleForAllFamilies("slow_ok1", "192.168.0.1");