Make ERR_ICANN_NAME_COLLISION work for async DNS resolver.

Make ERR_ICANN_NAME_COLLISION work for async DNS resolver.

BUG=670093
Committed: https://crrev.com/1efc237c5f0ef90195c3dd54e2d63f3e056194be
Cr-Commit-Position: refs/heads/master@{#438346}

[eroman, 2016-12-10 02:08:09]

The CQ bit was checked by eroman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-10 02:08:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[eroman, 2016-12-10 02:09:25]

eroman@chromium.org changed reviewers:
+ mmenke@chromium.org

[commit-bot: I haz the power, 2016-12-10 02:33:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-10 02:33:34]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[eroman, 2016-12-12 19:10:33]

The CQ bit was checked by eroman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-12 19:11:19]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[eroman, 2016-12-12 19:18:29]

The CQ bit was checked by eroman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-12 19:19:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[eroman, 2016-12-12 19:22:40]

The CQ bit was checked by eroman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-12 19:22:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-12 19:42:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-12 19:42:41]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[eroman, 2016-12-12 19:49:17]

The CQ bit was checked by eroman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-12 19:50:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-12 23:11:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-12 23:11:24]

Dry run: This issue passed the CQ dry run.

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

LGTM!

https://codereview.chromium.org/2567623003/diff/80001/net/dns/dns_test_util.cc
File net/dns/dns_test_util.cc (right):

https://codereview.chromium.org/2567623003/diff/80001/net/dns/dns_test_util.c...
net/dns/dns_test_util.cc:134: result_.ip.bytes().size());
nit:  result_.ip.bytes().size() -> size, or remove rdata_size and just inline
it?  getting the size for the rdata two different places in two adjacent lines
seems weird.

https://codereview.chromium.org/2567623003/diff/80001/net/dns/host_resolver_i...
File net/dns/host_resolver_impl.cc (right):

https://codereview.chromium.org/2567623003/diff/80001/net/dns/host_resolver_i...
net/dns/host_resolver_impl.cc:185: static const uint8_t kIcanNameCollisionIp[] =
{127, 0, 53, 53};
I thought not using static was preferred, to ensure no initializers?

https://codereview.chromium.org/2567623003/diff/80001/net/dns/host_resolver_i...
net/dns/host_resolver_impl.cc:1719: bounded_ttl);
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]

Thanks for the review!

https://codereview.chromium.org/2567623003/diff/80001/net/dns/dns_test_util.cc
File net/dns/dns_test_util.cc (right):

https://codereview.chromium.org/2567623003/diff/80001/net/dns/dns_test_util.c...
net/dns/dns_test_util.cc:134: result_.ip.bytes().size());
On 2016/12/13 19:24:15, mmenke wrote:
> nit:  result_.ip.bytes().size() -> size, or remove rdata_size and just inline
> it?  getting the size for the rdata two different places in two adjacent lines
> seems weird.

Done.

https://codereview.chromium.org/2567623003/diff/80001/net/dns/host_resolver_i...
File net/dns/host_resolver_impl.cc (right):

https://codereview.chromium.org/2567623003/diff/80001/net/dns/host_resolver_i...
net/dns/host_resolver_impl.cc:185: static const uint8_t kIcanNameCollisionIp[] =
{127, 0, 53, 53};
On 2016/12/13 19:24:15, mmenke wrote:
> I thought not using static was preferred, to ensure no initializers?

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.

https://codereview.chromium.org/2567623003/diff/80001/net/dns/host_resolver_i...
net/dns/host_resolver_impl.cc:1719: bounded_ttl);
On 2016/12/13 19:24:16, mmenke wrote:
> I know this is old behavior, but can we not pass the addr_list back on this
> error?

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.

[eroman, 2016-12-13 22:09:11]

The CQ bit was checked by eroman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-13 22:10:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

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

LGTM

https://codereview.chromium.org/2567623003/diff/80001/net/dns/host_resolver_i...
File net/dns/host_resolver_impl.cc (right):

https://codereview.chromium.org/2567623003/diff/80001/net/dns/host_resolver_i...
net/dns/host_resolver_impl.cc:1719: bounded_ttl);
On 2016/12/13 22:09:04, eroman (slow) wrote:
> On 2016/12/13 19:24:16, mmenke wrote:
> > I know this is old behavior, but can we not pass the addr_list back on this
> > error?
> 
> 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.

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]

https://codereview.chromium.org/2567623003/diff/80001/net/dns/host_resolver_i...
File net/dns/host_resolver_impl.cc (right):

https://codereview.chromium.org/2567623003/diff/80001/net/dns/host_resolver_i...
net/dns/host_resolver_impl.cc:1719: bounded_ttl);
On 2016/12/13 22:14:59, mmenke wrote:
> On 2016/12/13 22:09:04, eroman (slow) wrote:
> > On 2016/12/13 19:24:16, mmenke wrote:
> > > I know this is old behavior, but can we not pass the addr_list back on
this
> > > error?
> > 
> > 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.
> 
> 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.

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

[commit-bot: I haz the power, 2016-12-13 23:27:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-13 23:27:34]

Dry run: This issue passed the CQ dry run.

[eroman, 2016-12-13 23:54:12]

The CQ bit was checked by eroman@chromium.org

[commit-bot: I haz the power, 2016-12-13 23:54:45]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-14 00:00:52]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1481673252495310,
"parent_rev": "58e116fc3810776c20108b44f1c83678f1273fb2", "commit_rev":
"cb467717667a897b4670fb6c60b0772ef9d75367"}

[commit-bot: I haz the power, 2016-12-14 00:01:22]

Description was changed from

==========
Make ERR_ICANN_NAME_COLLISION work for async DNS resolver.

BUG=670093
==========

to

==========
Make ERR_ICANN_NAME_COLLISION work for async DNS resolver.

BUG=670093

Review-Url: https://codereview.chromium.org/2567623003
==========

[commit-bot: I haz the power, 2016-12-14 00:01:23]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-12-14 00:03:08]

Description was changed from

==========
Make ERR_ICANN_NAME_COLLISION work for async DNS resolver.

BUG=670093

Review-Url: https://codereview.chromium.org/2567623003
==========

to

==========
Make ERR_ICANN_NAME_COLLISION work for async DNS resolver.

BUG=670093

Committed: https://crrev.com/1efc237c5f0ef90195c3dd54e2d63f3e056194be
Cr-Commit-Position: refs/heads/master@{#438346}
==========

[commit-bot: I haz the power, 2016-12-14 00:03:09]

Patchset 6 (id:??) landed as
https://crrev.com/1efc237c5f0ef90195c3dd54e2d63f3e056194be
Cr-Commit-Position: refs/heads/master@{#438346}