[net/dns] Resolve AF_UNSPEC on dual-stacked systems. Sort addresses according to RFC3484.

[net/dns] Resolve AF_UNSPEC on dual-stacked systems. Sort addresses according to RFC3484.

Original review: http://codereview.chromium.org/10442098/

BUG=113993
TEST=./net_unittests --gtest_filter=AddressSorter*:HostResolverImplDnsTest.DnsTaskUnspec

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

[szym, 2012-08-15 17:16:48]

http://codereview.chromium.org/10855179/diff/1/net/dns/address_sorter_win.cc
File net/dns/address_sorter_win.cc (right):

http://codereview.chromium.org/10855179/diff/1/net/dns/address_sorter_win.cc#...
net/dns/address_sorter_win.cc:91: DCHECK_NE(INVALID_SOCKET, sock);
This could fail on systems without ipv6 stack (for example, Windows XP without
Teredo). However, in that case we'd expect to disable IPv6 via
TestIPv6Support().

The lenient solution is to simply quit here. The strict solution is to leave the
DCHECK. In both cases we need to disable the test if |socket(AF_INET6, ...) ==
INVALID_SOCKET|

[szym, 2012-08-15 17:29:17]

I decided to go the lenient way. PTAL.

On 2012/08/15 17:16:48, szym wrote:
> http://codereview.chromium.org/10855179/diff/1/net/dns/address_sorter_win.cc
> File net/dns/address_sorter_win.cc (right):
> 
>
http://codereview.chromium.org/10855179/diff/1/net/dns/address_sorter_win.cc#...
> net/dns/address_sorter_win.cc:91: DCHECK_NE(INVALID_SOCKET, sock);
> This could fail on systems without ipv6 stack (for example, Windows XP without
> Teredo). However, in that case we'd expect to disable IPv6 via
> TestIPv6Support().
> 
> The lenient solution is to simply quit here. The strict solution is to leave
the
> DCHECK. In both cases we need to disable the test if |socket(AF_INET6, ...) ==
> INVALID_SOCKET|

[mmenke1, 2012-08-15 17:36:53]

http://codereview.chromium.org/10855179/diff/1/net/dns/address_sorter_win.cc
File net/dns/address_sorter_win.cc (right):

http://codereview.chromium.org/10855179/diff/1/net/dns/address_sorter_win.cc#...
net/dns/address_sorter_win.cc:91: DCHECK_NE(INVALID_SOCKET, sock);
On 2012/08/15 17:16:48, szym wrote:
> This could fail on systems without ipv6 stack (for example, Windows XP without
> Teredo). However, in that case we'd expect to disable IPv6 via
> TestIPv6Support().
> 
> The lenient solution is to simply quit here. The strict solution is to leave
the
> DCHECK. In both cases we need to disable the test if |socket(AF_INET6, ...) ==
> INVALID_SOCKET|

May want to delete all the IPv6 addresses in this case, at least if we have IPv4
addresses as well.  No need to address that in this CL.

Couldn't we have the unit test check for graceful failure in this case - would
still need to detect the case, but instead of aborting, could check that
success_ = false.

[szym, 2012-08-15 18:10:39]

> May want to delete all the IPv6 addresses in this case, at least if we have
IPv4
> addresses as well.  No need to address that in this CL.

I'd rather conservatively fail to sort and fall back to getaddrinfo, at least
for now. In practice, ProbeIPv6Support should disable IPv6 and we should never
call AddressSorter::Sort in this case. The only reason to do this would be to be
consistent with AddressSorterPosix.
 
> Couldn't we have the unit test check for graceful failure in this case - would
> still need to detect the case, but instead of aborting, could check that
> success_ = false.

Would it be ok to have such platform-specific behavior? I.e., AddressSorterPosix
will succeed and filter out the IPv6 addresses, while AddressSorterWin will
fail.

[mmenke, 2012-08-15 18:19:01]

On 2012/08/15 18:10:39, szym wrote:
> > May want to delete all the IPv6 addresses in this case, at least if we have
> IPv4
> > addresses as well.  No need to address that in this CL.
> 
> I'd rather conservatively fail to sort and fall back to getaddrinfo, at least
> for now. In practice, ProbeIPv6Support should disable IPv6 and we should never
> call AddressSorter::Sort in this case. The only reason to do this would be to
be
> consistent with AddressSorterPosix.
>  
> > Couldn't we have the unit test check for graceful failure in this case -
would
> > still need to detect the case, but instead of aborting, could check that
> > success_ = false.
> 
> Would it be ok to have such platform-specific behavior? I.e.,
AddressSorterPosix
> will succeed and filter out the IPv6 addresses, while AddressSorterWin will
> fail.

That's basically what we're already doing.  Could make another test for it, if
you prefer.

[szym, 2012-08-15 18:24:10]

On 2012/08/15 18:19:01, Matt Menke wrote:
> On 2012/08/15 18:10:39, szym wrote:
> > > May want to delete all the IPv6 addresses in this case, at least if we
have
> > IPv4
> > > addresses as well.  No need to address that in this CL.
> > 
> > I'd rather conservatively fail to sort and fall back to getaddrinfo, at
least
> > for now. In practice, ProbeIPv6Support should disable IPv6 and we should
never
> > call AddressSorter::Sort in this case. The only reason to do this would be
to
> be
> > consistent with AddressSorterPosix.
> >  
> > > Couldn't we have the unit test check for graceful failure in this case -
> would
> > > still need to detect the case, but instead of aborting, could check that
> > > success_ = false.
> > 
> > Would it be ok to have such platform-specific behavior? I.e.,
> AddressSorterPosix
> > will succeed and filter out the IPv6 addresses, while AddressSorterWin will
> > fail.
> 
> That's basically what we're already doing.  Could make another test for it, if
> you prefer.

Ok, I added expected result to AddressSorterTest.Sort. PTAL.

[mmenke, 2012-08-15 18:27:31]

LGTM

[commit-bot: I haz the power, 2012-08-15 18:28:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/szym@chromium.org/10855179/8003