[net/dns] Add test DualFamilyLocalhost and fix it when DnsClient enabled.

[net/dns] Add test DualFamilyLocalhost and fix it when DnsClient enabled.

On some platforms, listening on "localhost" binds to the IPv6 loopback
(::1), even if there's no global IPv6 connectivity. In such situations,
HostResolverImpl restricts lookups to IPv4 (AF_INET) for performance
reasons, but then navigating to http://localhost results in "connection
refused".

HostResolverProc has a workaround for this situation. This CL adds a
test for this behavior and replicates the workaround in
HostResolverImpl::ServeFromHosts (for the built-in asynchronous DNS).

BUG=224215
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=194663

[szym, 2013-04-16 22:52:49]

PTAL.
Patch Set 3: with test and bug (test fails on ios_dbg_simulator, mac_rel and
win_rel)
Patch Set 4: with test and fix (test passes)

[mmenke, 2013-04-17 15:42:32]

This looks pretty reasonable to me, no major suggestions.

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

https://codereview.chromium.org/14208010/diff/20001/net/dns/host_resolver_imp...
net/dns/host_resolver_impl.cc:253: bool IsAllLocalhostOfOneFamily(const
AddressList& list) {
Think this function is worth a short description.

https://codereview.chromium.org/14208010/diff/20001/net/dns/host_resolver_imp...
net/dns/host_resolver_impl.cc:253: bool IsAllLocalhostOfOneFamily(const
AddressList& list) {
This function name seems to be inaccurate.  We check if they're localhost of any
family, rather than all of the same family.  The other function with this name
makes sure they were all in the same family.

https://codereview.chromium.org/14208010/diff/20001/net/dns/host_resolver_imp...
net/dns/host_resolver_impl.cc:253: bool IsAllLocalhostOfOneFamily(const
AddressList& list) {
optional nit:  Suggest using "addresses" rather than "list" to match the calling
function.

https://codereview.chromium.org/14208010/diff/20001/net/dns/host_resolver_imp...
net/dns/host_resolver_impl.cc:258: switch (list[i].GetFamily()) {
nit:  address.GetFamily()

https://codereview.chromium.org/14208010/diff/20001/net/dns/host_resolver_imp...
net/dns/host_resolver_impl.cc:260: if (address[0] != 127) return false;
nit:  Line break before "return".

https://codereview.chromium.org/14208010/diff/20001/net/dns/host_resolver_imp...
net/dns/host_resolver_impl.cc:262: case ADDRESS_FAMILY_IPV6:
Do we even need to check for IPv6?  I'm not particularly opposed to it, but
since we don't currently detect the IPv6-only support...

https://codereview.chromium.org/14208010/diff/20001/net/dns/host_resolver_imp...
File net/dns/host_resolver_impl_unittest.cc (right):

https://codereview.chromium.org/14208010/diff/20001/net/dns/host_resolver_imp...
net/dns/host_resolver_impl_unittest.cc:1492: // global IPv6 address. See
SystemHostResolverCall for rationale.
Test description generally goes before the text.  I think it's also worth
mentioning that this tests both the DnsClient and system host resolver paths.

https://codereview.chromium.org/14208010/diff/20001/net/dns/host_resolver_imp...
net/dns/host_resolver_impl_unittest.cc:1500: NULL));
optional nit:  May be a little more legible of these lines are indented 4 spaces
more than "new HostResolverImpl" .

https://codereview.chromium.org/14208010/diff/20001/net/dns/host_resolver_imp...
net/dns/host_resolver_impl_unittest.cc:1531:
EXPECT_TRUE(req->HasAddress("127.0.0.1", 80));
optional:  Seems like these could be:

EXPECT_EQ(saw_ipv4, req->HasAddress("127.0.0.1", 80));

Not that the negative case is terribly interesting, but since we try and match
the behavior on the local system in both cases...

[szym, 2013-04-17 17:09:16]

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

https://codereview.chromium.org/14208010/diff/20001/net/dns/host_resolver_imp...
net/dns/host_resolver_impl.cc:253: bool IsAllLocalhostOfOneFamily(const
AddressList& list) {
On 2013/04/17 15:42:32, mmenke wrote:
> Think this function is worth a short description.

Done.

https://codereview.chromium.org/14208010/diff/20001/net/dns/host_resolver_imp...
net/dns/host_resolver_impl.cc:253: bool IsAllLocalhostOfOneFamily(const
AddressList& list) {
On 2013/04/17 15:42:32, mmenke wrote:
> This function name seems to be inaccurate.  We check if they're localhost of
any
> family, rather than all of the same family.  The other function with this name
> makes sure they were all in the same family.

Done. Also changed "localhost" to "loopback" which is more accurate.

https://codereview.chromium.org/14208010/diff/20001/net/dns/host_resolver_imp...
net/dns/host_resolver_impl.cc:253: bool IsAllLocalhostOfOneFamily(const
AddressList& list) {
On 2013/04/17 15:42:32, mmenke wrote:
> optional nit:  Suggest using "addresses" rather than "list" to match the
calling
> function.

Done.

https://codereview.chromium.org/14208010/diff/20001/net/dns/host_resolver_imp...
net/dns/host_resolver_impl.cc:258: switch (list[i].GetFamily()) {
On 2013/04/17 15:42:32, mmenke wrote:
> nit:  address.GetFamily()

GetFamily is a function of IPEndPoint. IPAddressNumber is just a std::vector.

https://codereview.chromium.org/14208010/diff/20001/net/dns/host_resolver_imp...
net/dns/host_resolver_impl.cc:260: if (address[0] != 127) return false;
On 2013/04/17 15:42:32, mmenke wrote:
> nit:  Line break before "return".

Done.

https://codereview.chromium.org/14208010/diff/20001/net/dns/host_resolver_imp...
net/dns/host_resolver_impl.cc:262: case ADDRESS_FAMILY_IPV6:
On 2013/04/17 15:42:32, mmenke wrote:
> Do we even need to check for IPv6?  I'm not particularly opposed to it, but
> since we don't currently detect the IPv6-only support...

We don't, and this code path will not be exercised (only called if
DEFAULT_FAMILY_SET_DUE_TO_NO_IPV6).

Could change it to IsAllIPv4Loopback.

https://codereview.chromium.org/14208010/diff/20001/net/dns/host_resolver_imp...
File net/dns/host_resolver_impl_unittest.cc (right):

https://codereview.chromium.org/14208010/diff/20001/net/dns/host_resolver_imp...
net/dns/host_resolver_impl_unittest.cc:1492: // global IPv6 address. See
SystemHostResolverCall for rationale.
On 2013/04/17 15:42:32, mmenke wrote:
> Test description generally goes before the text.  I think it's also worth
> mentioning that this tests both the DnsClient and system host resolver paths.

Done.

https://codereview.chromium.org/14208010/diff/20001/net/dns/host_resolver_imp...
net/dns/host_resolver_impl_unittest.cc:1500: NULL));
On 2013/04/17 15:42:32, mmenke wrote:
> optional nit:  May be a little more legible of these lines are indented 4
spaces
> more than "new HostResolverImpl" .

Done. Indented at "("

https://codereview.chromium.org/14208010/diff/20001/net/dns/host_resolver_imp...
net/dns/host_resolver_impl_unittest.cc:1531:
EXPECT_TRUE(req->HasAddress("127.0.0.1", 80));
On 2013/04/17 15:42:32, mmenke wrote:
> optional:  Seems like these could be:
> 
> EXPECT_EQ(saw_ipv4, req->HasAddress("127.0.0.1", 80));
> 
> Not that the negative case is terribly interesting, but since we try and match
> the behavior on the local system in both cases...

Done.

[mmenke, 2013-04-17 17:35:00]

LGTM

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

https://codereview.chromium.org/14208010/diff/20001/net/dns/host_resolver_imp...
net/dns/host_resolver_impl.cc:262: case ADDRESS_FAMILY_IPV6:
On 2013/04/17 17:09:16, szym wrote:
> On 2013/04/17 15:42:32, mmenke wrote:
> > Do we even need to check for IPv6?  I'm not particularly opposed to it, but
> > since we don't currently detect the IPv6-only support...
> 
> We don't, and this code path will not be exercised (only called if
> DEFAULT_FAMILY_SET_DUE_TO_NO_IPV6).
> 
> Could change it to IsAllIPv4Loopback.

I think we should go ahead and do that.  If we need more functionality in the
future, can add it.

https://codereview.chromium.org/14208010/diff/26001/net/dns/host_resolver_imp...
File net/dns/host_resolver_impl.cc (right):

https://codereview.chromium.org/14208010/diff/26001/net/dns/host_resolver_imp...
net/dns/host_resolver_impl.cc:253: // Returns true if |list| contains only IPv4
loopback addresses or only
nit:  |addresses|

[szym, 2013-04-17 20:07:46]

Committed patchset #6 manually as r194663.