HostResolverImpl will return ERR_NAME_NOT_RESOLVED instead of Ok if address list is empty

net/dns/host_resolver_impl.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 "net/dns/host_resolver_impl.h"
 
 #if defined(OS_WIN)
 #include <Winsock2.h>
 #elif defined(OS_POSIX)
 #include <netdb.h>
@@ skipping 664 matching lines @@
     StartLookupAttempt();
   }
 
   // Callback for when DoLookup() completes (runs on origin thread).
   void OnLookupComplete(const AddressList& results,
                         const base::TimeTicks& start_time,
                         const uint32 attempt_number,
                         int error,
                         const int os_error) {
     DCHECK(origin_loop_->BelongsToCurrentThread());
-    DCHECK(error || !results.empty());

[szym, 2013/05/04 07:46:32]

Ok, hold on a second. If this DCHECK is here, then this means getaddrinfo never
gives us an empty address list and error == OK.

Now, DCHECKs are disabled in release builds, so maybe we're just not catching
it.

+    // empty results are not OK

[szym, 2013/05/04 07:46:32]

nit: Comments should be full sentences, with capital letter and full stop at
end.

[mef, 2013/05/06 16:49:11]

Done.

+    if (error == OK && results.empty())
+      error = ERR_NAME_NOT_RESOLVED;

[szym, 2013/05/04 07:46:32]

If you change |error| here then your |empty_list_on_ok| below will always be
false. (OnLookupComplete -> OnProcTaskComplete)

I'd suggest keeping it here, but also histogramming it here.

[mef, 2013/05/06 16:49:11]

Done.

 
     bool was_retry_attempt = attempt_number > 1;
 
     // Ideally the following code would be part of host_resolver_proc.cc,
     // however it isn't safe to call NetworkChangeNotifier from worker threads.
     // So we do it here on the IO thread instead.
     if (error != OK && NetworkChangeNotifier::IsOffline())
       error = ERR_INTERNET_DISCONNECTED;
 
     // If this is the first attempt that is finishing later, then record data
@@ skipping 734 matching lines @@
         resolver_->resolved_known_ipv6_hostname_ = true;
         bool got_ipv6_address = false;
         for (size_t i = 0; i < addr_list.size(); ++i) {
           if (addr_list[i].GetFamily() == ADDRESS_FAMILY_IPV6)
             got_ipv6_address = true;
         }
         UMA_HISTOGRAM_BOOLEAN("Net.UnspecResolvedIPv6", got_ipv6_address);
       }
     }
 
+    bool empty_list_on_ok = (net_error == OK && addr_list.empty());
+    UMA_HISTOGRAM_BOOLEAN("Net.DNS_EmptyListOnOk", empty_list_on_ok);

[szym, 2013/05/04 07:46:32]

I'd suggest putting the histogram in DNS.
Also, what do you want to count here? What fraction of all calls to getaddrinfo
returned OK and empty address list?

Then a name DNS.EmptyAddressListAndNoError would be appropriate.

[mef, 2013/05/06 16:49:11]

Done.

[mef, 2013/05/06 16:49:11]

Done.

+    if (empty_list_on_ok) {

[szym, 2013/05/04 07:46:32]

nit: no need for braces

[mef, 2013/05/06 16:49:11]

Done.

+      net_error = ERR_NAME_NOT_RESOLVED;
+    }
+
     if (dns_task_error_ != OK) {
       base::TimeDelta duration = base::TimeTicks::Now() - start_time;
       if (net_error == OK) {
         DNS_HISTOGRAM("AsyncDNS.FallbackSuccess", duration);
         if ((dns_task_error_ == ERR_NAME_NOT_RESOLVED) &&
             ResemblesNetBIOSName(key_.hostname)) {
           UmaAsyncDnsResolveStatus(RESOLVE_STATUS_SUSPECT_NETBIOS);
         } else {
           UmaAsyncDnsResolveStatus(RESOLVE_STATUS_PROC_SUCCESS);
         }
@@ skipping 779 matching lines @@
   }
   DnsConfig dns_config;
   NetworkChangeNotifier::GetDnsConfig(&dns_config);
   dns_client_->SetConfig(dns_config);
   num_dns_failures_ = 0;
   if (dns_config.IsValid())
     UMA_HISTOGRAM_BOOLEAN("AsyncDNS.DnsClientEnabled", true);
 }
 
 }  // namespace net