Don't probe for IPv6 connectivity when resolving IPv6 literals.

Don't probe for IPv6 connectivity when resolving IPv6 literals.

When the requester of a host resolution doesn't specify IPv4-only
(i.e. when it's safe to do so) HostResolverImpl should be
successfully resolving IPv6 literals.

BUG=428280
Committed: https://crrev.com/19eb734235f5c40970d8fd7f5958a8672d115d63
Cr-Commit-Position: refs/heads/master@{#325338}

[pauljensen, 2015-04-13 16:35:52]

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

[commit-bot: I haz the power, 2015-04-13 16:36:40]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1088513004/1

[mmenke, 2015-04-13 16:43:50]

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

[mmenke, 2015-04-13 16:43:51]

https://codereview.chromium.org/1088513004/diff/1/net/dns/host_resolver_impl.cc
File net/dns/host_resolver_impl.cc (right):

https://codereview.chromium.org/1088513004/diff/1/net/dns/host_resolver_impl....
net/dns/host_resolver_impl.cc:2167: !ParseIPLiteralToNumber(info.hostname(),
&ip_number)) {
Wonder if it'd be cleaner just to move ParseIPLiteralToNumber out of
ResolverHelper, and have the two resolve functions call it directly, before this
method.

[commit-bot: I haz the power, 2015-04-13 16:51:59]

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

[commit-bot: I haz the power, 2015-04-13 16:52:00]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[pauljensen, 2015-04-15 01:29:29]

I reworked it to call ParseIPLiteralToNumber just once.  It's a fair bit more
code but I think it'll be less maintenance in the future.

PTAL.

https://codereview.chromium.org/1088513004/diff/1/net/dns/host_resolver_impl.cc
File net/dns/host_resolver_impl.cc (right):

https://codereview.chromium.org/1088513004/diff/1/net/dns/host_resolver_impl....
net/dns/host_resolver_impl.cc:2167: !ParseIPLiteralToNumber(info.hostname(),
&ip_number)) {
On 2015/04/13 16:43:51, mmenke wrote:
> Wonder if it'd be cleaner just to move ParseIPLiteralToNumber out of
> ResolverHelper, and have the two resolve functions call it directly, before
this
> method.

Done.

[pauljensen, 2015-04-15 01:41:21]

I'll take a stab at adding a test tomorrow (hopefully).  Feel free to hold off
reviewing until then.

[mmenke, 2015-04-15 01:47:12]

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

https://codereview.chromium.org/1088513004/diff/20001/net/dns/host_resolver_i...
net/dns/host_resolver_impl.cc:1887: ip_number_ptr = &ip_number;
Is all this necessary?

Couldn't we just make ResolveAsIP capable of populating a RequestInfo struct,
and then use:

int rv;
if (!ResolveAsIP(info, ..., &rv)) {
  GetEffectiveKeyForRequest(...);
  ResolveHelper(...);
}

if (rv != ERR_DNS_CACHE_MISS) {
  ...
}

There's no need to call GetEffectiveKeyForRequest at all when we have a numeric
IP.  ResolveHelper then wouldn't handle numeric IPs, either.

[pauljensen, 2015-04-15 12:11:10]

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

https://codereview.chromium.org/1088513004/diff/20001/net/dns/host_resolver_i...
net/dns/host_resolver_impl.cc:1887: ip_number_ptr = &ip_number;
On 2015/04/15 01:47:12, mmenke wrote:
> Is all this necessary?
> 
> Couldn't we just make ResolveAsIP capable of populating a RequestInfo struct,
> and then use:
> 
> int rv;
> if (!ResolveAsIP(info, ..., &rv)) {
>   GetEffectiveKeyForRequest(...);
>   ResolveHelper(...);
> }
> 
> if (rv != ERR_DNS_CACHE_MISS) {
>   ...
> }
> 
> There's no need to call GetEffectiveKeyForRequest at all when we have a
numeric
> IP.  ResolveHelper then wouldn't handle numeric IPs, either.

GetEffectiveKeyForRequest() computes key.address_family from
info.address_family() and default_address_family_ and potentially an IPv6 probe.
 key.address_family is then used by ResolveAsIP() to determine if it's safe to
return particular address families.  If we move the call to ResolveAsIP() before
the call to GetEffectiveKeyForRequest() we'll have to duplicate
GetEffectiveKeyForRequest()'s logic that computes key.address_family in both
functions which doesn't seem like a good idea.  We could move that duplicated
logic into another function, but that would basically be most of the guts of
GetEffectiveKeyForRequest()...which is kind of what I have now:
GetEffectiveKeyForRequest() is called before ResolveAsIP().

Another alternative I considered was moving the ParseIPLiteralToNumber() call
into GetEffectiveKeyForRequest() but the IPAddressNumber has to have a lifetime
outside GetEffectiveKeyForRequest() so there'd still be ugliness like this:

  IPAddressNumber ip_number;
  IPAddressNumber* ip_number_ptr;
  Key key = GetEffectiveKeyForRequest(..., &ip_number, &ip_number_ptr);

Another alternative I considered was putting a "bool is_ip_literal;" and
"IPAddressNumber ip_number;" into the Key but that seems like a complete misuse
of the Key.

[mmenke, 2015-04-15 16:16:58]

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

https://codereview.chromium.org/1088513004/diff/20001/net/dns/host_resolver_i...
net/dns/host_resolver_impl.cc:1887: ip_number_ptr = &ip_number;
On 2015/04/15 12:11:10, pauljensen wrote:
> On 2015/04/15 01:47:12, mmenke wrote:
> > Is all this necessary?
> > 
> > Couldn't we just make ResolveAsIP capable of populating a RequestInfo
struct,
> > and then use:
> > 
> > int rv;
> > if (!ResolveAsIP(info, ..., &rv)) {
> >   GetEffectiveKeyForRequest(...);
> >   ResolveHelper(...);
> > }
> > 
> > if (rv != ERR_DNS_CACHE_MISS) {
> >   ...
> > }
> > 
> > There's no need to call GetEffectiveKeyForRequest at all when we have a
> numeric
> > IP.  ResolveHelper then wouldn't handle numeric IPs, either.
> 
> GetEffectiveKeyForRequest() computes key.address_family from
> info.address_family() and default_address_family_ and potentially an IPv6
probe.
>  key.address_family is then used by ResolveAsIP() to determine if it's safe to
> return particular address families.  If we move the call to ResolveAsIP()
before
> the call to GetEffectiveKeyForRequest() we'll have to duplicate
> GetEffectiveKeyForRequest()'s logic that computes key.address_family in both
> functions which doesn't seem like a good idea.  We could move that duplicated
> logic into another function, but that would basically be most of the guts of
> GetEffectiveKeyForRequest()...which is kind of what I have now:
> GetEffectiveKeyForRequest() is called before ResolveAsIP().

Ah, right - Since we can ignore the probe, I was thinking everything would work
out, but we still need to look at default_address_family_ (Wonder if we really
care about that case - the disable_ipv6 command line flag is really to avoid
using ipv6 when ipv6 is slow/buggy, but if we're given an IPv6 address to start
with....)

> Another alternative I considered was moving the ParseIPLiteralToNumber() call
> into GetEffectiveKeyForRequest() but the IPAddressNumber has to have a
lifetime
> outside GetEffectiveKeyForRequest() so there'd still be ugliness like this:
> 
>   IPAddressNumber ip_number;
>   IPAddressNumber* ip_number_ptr;
>   Key key = GetEffectiveKeyForRequest(..., &ip_number, &ip_number_ptr);
> 
> Another alternative I considered was putting a "bool is_ip_literal;" and
> "IPAddressNumber ip_number;" into the Key but that seems like a complete
misuse
> of the Key.

https://codereview.chromium.org/1088513004/diff/20001/net/dns/host_resolver_i...
net/dns/host_resolver_impl.cc:2052: default_address_family_ ==
ADDRESS_FAMILY_IPV4) {
Per your earlier comment, checking default_address_family_ here is weird, as it
duplicates logic.  Shouldn't we get rid of this entire check?  We've already
used those rules to set key.address_family, right?

https://codereview.chromium.org/1088513004/diff/20001/net/dns/host_resolver_i...
net/dns/host_resolver_impl.cc:2173: // limiting our resolution based on a probe.
 We've already
nit:  Don't use "we" in comments

https://codereview.chromium.org/1088513004/diff/20001/net/dns/host_resolver_i...
net/dns/host_resolver_impl.cc:2176: // (prove_ipv6_support_ is false if
default_address_family_ is
nit:  Think most of the comments in this file use || around member variable
names, so should probably be consistent.

[pauljensen, 2015-04-15 18:59:54]

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

https://codereview.chromium.org/1088513004/diff/20001/net/dns/host_resolver_i...
net/dns/host_resolver_impl.cc:2052: default_address_family_ ==
ADDRESS_FAMILY_IPV4) {
On 2015/04/15 16:16:57, mmenke wrote:
> Per your earlier comment, checking default_address_family_ here is weird, as
it
> duplicates logic.  Shouldn't we get rid of this entire check?  We've already
> used those rules to set key.address_family, right?

This check is indeed weird.  Unfortunately it's not entirely eclipsed by the
GetEffectiveKeyForRequest() logic; for example if info.address_family() is IPv6
but default_address_family_ is IPv4, then this check will be hit but the
key.address_family check wouldn't.
I think we should leave it here until we get rid of the --enable-ipv6 and
--disable-ipv6 flags entirely.  I partially say this because I don't want to
accidentally break anything and this logic is fragile and untested as the linked
bug demonstrates.  WDYT?

https://codereview.chromium.org/1088513004/diff/20001/net/dns/host_resolver_i...
net/dns/host_resolver_impl.cc:2173: // limiting our resolution based on a probe.
 We've already
On 2015/04/15 16:16:57, mmenke wrote:
> nit:  Don't use "we" in comments

Done.

https://codereview.chromium.org/1088513004/diff/20001/net/dns/host_resolver_i...
net/dns/host_resolver_impl.cc:2176: // (prove_ipv6_support_ is false if
default_address_family_ is
On 2015/04/15 16:16:57, mmenke wrote:
> nit:  Think most of the comments in this file use || around member variable
> names, so should probably be consistent.

Done.  Sorry, my Android ways got the better of me.

[mmenke, 2015-04-15 19:03:17]

LGTM

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

https://codereview.chromium.org/1088513004/diff/20001/net/dns/host_resolver_i...
net/dns/host_resolver_impl.cc:2052: default_address_family_ ==
ADDRESS_FAMILY_IPV4) {
On 2015/04/15 18:59:54, pauljensen wrote:
> On 2015/04/15 16:16:57, mmenke wrote:
> > Per your earlier comment, checking default_address_family_ here is weird, as
> it
> > duplicates logic.  Shouldn't we get rid of this entire check?  We've already
> > used those rules to set key.address_family, right?
> 
> This check is indeed weird.  Unfortunately it's not entirely eclipsed by the
> GetEffectiveKeyForRequest() logic; for example if info.address_family() is
IPv6
> but default_address_family_ is IPv4, then this check will be hit but the
> key.address_family check wouldn't.
> I think we should leave it here until we get rid of the --enable-ipv6 and
> --disable-ipv6 flags entirely.  I partially say this because I don't want to
> accidentally break anything and this logic is fragile and untested as the
linked
> bug demonstrates.  WDYT?

Ahh...Good point.  Yea, think we're best leaving well enough alone for now.

[pauljensen, 2015-04-15 19:26:03]

The CQ bit was checked by pauljensen@chromium.org

[commit-bot: I haz the power, 2015-04-15 19:26:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1088513004/40001

[commit-bot: I haz the power, 2015-04-15 23:20:13]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-04-15 23:21:03]

Patchset 3 (id:??) landed as
https://crrev.com/19eb734235f5c40970d8fd7f5958a8672d115d63
Cr-Commit-Position: refs/heads/master@{#325338}