[net/dns] Turn DnsConfigService on by default.

R=mmenke@chromium.org
BUG=125599
TEST=net_unittests


Review URL: https://chromiumcodereview.appspot.com/10334009

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

4 files changed, 89 insertions, 58 deletions

diff --git a/net/base/host_resolver.h b/net/base/host_resolver.h
index b199491..76ae779 100644
--- a/net/base/host_resolver.h
+++ b/net/base/host_resolver.h
@@ -185,6 +185,10 @@ class NET_EXPORT HostResolver {
 // |max_retry_attempts| is the maximum number of times we will retry for host
 // resolution. Pass HostResolver::kDefaultRetryAttempts to choose a default
 // value.
+// The created HostResolver uses an instance of DnsConfigService to retrieve
+// system DNS configuration.
+// This resolver should not be used in test context. Instead, use
+// MockHostResolver from net/base/mock_host_resolver.h.
 NET_EXPORT HostResolver* CreateSystemHostResolver(
     size_t max_concurrent_resolves,
     size_t max_retry_attempts,
diff --git a/net/base/host_resolver_impl.cc b/net/base/host_resolver_impl.cc
index d2d9d5d..a66f950 100644
--- a/net/base/host_resolver_impl.cc
+++ b/net/base/host_resolver_impl.cc
@@ -147,6 +147,8 @@ void UmaAsyncDnsResolveStatus(DnsResolveStatus result) {
                             RESOLVE_STATUS_MAX);
 }
 
+//-----------------------------------------------------------------------------
+
 // Wraps call to SystemHostResolverProc as an instance of HostResolverProc.
 // TODO(szym): This should probably be declared in host_resolver_proc.h.
 class CallSystemHostResolverProc : public HostResolverProc {
@@ -435,6 +437,7 @@ HostResolver* CreateHostResolver(size_t max_concurrent_resolves,
                                  size_t max_retry_attempts,
                                  HostCache* cache,
                                  scoped_ptr<DnsConfigService> config_service,
+                                 scoped_ptr<DnsClient> dns_client,
                                  NetLog* net_log) {
   if (max_concurrent_resolves == HostResolver::kDefaultParallelism)
     max_concurrent_resolves = kDefaultMaxProcTasks;
@@ -449,6 +452,7 @@ HostResolver* CreateHostResolver(size_t max_concurrent_resolves,
       limits,
       HostResolverImpl::ProcTaskParams(NULL, max_retry_attempts),
       config_service.Pass(),
+      dns_client.Pass(),
       net_log);
 
   return resolver;
@@ -464,7 +468,8 @@ HostResolver* CreateSystemHostResolver(size_t max_concurrent_resolves,
   return CreateHostResolver(max_concurrent_resolves,
                             max_retry_attempts,
                             HostCache::CreateDefaultCache(),
-                            scoped_ptr<DnsConfigService>(NULL),
+                            DnsConfigService::CreateSystemService(),
+                            scoped_ptr<DnsClient>(NULL),
                             net_log);
 }
 
@@ -475,18 +480,18 @@ HostResolver* CreateNonCachingSystemHostResolver(size_t max_concurrent_resolves,
                             max_retry_attempts,
                             NULL,
                             scoped_ptr<DnsConfigService>(NULL),
+                            scoped_ptr<DnsClient>(NULL),
                             net_log);
 }
 
 HostResolver* CreateAsyncHostResolver(size_t max_concurrent_resolves,
                                       size_t max_retry_attempts,
                                       NetLog* net_log) {
-  scoped_ptr<DnsConfigService> config_service =
-      DnsConfigService::CreateSystemService();
   return CreateHostResolver(max_concurrent_resolves,
                             max_retry_attempts,
                             HostCache::CreateDefaultCache(),
-                            config_service.Pass(),
+                            DnsConfigService::CreateSystemService(),
+                            DnsClient::CreateClient(net_log),
                             net_log);
 }
 
@@ -1424,8 +1429,14 @@ class HostResolverImpl::Job : public PrioritizedDispatcher::Job {
 
     DCHECK(!requests_.empty());
 
-    if (net_error == OK)
+    if (net_error == OK) {
       SetPortOnAddressList(requests_->front()->info().port(), &list);
+      // Record this histogram here, when we know the system has a valid DNS
+      // configuration.
+      UMA_HISTOGRAM_ENUMERATION("AsyncDNS.HaveDnsConfig",
+                                resolver_->received_dns_config_ ? 1 : 0,
+                                2);
+    }
 
     if ((net_error != ERR_ABORTED) &&
         (net_error != ERR_HOST_RESOLVER_QUEUE_TOO_LARGE)) {
@@ -1516,14 +1527,16 @@ HostResolverImpl::HostResolverImpl(
     const PrioritizedDispatcher::Limits& job_limits,
     const ProcTaskParams& proc_params,
     scoped_ptr<DnsConfigService> dns_config_service,
+    scoped_ptr<DnsClient> dns_client,
     NetLog* net_log)
     : cache_(cache),
       dispatcher_(job_limits),
       max_queued_jobs_(job_limits.total_jobs * 100u),
       proc_params_(proc_params),
       default_address_family_(ADDRESS_FAMILY_UNSPECIFIED),
-      dns_client_(NULL),
       dns_config_service_(dns_config_service.Pass()),
+      dns_client_(dns_client.Pass()),
+      received_dns_config_(false),
       ipv6_probe_monitoring_(false),
       additional_resolver_flags_(0),
       net_log_(net_log) {
@@ -1544,18 +1557,16 @@ HostResolverImpl::HostResolverImpl(
     additional_resolver_flags_ |= HOST_RESOLVER_LOOPBACK_ONLY;
 #endif
   NetworkChangeNotifier::AddIPAddressObserver(this);
-#if defined(OS_POSIX) && !defined(OS_MACOSX) && !defined(OS_OPENBSD)
-#if !defined(OS_ANDROID)
-  EnsureDnsReloaderInit();
-#endif
+#if defined(OS_POSIX) && !defined(OS_MACOSX) && !defined(OS_OPENBSD) && \
+    !defined(OS_ANDROID)
   NetworkChangeNotifier::AddDNSObserver(this);
+  EnsureDnsReloaderInit();
 #endif
 
   if (dns_config_service_.get()) {
     dns_config_service_->Watch(
         base::Bind(&HostResolverImpl::OnDnsConfigChanged,
                    base::Unretained(this)));
-    dns_client_ = DnsClient::CreateClient(net_log_);
   }
 }
 
@@ -1566,9 +1577,7 @@ HostResolverImpl::~HostResolverImpl() {
   STLDeleteValues(&jobs_);
 
   NetworkChangeNotifier::RemoveIPAddressObserver(this);
-#if defined(OS_POSIX) && !defined(OS_MACOSX) && !defined(OS_OPENBSD)
   NetworkChangeNotifier::RemoveDNSObserver(this);
-#endif
 }
 
 void HostResolverImpl::SetMaxQueuedJobs(size_t value) {
@@ -1931,6 +1940,12 @@ void HostResolverImpl::OnIPAddressChanged() {
 }
 
 void HostResolverImpl::OnDNSChanged(unsigned detail) {
+  // Ignore signals about watches.
+  const unsigned kIgnoredDetail =
+      NetworkChangeNotifier::CHANGE_DNS_WATCH_STARTED |
+      NetworkChangeNotifier::CHANGE_DNS_WATCH_FAILED;
+  if ((detail & ~kIgnoredDetail) == 0)
+    return;
   // If the DNS server has changed, existing cached info could be wrong so we
   // have to drop our internal cache :( Note that OS level DNS caches, such
   // as NSCD's cache should be dropped automatically by the OS when
@@ -1938,7 +1953,7 @@ void HostResolverImpl::OnDNSChanged(unsigned detail) {
   if (cache_.get())
     cache_->clear();
   // Existing jobs will have been sent to the original server so they need to
-  // be aborted. TODO(Craig): Should these jobs be restarted?
+  // be aborted.
   AbortAllInProgressJobs();
   // |this| may be deleted inside AbortAllInProgressJobs().
 }
@@ -1950,28 +1965,21 @@ void HostResolverImpl::OnDnsConfigChanged(const DnsConfig& dns_config) {
         make_scoped_refptr(new DnsConfigParameters(dns_config)));
   }
 
-  DCHECK(dns_client_.get());
+  // TODO(szym): Remove once http://crbug.com/125599 is resolved.
+  received_dns_config_ = dns_config.IsValid();
 
   // Life check to bail once |this| is deleted.
   base::WeakPtr<HostResolverImpl> self = AsWeakPtr();
 
-  bool config_changed = (dns_client_->GetConfig() != NULL) &&
-      !dns_config.EqualsIgnoreHosts(*dns_client_->GetConfig());
-
-  // We want a new factory in place, before we Abort running Jobs, so that the
-  // newly started jobs use the new factory.
-  dns_client_->SetConfig(dns_config);
-
-  // Don't Abort running Jobs unless they were running on DnsTransaction and
-  // DnsConfig changed beyond DnsHosts. HOSTS-only change will be resolved by
-  // TryServingAllJobsFromHosts below.
-  if (config_changed) {
-    // TODO(szym): This will change once http://crbug.com/114827 is fixed.
+  if (dns_client_.get()) {
+    // We want a new factory in place, before we Abort running Jobs, so that the
+    // newly started jobs use the new factory.
+    dns_client_->SetConfig(dns_config);
     OnDNSChanged(NetworkChangeNotifier::CHANGE_DNS_SETTINGS);
+    // |this| may be deleted inside OnDNSChanged().
+    if (self)
+      TryServingAllJobsFromHosts();
   }
-
-  if (self && dns_config.IsValid())
-    TryServingAllJobsFromHosts();
 }
 
 bool HostResolverImpl::HaveDnsConfig() const {
diff --git a/net/base/host_resolver_impl.h b/net/base/host_resolver_impl.h
index 78d1563..4918208 100644
--- a/net/base/host_resolver_impl.h
+++ b/net/base/host_resolver_impl.h
@@ -109,8 +109,10 @@ class NET_EXPORT HostResolverImpl
   // run at once. This upper-bounds the total number of outstanding
   // DNS transactions (not counting retransmissions and retries).
   //
-  // |dns_config_service| will be used to obtain DnsConfig for
-  // DnsTransactionFactory.
+  // |dns_config_service| will be used to detect changes to DNS configuration
+  // and obtain DnsConfig for DnsClient.
+  //
+  // |dns_client|, if set, will be used to resolve requests.
   //
   // |net_log| must remain valid for the life of the HostResolverImpl.
   // TODO(szym): change to scoped_ptr<HostCache>.
@@ -118,6 +120,7 @@ class NET_EXPORT HostResolverImpl
                    const PrioritizedDispatcher::Limits& job_limits,
                    const ProcTaskParams& proc_params,
                    scoped_ptr<DnsConfigService> dns_config_service,
+                   scoped_ptr<DnsClient> dns_client,
                    NetLog* net_log);
 
   // If any completion callbacks are pending when the resolver is destroyed,
@@ -156,10 +159,6 @@ class NET_EXPORT HostResolverImpl
   typedef std::map<Key, Job*> JobMap;
   typedef ScopedVector<Request> RequestsList;
 
-  void set_dns_client_for_tests(scoped_ptr<DnsClient> client) {
-    dns_client_ = client.Pass();
-  }
-
   // Helper used by |Resolve()| and |ResolveFromCache()|.  Performs IP
   // literal, cache and HOSTS lookup (if enabled), returns OK if successful,
   // ERR_NAME_NOT_RESOLVED if either hostname is invalid or IP literal is
@@ -184,8 +183,8 @@ class NET_EXPORT HostResolverImpl
                       int* net_error,
                       AddressList* addresses);
 
-  // If |key| is not found in the HOSTS file or no HOSTS file known, returns
-  // false, otherwise returns true and fills |addresses|.
+  // If we have a DnsClient with a valid DnsConfig, and |key| is found in the
+  // HOSTS file, returns true and fills |addresses|. Otherwise returns false.
   bool ServeFromHosts(const Key& key,
                       const RequestInfo& info,
                       AddressList* addresses);
@@ -214,7 +213,8 @@ class NET_EXPORT HostResolverImpl
   // Might start new jobs.
   void AbortAllInProgressJobs();
 
-  // Attempts to serve each Job in |jobs_| from the HOSTS file.
+  // Attempts to serve each Job in |jobs_| from the HOSTS file if we have
+  // a DnsClient with a valid DnsConfig.
   void TryServingAllJobsFromHosts();
 
   // NetworkChangeNotifier::IPAddressObserver:
@@ -226,10 +226,10 @@ class NET_EXPORT HostResolverImpl
   // DnsConfigService callback:
   void OnDnsConfigChanged(const DnsConfig& dns_config);
 
-  // True if have fully configured DNS client.
+  // True if have a DnsClient with a valid DnsConfig.
   bool HaveDnsConfig() const;
-  // Allows the tests to catch slots leaking out of the dispatcher.
 
+  // Allows the tests to catch slots leaking out of the dispatcher.
   size_t num_running_jobs_for_tests() const {
     return dispatcher_.num_running_jobs();
   }
@@ -252,12 +252,17 @@ class NET_EXPORT HostResolverImpl
   // Address family to use when the request doesn't specify one.
   AddressFamily default_address_family_;
 
-  scoped_ptr<DnsClient> dns_client_;
   scoped_ptr<DnsConfigService> dns_config_service_;
 
+  // If present, used by DnsTask and ServeFromHosts to resolve requests.
+  scoped_ptr<DnsClient> dns_client_;
+
+  // True if received valid config from |dns_config_service_|. Temporary, used
+  // to measure performance of DnsConfigService: http://crbug.com/125599
+  bool received_dns_config_;
+
   // Indicate if probing is done after each network change event to set address
-  // family.
-  // When false, explicit setting of address family is used.
+  // family. When false, explicit setting of address family is used.
   bool ipv6_probe_monitoring_;
 
   // The last un-cancelled IPv6ProbeJob (if any).
diff --git a/net/base/host_resolver_impl_unittest.cc b/net/base/host_resolver_impl_unittest.cc
index fa88f78..60d6ed1 100644
--- a/net/base/host_resolver_impl_unittest.cc
+++ b/net/base/host_resolver_impl_unittest.cc
@@ -49,17 +49,20 @@ HostResolverImpl* CreateHostResolverImpl(HostResolverProc* resolver_proc) {
       DefaultLimits(),
       DefaultParams(resolver_proc),
       scoped_ptr<DnsConfigService>(NULL),
+      scoped_ptr<DnsClient>(NULL),
       NULL);
 }
 
-HostResolverImpl* CreateHostResolverImplWithDnsConfig(
+HostResolverImpl* CreateHostResolverImplWithDnsClient(
     HostResolverProc* resolver_proc,
-    scoped_ptr<DnsConfigService> config_service) {
+    scoped_ptr<DnsConfigService> dns_config_service) {
+  // Initially with empty DnsConfig. Use |dns_config_service| to update it.
   return new HostResolverImpl(
       HostCache::CreateDefaultCache(),
       DefaultLimits(),
       DefaultParams(resolver_proc),
-      config_service.Pass(),
+      dns_config_service.Pass(),
+      CreateMockDnsClient(DnsConfig()),
       NULL);
 }
 
@@ -76,6 +79,7 @@ HostResolverImpl* CreateSerialHostResolverImpl(
       limits,
       params,
       scoped_ptr<DnsConfigService>(NULL),
+      scoped_ptr<DnsClient>(NULL),
       NULL);
 }
 
@@ -523,10 +527,6 @@ class HostResolverImplTest : public testing::Test {
     return resolver_->num_running_jobs_for_tests();
   }
 
-  void set_dns_client(scoped_ptr<DnsClient> client) {
-    resolver_->set_dns_client_for_tests(client.Pass());
-  }
-
   scoped_refptr<MockHostResolverProc> proc_;
   scoped_ptr<HostResolverImpl> resolver_;
   ScopedVector<Request> requests_;
@@ -782,6 +782,7 @@ TEST_F(HostResolverImplTest, StartWithinCallback) {
       DefaultLimits(),
       DefaultParams(proc_),
       scoped_ptr<DnsConfigService>(NULL),
+      scoped_ptr<DnsClient>(NULL),
       NULL));
 
   for (size_t i = 0; i < 4; ++i) {
@@ -1219,6 +1220,7 @@ TEST_F(HostResolverImplTest, MultipleAttempts) {
                            DefaultLimits(),
                            params,
                            scoped_ptr<DnsConfigService>(NULL),
+                           scoped_ptr<DnsClient>(NULL),
                            NULL));
 
   // Resolve "host1".
@@ -1254,6 +1256,14 @@ DnsConfig CreateValidDnsConfig() {
 
 // Test successful and fallback resolutions in HostResolverImpl::DnsTask.
 TEST_F(HostResolverImplTest, DnsTask) {
+  // Initially, there's DnsConfigService, but no DnsConfig.
+  // Note, |config_service| will be owned by |resolver_|.
+  MockDnsConfigService* config_service = new MockDnsConfigService();
+  resolver_.reset(
+      CreateHostResolverImplWithDnsClient(
+          proc_,
+          scoped_ptr<DnsConfigService>(config_service)));
+
   proc_->AddRuleForAllFamilies("er_succeed", "192.168.1.101");
   proc_->AddRuleForAllFamilies("nx_succeed", "192.168.1.102");
   // All other hostnames will fail in proc_.
@@ -1264,7 +1274,9 @@ TEST_F(HostResolverImplTest, DnsTask) {
 
   EXPECT_EQ(ERR_NAME_NOT_RESOLVED, requests_[0]->WaitForResult());
 
-  set_dns_client(CreateMockDnsClient(CreateValidDnsConfig()));
+  DnsConfig config = CreateValidDnsConfig();
+  config_service->ChangeHosts(config.hosts);
+  config_service->ChangeConfig(config);
 
   EXPECT_EQ(ERR_IO_PENDING, CreateRequest("ok_fail", 80)->Resolve());
   EXPECT_EQ(ERR_IO_PENDING, CreateRequest("er_fail", 80)->Resolve());
@@ -1290,19 +1302,21 @@ TEST_F(HostResolverImplTest, DnsTask) {
 }
 
 TEST_F(HostResolverImplTest, ServeFromHosts) {
-  // Initially, there's DnsConfigService, but no DnsConfig.
+  // Note, |config_service| will be owned by |resolver_|.
   MockDnsConfigService* config_service = new MockDnsConfigService();
   resolver_.reset(
-      CreateHostResolverImplWithDnsConfig(
+      CreateHostResolverImplWithDnsClient(
           proc_,
           scoped_ptr<DnsConfigService>(config_service)));
 
+  // Initially, use empty HOSTS file.
+  DnsConfig config = CreateValidDnsConfig();
+  config_service->ChangeHosts(DnsHosts());
+  config_service->ChangeConfig(config);
+
   proc_->AddRuleForAllFamilies("", "0.0.0.0");  // Default to failures.
   proc_->SignalMultiple(1u);  // For the first request which misses.
 
-  DnsConfig config = CreateValidDnsConfig();
-  set_dns_client(CreateMockDnsClient(config));
-
   Request* req0 = CreateRequest("er_ipv4", 80);
   EXPECT_EQ(ERR_IO_PENDING, req0->Resolve());
   EXPECT_EQ(ERR_NAME_NOT_RESOLVED, req0->WaitForResult());
@@ -1317,7 +1331,7 @@ TEST_F(HostResolverImplTest, ServeFromHosts) {
   hosts[DnsHostsKey("er_both", ADDRESS_FAMILY_IPV4)] = local_ipv4;
   hosts[DnsHostsKey("er_both", ADDRESS_FAMILY_IPV6)] = local_ipv6;
 
-  // Then we introduce valid DnsConfig.
+  // Update HOSTS file.
   config_service->ChangeConfig(config);
   config_service->ChangeHosts(hosts);