Fix remaining localhost resolution issues.

Fix remaining localhost resolution issues.

If host resolution is restricted to a single address family and the results of a resolution are all localhost of that type, try again without the address family restriction.

Also make --disable-ipv6 apply to resolution of ip literals.

BUG=42058, 49024, 32522
TEST=Manual testing in various network configurations. See https://spreadsheets.google.com/ccc?key=0AhSnKEusL6UgdFZIRmxTUnYtZDhjX3lKclBqMHo4YUE&hl=en&authkey=CLKDxMYB

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=58534

[vandebo (ex-Chrome), 2010-08-27 18:14:38]

I tested the matrix in the spreadsheet on Linux and Mac and this resolves all
the problem cases.  I tried both Apache and Python to setup an IPv6 webserver on
Windows without success before giving up on manual testing on Windows.

[eroman, 2010-08-27 19:03:14]

Is this only for resolving "localhost" ?

It seems like the problem is the automatic IPv6 probe, in which case wouldn't it
make sense to modify host_resolver_impl.cc to simply not pass the address family
restriction when resolving "localhost"?

I am a bit skeptical about doing it globally as a fallback in
host_resolver_proc, since it means that consumers which explicitly restricted
the input to IPv4 only (like say proxy auto config scripts) may in fact get
non-IPv4 addresses.

http://codereview.chromium.org/3231005/diff/1/2
File net/base/host_resolver_proc.cc (right):

http://codereview.chromium.org/3231005/diff/1/2#newcode26
net/base/host_resolver_proc.cc:26: const struct sockaddr_in* addr_in;
can you move these inside of the switch?

http://codereview.chromium.org/3231005/diff/1/2#newcode28
net/base/host_resolver_proc.cc:28: for (;ai != NULL; ai = ai->ai_next) {
nit: space after first semicolon.

http://codereview.chromium.org/3231005/diff/1/2#newcode204
net/base/host_resolver_proc.cc:204: if (err == EAI_ADDRFAMILY && (hints.ai_flags
& AI_ADDRCONFIG)) {
Please add comment to each of these code blocks on why we do it, also linking to
the bugs.

http://codereview.chromium.org/3231005/diff/1/2#newcode210
net/base/host_resolver_proc.cc:210: !err && IsAllLocalhostOfOneFamily(ai)) {
Can you  use err != 0 instead? (!err reads funny to me).

[vandebo (ex-Chrome), 2010-08-27 21:40:37]

Arg, codereview lost the first reply.

On 2010/08/27 19:03:14, eroman wrote:
> Is this only for resolving "localhost" ?

Yes, by value, but no by name.  Some distros add entries like ipv6-localhost to
/etc/hosts.  Users can also add arbitrary names for localhost either to
/etc/hosts or to a local DNS server.

> It seems like the problem is the automatic IPv6 probe, in which case wouldn't
it
> make sense to modify host_resolver_impl.cc to simply not pass the address
family
> restriction when resolving "localhost"?

Whitelisting "localhost" seems like a bad idea to me.

> I am a bit skeptical about doing it globally as a fallback in
> host_resolver_proc, since it means that consumers which explicitly restricted
> the input to IPv4 only (like say proxy auto config scripts) may in fact get
> non-IPv4 addresses.

Hmm - the address family value is changed for conflicting reasons. I could make
the ipv6 detection code set a flag that the resolver consults instead of
changing the address family then the passed address family would always
represent intent, and the resolver implementation might further restrict it for
performance reasons (no ipv6 deteted).

[eroman, 2010-08-27 23:02:11]

My main interest is that if a caller to HostResolver::Resolve() passes in a
RequestInfo that restricts the address_family, it should be guaranteed that it
will only receive results of that address family, or none at all.

I am less concerned about what happens with the default address family in
HostResolverImpl, but I do think it is important to maintain the invariant for
individual requests that pass a restriction.

[vandebo (ex-Chrome), 2010-09-02 21:40:57]

> My main interest is that if a caller to HostResolver::Resolve() passes in a
> RequestInfo that restricts the address_family, it should be guaranteed that it
> will only receive results of that address family, or none at all.
> 
> I am less concerned about what happens with the default address family in
> HostResolverImpl, but I do think it is important to maintain the invariant for
> individual requests that pass a restriction.

I think my change addresses your concern.  It will only reset the family to
unspecified if the HOST_RESOLVER_DEFAULT_FAMILY_SET_DUE_TO_NO_IPV6 host resolver
flag is set - which is only set when changing the family from unspecified to
ipv4 because ipv6 support was not detected.

http://codereview.chromium.org/3231005/diff/1/2
File net/base/host_resolver_proc.cc (right):

http://codereview.chromium.org/3231005/diff/1/2#newcode26
net/base/host_resolver_proc.cc:26: const struct sockaddr_in* addr_in;
On 2010/08/27 19:03:14, eroman wrote:
> can you move these inside of the switch?

Done.

http://codereview.chromium.org/3231005/diff/1/2#newcode28
net/base/host_resolver_proc.cc:28: for (;ai != NULL; ai = ai->ai_next) {
On 2010/08/27 19:03:14, eroman wrote:
> nit: space after first semicolon.

Done.

http://codereview.chromium.org/3231005/diff/1/2#newcode204
net/base/host_resolver_proc.cc:204: if (err == EAI_ADDRFAMILY && (hints.ai_flags
& AI_ADDRCONFIG)) {
On 2010/08/27 19:03:14, eroman wrote:
> Please add comment to each of these code blocks on why we do it, also linking
to
> the bugs.

Hmm.  Looks like the IP literal change obviated the need for this clause.

http://codereview.chromium.org/3231005/diff/1/2#newcode210
net/base/host_resolver_proc.cc:210: !err && IsAllLocalhostOfOneFamily(ai)) {
On 2010/08/27 19:03:14, eroman wrote:
> Can you  use err != 0 instead? (!err reads funny to me).

Done - well, err == 0.

[vandebo (ex-Chrome), 2010-09-02 21:42:04]

Will, do you feel comfortable reviewing this change while Eric is out?  I'd like
to get it into M7.

[willchan no longer on Chromium, 2010-09-03 19:36:50]

Some preliminary comments.  Lunch and then I'll be back to finish.

http://codereview.chromium.org/3231005/diff/12001/13002
File net/base/host_resolver_impl.cc (right):

http://codereview.chromium.org/3231005/diff/12001/13002#newcode893
net/base/host_resolver_impl.cc:893: net_error = ERR_NAME_NOT_RESOLVED;
This strikes me as a weird error to return, since it's an IP address literal
after all.  Probably not a big deal.

http://codereview.chromium.org/3231005/diff/12001/13003
File net/base/host_resolver_proc.cc (right):

http://codereview.chromium.org/3231005/diff/12001/13003#newcode215
net/base/host_resolver_proc.cc:215: err == 0 && IsAllLocalhostOfOneFamily(ai)) {
I personally think "err == OK" is more readable, but I defer to you.

http://codereview.chromium.org/3231005/diff/12001/13003#newcode226
net/base/host_resolver_proc.cc:226: freeaddrinfo(ai);
don't you need a NULL check here?  In the line 207 condition, won't ai be NULL? 
delete does a NULL check, but I don't think free() does (I'm not sure).

[vandebo (ex-Chrome), 2010-09-03 19:42:18]

http://codereview.chromium.org/3231005/diff/12001/13003
File net/base/host_resolver_proc.cc (right):

http://codereview.chromium.org/3231005/diff/12001/13003#newcode215
net/base/host_resolver_proc.cc:215: err == 0 && IsAllLocalhostOfOneFamily(ai)) {
On 2010/09/03 19:36:50, willchan wrote:
> I personally think "err == OK" is more readable, but I defer to you.

err is the result of getaddrinfo, not a chrome network function.  While OK is 0,
it could be something else.  I think == 0 is the right thing, though maybe a bit
pedantic.

http://codereview.chromium.org/3231005/diff/12001/13003#newcode226
net/base/host_resolver_proc.cc:226: freeaddrinfo(ai);
On 2010/09/03 19:36:50, willchan wrote:
> don't you need a NULL check here?  In the line 207 condition, won't ai be
NULL? 
> delete does a NULL check, but I don't think free() does (I'm not sure).

From man 3 free: 'If ptr is NULL, no operation is performed.'

[willchan no longer on Chromium, 2010-09-03 20:23:07]

I agree with your statement that you've addressed eroman's concern earlier.  I
feel like the placement of the retry is slightly odd.  I sort of feel like it is
something that HostResolverImpl should be in charge of.  This isn't a terribly
strong feeling though.  I will defer to eroman here.  But, to unblock you, I am
comfortable LGTM'ing this in terms of functional desirability and correctness. 
But I encourage eroman to still take a look and suggest changes if he wants even
after this lands.

LGTM.

http://codereview.chromium.org/3231005/diff/12001/13001
File net/base/address_family.h (right):

http://codereview.chromium.org/3231005/diff/12001/13001#newcode26
net/base/address_family.h:26: HOST_RESOLVER_DEFAULT_FAMILY_SET_DUE_TO_NO_IPV6 =
1 << 2,
Is this flag an abuse of HostResolverFlags?  It doesn't jive with the comment
above which says this only controls addrinfo.ai_flags.  Or perhaps we need to
update the comment?

http://codereview.chromium.org/3231005/diff/12001/13003
File net/base/host_resolver_proc.cc (right):

http://codereview.chromium.org/3231005/diff/12001/13003#newcode215
net/base/host_resolver_proc.cc:215: err == 0 && IsAllLocalhostOfOneFamily(ai)) {
On 2010/09/03 19:42:18, vandebo wrote:
> On 2010/09/03 19:36:50, willchan wrote:
> > I personally think "err == OK" is more readable, but I defer to you.
> 
> err is the result of getaddrinfo, not a chrome network function.  While OK is
0,
> it could be something else.  I think == 0 is the right thing, though maybe a
bit
> pedantic.

Oops, sorry, I thought this was a net::Error.  I completely agree.

http://codereview.chromium.org/3231005/diff/12001/13003#newcode226
net/base/host_resolver_proc.cc:226: freeaddrinfo(ai);
On 2010/09/03 19:42:18, vandebo wrote:
> On 2010/09/03 19:36:50, willchan wrote:
> > don't you need a NULL check here?  In the line 207 condition, won't ai be
> NULL? 
> > delete does a NULL check, but I don't think free() does (I'm not sure).
> 
> From man 3 free: 'If ptr is NULL, no operation is performed.'
> 

Clearly I haven't done C programming in forever.

[vandebo (ex-Chrome), 2010-09-03 20:33:42]

http://codereview.chromium.org/3231005/diff/12001/13001
File net/base/address_family.h (right):

http://codereview.chromium.org/3231005/diff/12001/13001#newcode26
net/base/address_family.h:26: HOST_RESOLVER_DEFAULT_FAMILY_SET_DUE_TO_NO_IPV6 =
1 << 2,
On 2010/09/03 20:23:07, willchan wrote:
> Is this flag an abuse of HostResolverFlags?  It doesn't jive with the comment
> above which says this only controls addrinfo.ai_flags.  Or perhaps we need to
> update the comment?

The flags do determine addrinfo.ai_flags, though there's not a direct
correspondence. It is somewhat of an abuse, yes.  I updated the comment to
describe reality a bit better.