Make ERR_ICANN_NAME_COLLISION work for async DNS resolver.

net/dns/host_resolver_impl.cc

Index: net/dns/host_resolver_impl.cc
diff --git a/net/dns/host_resolver_impl.cc b/net/dns/host_resolver_impl.cc
index e4ee073591d764edfd0f20e786493da4eef2b7dd..2abab7d52167c39a575949fd966fca27712a8a98 100644
--- a/net/dns/host_resolver_impl.cc
+++ b/net/dns/host_resolver_impl.cc
@@ -181,7 +181,16 @@ enum DnsResolveStatus {
 // possible to navigate to http://127.0.53.53/ directly.
 //
 // For more details: https://www.icann.org/news/announcement-2-2014-08-01-en
-const uint8_t kIcanNameCollisionIp[] = {127, 0, 53, 53};
+bool ContainsIcannNameCollisionIp(const AddressList& addr_list) {
+  static const uint8_t kIcanNameCollisionIp[] = {127, 0, 53, 53};

[mmenke, 2016/12/13 19:24:15]

I thought not using static was preferred, to ensure no initializers?

[eroman, 2016/12/13 22:09:04]

Done.

Not sure (I have seen both instances in our code).
I have hoisted it back up to file-level scope to ensure consistency.

I think the C++ style guide says to use constexpr for function-level statics,
but didn't dig in too much.

+  for (const auto& endpoint : addr_list) {
+    const IPAddress& addr = endpoint.address();
+    if (addr.IsIPv4() && IPAddressStartsWith(addr, kIcanNameCollisionIp)) {
+      return true;
+    }
+  }
+  return false;
+}
 
 void UmaAsyncDnsResolveStatus(DnsResolveStatus result) {
   UMA_HISTOGRAM_ENUMERATION("AsyncDNS.ResolveStatus",
@@ -739,16 +748,6 @@ class HostResolverImpl::ProcTask
                                                &results,
                                                &os_error);
 
-    // Fail the resolution if the result contains 127.0.53.53. See the comment
-    // block of kIcanNameCollisionIp for details on why.
-    for (const auto& it : results) {
-      const IPAddress& cur = it.address();
-      if (cur.IsIPv4() && IPAddressStartsWith(cur, kIcanNameCollisionIp)) {
-        error = ERR_ICANN_NAME_COLLISION;
-        break;
-      }
-    }
-
     network_task_runner_->PostTask(
         FROM_HERE, base::Bind(&ProcTask::OnLookupComplete, this, results,
                               start_time, attempt_number, error, os_error));
@@ -1619,6 +1618,9 @@ class HostResolverImpl::Job : public PrioritizedDispatcher::Job,
       }
     }
 
+    if (ContainsIcannNameCollisionIp(addr_list))
+      net_error = ERR_ICANN_NAME_COLLISION;
+
     base::TimeDelta ttl =
         base::TimeDelta::FromSeconds(kNegativeCacheEntryTTLSeconds);
     if (net_error == OK)
@@ -1709,6 +1711,9 @@ class HostResolverImpl::Job : public PrioritizedDispatcher::Job,
     base::TimeDelta bounded_ttl =
         std::max(ttl, base::TimeDelta::FromSeconds(kMinimumTTLSeconds));
 
+    if (ContainsIcannNameCollisionIp(addr_list))
+      net_error = ERR_ICANN_NAME_COLLISION;
+
     CompleteRequests(
         HostCache::Entry(net_error, MakeAddressListForRequest(addr_list), ttl),
         bounded_ttl);

[mmenke, 2016/12/13 19:24:16]

I know this is old behavior, but can we not pass the addr_list back on this
error?

[eroman, 2016/12/13 22:09:04]

Correct, I believe that addr_list shouldn't be used in cases where net_error !=
OK.

That applies to both the proc-based codepath, and the dns-task-based codepath.

I can take a look at this as a follow-up and see if special-casing makes sense
(i.e. don't call MakeAddressListForRequest).

My gut reaction is it isn't worth the extra code (and branch) to reduce doing
the small extra work in this failure case, but I'll check it out.

[mmenke, 2016/12/13 22:14:59]

I'm actually not at all concerned about perf here.  My concern is that we stick
the entries in the cache, and I can just think of all the potential ways things
could go bad from there.

I'm fine with another CL, if you think my paranoia is not completely
unwarranted.

[eroman, 2016/12/13 22:50:18]

That's a fair concern.
I will address in a follow-up.

@@ -1769,7 +1774,7 @@ class HostResolverImpl::Job : public PrioritizedDispatcher::Job,
 
     DCHECK(!requests_.empty());
 
-    if (entry.error() == OK) {
+    if (entry.error() == OK || entry.error() == ERR_ICANN_NAME_COLLISION) {
       // Record this histogram here, when we know the system has a valid DNS
       // configuration.
       UMA_HISTOGRAM_BOOLEAN("AsyncDNS.HaveDnsConfig",