[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.

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

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

[cbentzel, 2012-06-02 00:52:45]

cool

[szym, 2012-06-07 01:48:04]

If you feel adventurous, this should now work on Windows (Vista+). I need to
devise a testing plan, although I'm leaning towards testing just whether the
ioctl call returns true. 

So far, in my Debug build this ioctl takes ~1ms (measurement resolution), so I'm
proposing we keep it on the IO thread, but the machine is rather overpowered, so
I'd appreciate if you tested it on a weaker box.

On Linux, source address information is needed, so this will have to wait for
another CL which will add AddressTracker to NetworkChangeNotifierLinux.

[mmenke, 2012-06-07 18:32:34]

Preliminary comments.  I'm going to have to take a close look at the relevant
RFCs to fully review this (Obviously).  I'll try to get you more comments
tomorrow, but not sure I'll be able to do a full review until after the weekend.

I'd also like to have some unit tests for this.  Particularly the posix side of
things, but even a simple test under (non-XP) Windows that just makes sure we
get the right number of IPv4 addresses back and don't crash seems reasonable to
me.

http://codereview.chromium.org/10442098/diff/21002/net/base/host_resolver_imp...
File net/base/host_resolver_impl.cc (right):

http://codereview.chromium.org/10442098/diff/21002/net/base/host_resolver_imp...
net/base/host_resolver_impl.cc:1096: OnFailure(net_error,
DnsResponse::DNS_SUCCESS);
These DNS_SUCCESSes everywhere caused me a fair bit of confusion, until I
remembered they meant there was no error parsing the data we received.  I think
this is either worth a comment in OnFailure that this is specifically a code for
parsing the result, if received, or "DnsResponse::DNS_SUCCESS" should be renamed
to something less confusing and more accurate, like maybe "DNS_NO_PARSE_ERROR"
or "DNS_PARSER_OK".

No need to do anything in this CL, though feel free to do so if you want.

http://codereview.chromium.org/10442098/diff/21002/net/base/host_resolver_imp...
net/base/host_resolver_impl.cc:1117: if (family_ == ADDRESS_FAMILY_UNSPECIFIED)
{
nit:  You might want to swap these two if's, so the ADDRESS_FAMILY_UNSPECIFIED
behavior is together, rather than having that little intervening
ADDRESS_FAMILY_IPV6 case.

http://codereview.chromium.org/10442098/diff/21002/net/base/host_resolver_impl.h
File net/base/host_resolver_impl.h (right):

http://codereview.chromium.org/10442098/diff/21002/net/base/host_resolver_imp...
net/base/host_resolver_impl.h:31: class DnsConfigService;
I believe you can do the same for net_log.h as well.  Just need to forward
declare NetLog and BoundNetLog.

http://codereview.chromium.org/10442098/diff/21002/net/base/host_resolver_imp...
File net/base/host_resolver_impl_unittest.cc (right):

http://codereview.chromium.org/10442098/diff/21002/net/base/host_resolver_imp...
net/base/host_resolver_impl_unittest.cc:410: : proc_(new MockHostResolverProc())
{
nit:  Think this can all fit on one line now.

http://codereview.chromium.org/10442098/diff/21002/net/base/host_resolver_imp...
net/base/host_resolver_impl_unittest.cc:1305: EXPECT_EQ(ERR_IO_PENDING,
CreateRequest("ok_fail", 80)->Resolve());
I think "ok_fail" is a somewhat strange name for a request that succeeds.  Only
makes sense in that above test because it fails the first time.

http://codereview.chromium.org/10442098/diff/21002/net/base/host_resolver_imp...
net/base/host_resolver_impl_unittest.cc:1311: }
I'd like to see test where the MockDnsTransaction succeeds with only an IPv4
result, and only an ipv6 result.

http://codereview.chromium.org/10442098/diff/21002/net/dns/address_sorter_pos...
File net/dns/address_sorter_posix.cc (right):

http://codereview.chromium.org/10442098/diff/21002/net/dns/address_sorter_pos...
net/dns/address_sorter_posix.cc:173: SCOPE_LINKLOCAL },
nit:  Think this and the next one both fit on one line.

http://codereview.chromium.org/10442098/diff/21002/net/dns/address_sorter_pos...
net/dns/address_sorter_posix.cc:355: // Filter out unsable destinations.
nit:  unusable.

http://codereview.chromium.org/10442098/diff/21002/net/dns/address_sorter_pos...
net/dns/address_sorter_posix.cc:355: // Filter out unsable destinations.
Also, doesn't this comment also apply to the line above it as well?  At least I
assume it would also catch bad destination addresses.

http://codereview.chromium.org/10442098/diff/21002/net/dns/address_sorter_pos...
net/dns/address_sorter_posix.cc:363: // TODO(szym): This should not be possible.
Are you sure about this?  Adapters can be removed/added and the map may not yet
have been updated.

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

http://codereview.chromium.org/10442098/diff/21002/net/dns/address_sorter_win...
net/dns/address_sorter_win.cc:51: SOCKET sock = socket(AF_INET6, SOCK_DGRAM,
IPPROTO_UDP);
I believe we technically need to call closesocket on this when done.  It's also
not clear how expensive this function call is.  We may want to cache the socket.

http://codereview.chromium.org/10442098/diff/21002/net/dns/address_sorter_win...
net/dns/address_sorter_win.cc:55: sort_list, heap_size, &result_size, NULL,
NULL);
I do wonder about doing this on the IO thread.  Suspect there are a couple
registry reads involved.

http://codereview.chromium.org/10442098/diff/21002/net/dns/address_sorter_win...
net/dns/address_sorter_win.cc:57: printf("FAILED %d\n", WSAGetLastError());
You could make a NetLog entry or something here, if you want.  Regardless, get
rid of the printf.  :)

[mmenke, 2012-06-08 18:12:53]

Oh...Were you not asking for comments yet?  I didn't even read what you said,
just saw the "please review" and put it on my TODO list.

On 2012/06/07 01:48:04, szym wrote:
> If you feel adventurous, this should now work on Windows (Vista+). I need to
> devise a testing plan, although I'm leaning towards testing just whether the
> ioctl call returns true. 
> 
> So far, in my Debug build this ioctl takes ~1ms (measurement resolution), so
I'm
> proposing we keep it on the IO thread, but the machine is rather overpowered,
so
> I'd appreciate if you tested it on a weaker box.
> 
> On Linux, source address information is needed, so this will have to wait for
> another CL which will add AddressTracker to NetworkChangeNotifierLinux.

[mmenke, 2012-06-14 15:02:35]

Are you waiting for more comments from me on this CL, or still working on it?

[szym, 2012-06-14 15:24:19]

Still working on AddressTracker which will be used by Linux.
Alternatively, to keep this CL small, I could land only the OS_WIN
version for now, or perform sort using |mock_source_info_| only.

On Thu, Jun 14, 2012 at 11:02 AM,  <mmenke@chromium.org> wrote:
> Are you waiting for more comments from me on this CL, or still working on
> it?
>
> http://codereview.chromium.org/10442098/

[mmenke, 2012-06-14 15:32:16]

I'm fine with this as a single CL.  All the new complexity is in the posix code,
anyways.

On 2012/06/14 15:24:19, szym wrote:
> Still working on AddressTracker which will be used by Linux.
> Alternatively, to keep this CL small, I could land only the OS_WIN
> version for now, or perform sort using |mock_source_info_| only.
> 
> On Thu, Jun 14, 2012 at 11:02 AM,  <mailto:mmenke@chromium.org> wrote:
> > Are you waiting for more comments from me on this CL, or still working on
> > it?
> >
> > http://codereview.chromium.org/10442098/

[szym, 2012-07-25 21:58:33]

It took a while, but this is now ready for review. Some things have been left
out intentionally in this CL: 

1) Support for determining |native| on Linux. glibc implementation uses
RTNETLINK NEWLINK announcement to identify interface type. BSD and Android
implementations don't support it.

2) Reading gai.conf to allow override (Linux only).

[cbentzel, 2012-07-26 20:33:37]

Likely won't get to it until tomorrow.

On Wed, Jul 25, 2012 at 5:58 PM, <szym@chromium.org> wrote:

> It took a while, but this is now ready for review. Some things have been
> left
> out intentionally in this CL:
>
> 1) Support for determining |native| on Linux. glibc implementation uses
> RTNETLINK NEWLINK announcement to identify interface type. BSD and Android
> implementations don't support it.
>
> 2) Reading gai.conf to allow override (Linux only).
>
>
>
http://codereview.chromium.**org/10442098/<http://codereview.chromium.org/104...
>

[mmenke, 2012-07-26 20:34:47]

On 2012/07/26 20:33:37, cbentzel wrote:
> Likely won't get to it until tomorrow.
> 
> On Wed, Jul 25, 2012 at 5:58 PM, <mailto:szym@chromium.org> wrote:
> 
> > It took a while, but this is now ready for review. Some things have been
> > left
> > out intentionally in this CL:
> >
> > 1) Support for determining |native| on Linux. glibc implementation uses
> > RTNETLINK NEWLINK announcement to identify interface type. BSD and Android
> > implementations don't support it.
> >
> > 2) Reading gai.conf to allow override (Linux only).
> >
> >
> >
>
http://codereview.chromium.**org/10442098/%3Chttp://codereview.chromium.org/1...>
> >

Sorry, meant to look at this earlier today, but forgot about it.  I'll try and
get some preliminary feedback tonight.

[cbentzel, 2012-07-26 20:35:00]

On 2012/07/26 20:33:37, cbentzel wrote:
> Likely won't get to it until tomorrow.

Yeah - took a glance at address_sorter_*.cc and this will not be a trivial
review. 

> 
> On Wed, Jul 25, 2012 at 5:58 PM, <mailto:szym@chromium.org> wrote:
> 
> > It took a while, but this is now ready for review. Some things have been
> > left
> > out intentionally in this CL:
> >
> > 1) Support for determining |native| on Linux. glibc implementation uses
> > RTNETLINK NEWLINK announcement to identify interface type. BSD and Android
> > implementations don't support it.
> >
> > 2) Reading gai.conf to allow override (Linux only).
> >
> >
> >
>
http://codereview.chromium.**org/10442098/%3Chttp://codereview.chromium.org/1...>
> >

[mmenke, 2012-07-27 20:51:47]

Still working my way through the ietf draft for the second time, but thought I'd
point out there have been 3 new versions.  Latest one is
http://tools.ietf.org/html/draft-ietf-6man-rfc3484bis-06

[cbentzel, 2012-07-30 15:30:14]

I haven't even gone through the details, just trying to understand the
interface.

http://codereview.chromium.org/10442098/diff/77004/net/dns/address_sorter.h
File net/dns/address_sorter.h (right):

http://codereview.chromium.org/10442098/diff/77004/net/dns/address_sorter.h#n...
net/dns/address_sorter.h:11: namespace net {
Would help to have file/class specific comment here.

http://codereview.chromium.org/10442098/diff/77004/net/dns/address_sorter.h#n...
net/dns/address_sorter.h:15: class AddressSorter {
Are there any thread restrictions?

http://codereview.chromium.org/10442098/diff/77004/net/dns/address_sorter.h#n...
net/dns/address_sorter.h:22: // Creates platform-dependent AddressSorter.
Can it be created multiple times?

[mmenke, 2012-07-30 15:37:00]

Some comments I put together last week.  Didn't have time to work my way through
the entire spec last week.  Plan to spend this afternoon on it.

http://codereview.chromium.org/10442098/diff/77004/net/base/host_resolver_imp...
File net/base/host_resolver_impl.cc (right):

http://codereview.chromium.org/10442098/diff/77004/net/base/host_resolver_imp...
net/base/host_resolver_impl.cc:1091: OnFailure(net_error,
DnsResponse::DNS_PARSE_OK);
Might want to distinguish in histograms the first query failing and the second
one, so we have a chance of noticing AAAA-hostile DNS servers in our stats.

May also be interesting to have a histogram with the ratio of A time to AAAA
time for the same request.

No need to do anything in this CL, just throwing ideas out.

http://codereview.chromium.org/10442098/diff/77004/net/base/host_resolver_imp...
net/base/host_resolver_impl.cc:1161: OnFailure(ERR_NAME_NOT_RESOLVED,
DnsResponse::DNS_PARSE_OK);
Should we be recording DNS_NO_ADDRESSES here, or some new error?

http://codereview.chromium.org/10442098/diff/77004/net/base/host_resolver_imp...
net/base/host_resolver_impl.cc:1191: // Results from the first transaction. Used
only if |family_| is unspecified.
nit:  Convention seems to be to put the TODOs after other comments, probably so
it's clear the other comments aren't part of the TODO.

http://codereview.chromium.org/10442098/diff/77004/net/base/host_resolver_imp...
net/base/host_resolver_impl.cc:1292: // this Job marking it as cancelled, and
deletes it.
nit:  "Job, marking"

http://codereview.chromium.org/10442098/diff/77004/net/base/host_resolver_imp...
File net/base/host_resolver_impl_unittest.cc (right):

http://codereview.chromium.org/10442098/diff/77004/net/base/host_resolver_imp...
net/base/host_resolver_impl_unittest.cc:1323: TEST_F(HostResolverImplDnsTest,
DnsTaskUnspec) {
Should we do any verification of sort order?

http://codereview.chromium.org/10442098/diff/77004/net/base/host_resolver_imp...
net/base/host_resolver_impl_unittest.cc:1338: EXPECT_EQ(1u,
requests_[2]->NumberOfAddresses());
nit:  The general convention (Both in Chrome and elsewhere in this file) is to
check the number before individual items.

http://codereview.chromium.org/10442098/diff/77004/net/dns/address_sorter_pos...
File net/dns/address_sorter_posix.cc (right):

http://codereview.chromium.org/10442098/diff/77004/net/dns/address_sorter_pos...
net/dns/address_sorter_posix.cc:8: #include <netinet/in.h>
nit:  Not in alphabetical order.

http://codereview.chromium.org/10442098/diff/77004/net/dns/address_sorter_pos...
net/dns/address_sorter_posix.cc:15: #include <string.h>
Why is this needed?

http://codereview.chromium.org/10442098/diff/77004/net/dns/address_sorter_pos...
net/dns/address_sorter_posix.cc:42: //
http://tools.ietf.org/html/draft-ietf-6man-rfc3484bis-03
They're up to http://tools.ietf.org/html/draft-ietf-6man-rfc3484bis-06 now.

http://codereview.chromium.org/10442098/diff/77004/net/dns/address_sorter_pos...
net/dns/address_sorter_posix.cc:75: return GetPolicyValue(table,
ConvertIPv4NumberToIPv6Number(address));
I think converting everything up front may be a little less error prone, and
guarantees consistent behavior with IPv4-mapped addresses, which we don't have
now.  Only downside is we don't preserve whether something starts out as
IPv4-mapped or true IPv4, but we don't do that on Windows, either.

http://codereview.chromium.org/10442098/diff/77004/net/dns/address_sorter_pos...
net/dns/address_sorter_posix.cc:124: if (address.size() == kIPv6AddressSize) {
Should we handle IPv4 mapped addresses here?  If so, sounds like IPv4-mapped
addresses should use |table|.

http://codereview.chromium.org/10442098/diff/77004/net/dns/address_sorter_pos...
net/dns/address_sorter_posix.cc:186: PolicyEntry kDefaultScopeTable[] = {
Think this name is very confusing.  May be worth just inlining the logic. 
Alternatively, explicitly call it kIPv4ScopeTable, and call it ipv4_scope_table
everywhere.

http://codereview.chromium.org/10442098/diff/77004/net/dns/address_sorter_pos...
net/dns/address_sorter_posix.cc:437: LOG(WARNING) << "could not ToSockAddr " <<
rv;
FromSockAddr?

http://codereview.chromium.org/10442098/diff/77004/net/dns/address_sorter_pos...
net/dns/address_sorter_posix.cc:443: LOG(WARNING) << "could not ToSockAddr " <<
rv;
FromSockAddr?

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

http://codereview.chromium.org/10442098/diff/77004/net/dns/address_sorter_win...
net/dns/address_sorter_win.cc:21: // If |ipe| has IPv4-mapped IPv6 address,
converts it to IPv4.
nit:  "and leaves all other addresses unmodified."

http://codereview.chromium.org/10442098/diff/77004/net/dns/address_sorter_win...
net/dns/address_sorter_win.cc:22: void ConvertIPv4MappedToIPv4(IPEndPoint* ipe)
{
May want to put this in net_util.cc.  I don't see it being needed elsewhere, but
if it is, nice to have it easy to find.

http://codereview.chromium.org/10442098/diff/77004/net/dns/address_sorter_win...
net/dns/address_sorter_win.cc:75: sort_list, heap_size, &result_size, NULL,
NULL);
Wonder if this ever eats addresses depending on local connectivity state. 
Mostly concerned about it breaking unit tests.

[cbentzel, 2012-07-30 15:53:43]

I definitely need to go back over the AddressSorter implementations. Mainly
happy with interface and impact on HostResolverImpl

http://codereview.chromium.org/10442098/diff/77004/net/base/host_resolver_imp...
File net/base/host_resolver_impl.cc (right):

http://codereview.chromium.org/10442098/diff/77004/net/base/host_resolver_imp...
net/base/host_resolver_impl.cc:1052: DnsTask(DnsClient* client,
Wondering if it would be better to pass in DnsTransactionFactory and
AddressSorter rather than DnsClient. Since this is not a public interface, I
don't lean too strongly one way or another.

http://codereview.chromium.org/10442098/diff/77004/net/base/host_resolver_imp...
net/base/host_resolver_impl.cc:1120: dns_protocol::kTypeAAAA,
Why don't we issue A and AAAA in parallel for all search suffixes? Is there a
plan to eventually do that?

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

http://codereview.chromium.org/10442098/diff/77004/net/dns/address_sorter_win...
net/dns/address_sorter_win.cc:74: int result = WSAIoctl(socket_,
SIO_ADDRESS_LIST_SORT, sort_list, heap_size,
Could this be blocking the IO thread?

http://codereview.chromium.org/10442098/diff/77004/net/dns/dns_client.h
File net/dns/dns_client.h (right):

http://codereview.chromium.org/10442098/diff/77004/net/dns/dns_client.h#newco...
net/dns/dns_client.h:33: virtual AddressSorter* GetAddressSorter() = 0;
Should this clarify how long the returned pointer is valid for?

[szym, 2012-07-30 16:28:45]

http://codereview.chromium.org/10442098/diff/77004/net/base/host_resolver_imp...
File net/base/host_resolver_impl_unittest.cc (right):

http://codereview.chromium.org/10442098/diff/77004/net/base/host_resolver_imp...
net/base/host_resolver_impl_unittest.cc:1323: TEST_F(HostResolverImplDnsTest,
DnsTaskUnspec) {
On 2012/07/30 15:37:00, Matt Menke wrote:
> Should we do any verification of sort order?

In this test, the AddressSorter is a mock so it does not perform any sorting. We
could verify that IPv6 are expected before IPv4 (see DnsTask), but are we fully
certain that we will be keeping this behavior?

http://codereview.chromium.org/10442098/diff/77004/net/dns/address_sorter.h
File net/dns/address_sorter.h (right):

http://codereview.chromium.org/10442098/diff/77004/net/dns/address_sorter.h#n...
net/dns/address_sorter.h:15: class AddressSorter {
On 2012/07/30 15:30:14, cbentzel wrote:
> Are there any thread restrictions?

This is implementation-dependent, but also not expected to be used from more
than one thread. Current implementations are NonThreadSafe, like most other
classes in net/. Should I make it explicit in the ABC or in the implementation?

http://codereview.chromium.org/10442098/diff/77004/net/dns/address_sorter.h#n...
net/dns/address_sorter.h:22: // Creates platform-dependent AddressSorter.
On 2012/07/30 15:30:14, cbentzel wrote:
> Can it be created multiple times?

I was hoping that scoped_ptr<> indicated that the caller owned it, so there's no
reason to suspect that it couldn't be created multiple times.

http://codereview.chromium.org/10442098/diff/77004/net/dns/address_sorter_pos...
File net/dns/address_sorter_posix.cc (right):

http://codereview.chromium.org/10442098/diff/77004/net/dns/address_sorter_pos...
net/dns/address_sorter_posix.cc:8: #include <netinet/in.h>
On 2012/07/30 15:37:00, Matt Menke wrote:
> nit:  Not in alphabetical order.

"stdint.h" is needed for "netinet/in.h", but it seems that in.h includes
stdint.h, so I'll see if it builds without it.

http://codereview.chromium.org/10442098/diff/77004/net/dns/address_sorter_pos...
net/dns/address_sorter_posix.cc:15: #include <string.h>
On 2012/07/30 15:37:00, Matt Menke wrote:
> Why is this needed?

memcpy in line 453. But now I see it's included in "base/basictypes.h" so I'll
switch to that.

http://codereview.chromium.org/10442098/diff/77004/net/dns/address_sorter_pos...
net/dns/address_sorter_posix.cc:75: return GetPolicyValue(table,
ConvertIPv4NumberToIPv6Number(address));
On 2012/07/30 15:37:00, Matt Menke wrote:
> I think converting everything up front may be a little less error prone, and
> guarantees consistent behavior with IPv4-mapped addresses, which we don't have
> now.  Only downside is we don't preserve whether something starts out as
> IPv4-mapped or true IPv4, but we don't do that on Windows, either.

I agree, although converting everything up front will require different logic in
|GetScope|.

http://codereview.chromium.org/10442098/diff/77004/net/dns/address_sorter_pos...
net/dns/address_sorter_posix.cc:124: if (address.size() == kIPv6AddressSize) {
On 2012/07/30 15:37:00, Matt Menke wrote:
> Should we handle IPv4 mapped addresses here?  If so, sounds like IPv4-mapped
> addresses should use |table|.

Good question. Now that I think about it, I wonder if DNS server can ever return
an IPv4-mapped address in AAAA record. According to
http://www.kame.net/newsletter/20000216/ IPv4 mapped addresses "MUST NOT" be
used in AAAA records nor in /etc/hosts.

http://codereview.chromium.org/10442098/diff/77004/net/dns/address_sorter_pos...
net/dns/address_sorter_posix.cc:186: PolicyEntry kDefaultScopeTable[] = {
On 2012/07/30 15:37:00, Matt Menke wrote:
> Think this name is very confusing.  May be worth just inlining the logic. 
> Alternatively, explicitly call it kIPv4ScopeTable, and call it
ipv4_scope_table
> everywhere.

The reason why this exists (and has 'Default' in name) is because /etc/gai.conf
(see scopev4 in `man gai.conf`) can override it. If we want to support that then
it needs to be explicit.

I can add 'IPv4' to the name, here and where it is used.

http://codereview.chromium.org/10442098/diff/77004/net/dns/address_sorter_pos...
net/dns/address_sorter_posix.cc:401: return;
There's a potential race during shutdown of NetworkChangeNotifier. Currently,
NCN can live on any thread, so it can be destroyed on any thread. Consequently,
the result of NCN::GetAddressTracker might disappear at any time.

I see two potential solutions:
1) Make NCN live on the I/O thread and GetAddressTracker only legal on the I/O
thread.
2) Make AddressTracker internal to NCN and have another copy for
AddressSorterPosix. In the original review of AddressTracker
(https://chromiumcodereview.appspot.com/10689015), vandebo suggested that this
is redundant, but the threading around NCN complicates things.

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

http://codereview.chromium.org/10442098/diff/77004/net/dns/address_sorter_win...
net/dns/address_sorter_win.cc:74: int result = WSAIoctl(socket_,
SIO_ADDRESS_LIST_SORT, sort_list, heap_size,
On 2012/07/30 15:53:43, cbentzel wrote:
> Could this be blocking the IO thread? 

Yes, but no more than GetAdaptersAddresses. The work that this ioctl performs is
comparable to the work performed in AddressSorterPosix with the additional
requirement to enumerate the source addresses.

In my experiments so far, AddressSorterWin::Sort mostly measures at 0ms, and
never breaks 1ms. I'll try to test it on a slow XP machine, and I'm prepared to
move it over to a WorkerPool.

http://codereview.chromium.org/10442098/diff/77004/net/dns/address_sorter_win...
net/dns/address_sorter_win.cc:75: sort_list, heap_size, &result_size, NULL,
NULL);
On 2012/07/30 15:37:00, Matt Menke wrote:
> Wonder if this ever eats addresses depending on local connectivity state. 
> Mostly concerned about it breaking unit tests.

I'll need to test it, but either way I wouldn't test the the contents of the
resulting AddressList, just that the WSAIoctl (and AddressSorterWin*::Sort)
succeeds.

http://codereview.chromium.org/10442098/diff/77004/net/dns/dns_client.h
File net/dns/dns_client.h (right):

http://codereview.chromium.org/10442098/diff/77004/net/dns/dns_client.h#newco...
net/dns/dns_client.h:33: virtual AddressSorter* GetAddressSorter() = 0;
On 2012/07/30 15:53:43, cbentzel wrote:
> Should this clarify how long the returned pointer is valid for? 

As long as GetTransactionFactory. I'll add explicit comments.

[mmenke1, 2012-07-30 17:19:06]

http://codereview.chromium.org/10442098/diff/77004/net/base/host_resolver_imp...
File net/base/host_resolver_impl_unittest.cc (right):

http://codereview.chromium.org/10442098/diff/77004/net/base/host_resolver_imp...
net/base/host_resolver_impl_unittest.cc:1323: TEST_F(HostResolverImplDnsTest,
DnsTaskUnspec) {
On 2012/07/30 16:28:45, szym wrote:
> On 2012/07/30 15:37:00, Matt Menke wrote:
> > Should we do any verification of sort order?
> 
> In this test, the AddressSorter is a mock so it does not perform any sorting.
We
> could verify that IPv6 are expected before IPv4 (see DnsTask), but are we
fully
> certain that we will be keeping this behavior?

I was thinking more along the lines of Linux-specific tests for ordering, and
maybe a general Windows one just to exercise the code and make sure we get out
what we put in.

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

http://codereview.chromium.org/10442098/diff/77004/net/dns/address_sorter_win...
net/dns/address_sorter_win.cc:74: int result = WSAIoctl(socket_,
SIO_ADDRESS_LIST_SORT, sort_list, heap_size,
On 2012/07/30 16:28:45, szym wrote:
> On 2012/07/30 15:53:43, cbentzel wrote:
> > Could this be blocking the IO thread? 
> 
> Yes, but no more than GetAdaptersAddresses. The work that this ioctl performs
is
> comparable to the work performed in AddressSorterPosix with the additional
> requirement to enumerate the source addresses.
> 
> In my experiments so far, AddressSorterWin::Sort mostly measures at 0ms, and
> never breaks 1ms. I'll try to test it on a slow XP machine, and I'm prepared
to
> move it over to a WorkerPool.

If I recall correctly, eroman has said previously that GetAdaptersAddresses has
been a cause of jank.

[szym, 2012-07-30 17:34:49]

Given the feedback so far, I'll be adding unittests to exercise
AddressSorterPosix and I'll move the WSAIoctl call to WorkerPool.

[cbentzel, 2012-07-30 18:18:46]

http://codereview.chromium.org/10442098/diff/77004/net/dns/address_sorter.h
File net/dns/address_sorter.h (right):

http://codereview.chromium.org/10442098/diff/77004/net/dns/address_sorter.h#n...
net/dns/address_sorter.h:15: class AddressSorter {
On 2012/07/30 16:28:45, szym wrote:
> On 2012/07/30 15:30:14, cbentzel wrote:
> > Are there any thread restrictions?
> 
> This is implementation-dependent, but also not expected to be used from more
> than one thread. Current implementations are NonThreadSafe, like most other
> classes in net/. Should I make it explicit in the ABC or in the
implementation?

I think thread-safety is actually part of the interface, not the implementation
(assuming it impacts which threads it can be created and destroyed on, as well
as where Sort can be called).

http://codereview.chromium.org/10442098/diff/77004/net/dns/address_sorter.h#n...
net/dns/address_sorter.h:22: // Creates platform-dependent AddressSorter.
On 2012/07/30 16:28:45, szym wrote:
> On 2012/07/30 15:30:14, cbentzel wrote:
> > Can it be created multiple times?
> 
> I was hoping that scoped_ptr<> indicated that the caller owned it, so there's
no
> reason to suspect that it couldn't be created multiple times.

Yeah, I think this is reasonable.

[szym, 2012-07-30 18:24:16]

http://codereview.chromium.org/10442098/diff/77004/net/dns/address_sorter.h
File net/dns/address_sorter.h (right):

http://codereview.chromium.org/10442098/diff/77004/net/dns/address_sorter.h#n...
net/dns/address_sorter.h:15: class AddressSorter {
On 2012/07/30 18:18:47, cbentzel wrote:
> On 2012/07/30 16:28:45, szym wrote:
> > On 2012/07/30 15:30:14, cbentzel wrote:
> > > Are there any thread restrictions?
> > 
> > This is implementation-dependent, but also not expected to be used from more
> > than one thread. Current implementations are NonThreadSafe, like most other
> > classes in net/. Should I make it explicit in the ABC or in the
> implementation?
> 
> I think thread-safety is actually part of the interface, not the
implementation
> (assuming it impacts which threads it can be created and destroyed on, as well
> as where Sort can be called).
> 

AddressSorterWin* has no essential state so it could be entirely thread-safe. It
currently caches the socket for ioctl, but once I move it to a WorkerPool, I'll
have to open a new socket every time. Consequently, AddressSorterWin* will be
entirely thread-safe.

In contrast, AddressSorterPosix maintains a SourceAddressMap which makes it
NonThreadSafe. It also uses a ClientSocketFactory (which is NonThreadSafe).
Therefore, Sort and dtor must be called on the same thread as ctor.

[mmenke1, 2012-07-30 18:27:00]

On 2012/07/30 18:24:16, szym wrote:
> http://codereview.chromium.org/10442098/diff/77004/net/dns/address_sorter.h
> File net/dns/address_sorter.h (right):
> 
>
http://codereview.chromium.org/10442098/diff/77004/net/dns/address_sorter.h#n...
> net/dns/address_sorter.h:15: class AddressSorter {
> On 2012/07/30 18:18:47, cbentzel wrote:
> > On 2012/07/30 16:28:45, szym wrote:
> > > On 2012/07/30 15:30:14, cbentzel wrote:
> > > > Are there any thread restrictions?
> > > 
> > > This is implementation-dependent, but also not expected to be used from
more
> > > than one thread. Current implementations are NonThreadSafe, like most
other
> > > classes in net/. Should I make it explicit in the ABC or in the
> > implementation?
> > 
> > I think thread-safety is actually part of the interface, not the
> implementation
> > (assuming it impacts which threads it can be created and destroyed on, as
well
> > as where Sort can be called).
> > 
> 
> AddressSorterWin* has no essential state so it could be entirely thread-safe.
It
> currently caches the socket for ioctl, but once I move it to a WorkerPool,
I'll
> have to open a new socket every time. Consequently, AddressSorterWin* will be
> entirely thread-safe.
> 
> In contrast, AddressSorterPosix maintains a SourceAddressMap which makes it
> NonThreadSafe. It also uses a ClientSocketFactory (which is NonThreadSafe).
> Therefore, Sort and dtor must be called on the same thread as ctor.

Have you verified that AddressSorterWin really requires a socket?  Specs say
that it ignores whatever the sockets been bound to, if anything, I believe. 
Seems kinda weird.  There's a new way of doing it that doesn't need a socket. 
Too bad it's Vista and later only, I believe.  :(

[szym, 2012-07-30 18:33:55]

On 2012/07/30 18:27:00, mmenke (incorrect) wrote:
> Have you verified that AddressSorterWin really requires a socket?  Specs say
> that it ignores whatever the sockets been bound to, if anything, I believe. 
> Seems kinda weird.  There's a new way of doing it that doesn't need a socket. 
> Too bad it's Vista and later only, I believe.  :(

The new API (CreateSortedAddressPairs) is preferred and does not require a
socket. It does not support XP at all. I could make AddressSorterWinXP use the
SIO_ADDRESS_LIST_SORT ioctl, and AddressSorterWin use the new API if that's
preferable.

Unfortunately, the ioctl requires a socket. It fails with WSAENOTSOCK=10038
otherwise.

[mmenke, 2012-07-30 18:35:52]

On 2012/07/30 18:33:55, szym wrote:
> On 2012/07/30 18:27:00, mmenke (incorrect) wrote:
> > Have you verified that AddressSorterWin really requires a socket?  Specs say
> > that it ignores whatever the sockets been bound to, if anything, I believe. 
> > Seems kinda weird.  There's a new way of doing it that doesn't need a
socket. 
> > Too bad it's Vista and later only, I believe.  :(
> 
> The new API (CreateSortedAddressPairs) is preferred and does not require a
> socket. It does not support XP at all. I could make AddressSorterWinXP use the
> SIO_ADDRESS_LIST_SORT ioctl, and AddressSorterWin use the new API if that's
> preferable.
> 
> Unfortunately, the ioctl requires a socket. It fails with WSAENOTSOCK=10038
> otherwise.

Unless there's a significant performance penalty (Which I don't believe there
is), think we're better off just using the same code on all Windows versions.

[mmenke, 2012-07-30 19:40:34]

http://codereview.chromium.org/10442098/diff/77004/net/dns/address_sorter_pos...
File net/dns/address_sorter_posix.cc (right):

http://codereview.chromium.org/10442098/diff/77004/net/dns/address_sorter_pos...
net/dns/address_sorter_posix.cc:44: //
http://www.akkadia.org/drepper/linux-rfc3484.html
Is this still needed?  Looks like the latest 3484bis includes a similar fix.

http://codereview.chromium.org/10442098/diff/77004/net/dns/address_sorter_pos...
net/dns/address_sorter_posix.cc:155: { { 0x20, 0x01, 0, 0 }, 32, 10 },
The tables look a bit different from the tables in the RFC (Both the 2.1 and
10.4 tables) and the glibc ones you also link to.  Am I missing something?

http://codereview.chromium.org/10442098/diff/77004/net/dns/address_sorter_pos...
net/dns/address_sorter_posix.cc:159: { { 0xFE, 0xC0 }, 16, 1 },
This looks wrong.

fc00::/7 is unique local.
fec0::/10 is site-local (deprecated).
fec0::/16 is a subset of site-local.

http://codereview.chromium.org/10442098/diff/77004/net/dns/address_sorter_pos...
net/dns/address_sorter_posix.cc:186: PolicyEntry kDefaultScopeTable[] = {
On 2012/07/30 16:28:45, szym wrote:
> On 2012/07/30 15:37:00, Matt Menke wrote:
> > Think this name is very confusing.  May be worth just inlining the logic. 
> > Alternatively, explicitly call it kIPv4ScopeTable, and call it
> ipv4_scope_table
> > everywhere.
> 
> The reason why this exists (and has 'Default' in name) is because
/etc/gai.conf
> (see scopev4 in `man gai.conf`) can override it. If we want to support that
then
> it needs to be explicit.
> 
> I can add 'IPv4' to the name, here and where it is used.

Curiously, man gai.conf didn't mention the field, but the file itself did have
comments about it.

http://codereview.chromium.org/10442098/diff/77004/net/dns/address_sorter_pos...
net/dns/address_sorter_posix.cc:212: unsigned PrefixLength(const
IPAddressNumber& addr) {
Suggest you rename this MaskPrefixLength(addr_mask), since it takes masks,
rather than addresses.

http://codereview.chromium.org/10442098/diff/77004/net/dns/address_sorter_pos...
net/dns/address_sorter_posix.cc:240: bool CompareElements(const SortElement* a1,
const SortElement* a2) {
Think a1 / a2 could be named better.  Suggest dest_a / dest_b (A/B in keeping
with the RFC).

http://codereview.chromium.org/10442098/diff/77004/net/dns/address_sorter_pos...
net/dns/address_sorter_posix.cc:258: return !a1->src->home;
Shouldn't this be "return a1->src->home;"? (No negation)

http://codereview.chromium.org/10442098/diff/77004/net/dns/address_sorter_pos...
net/dns/address_sorter_posix.cc:272: return !a1->src->native;
Believe the negation here is also wrong.

http://codereview.chromium.org/10442098/diff/77004/net/dns/address_sorter_pos...
net/dns/address_sorter_posix.cc:339: scoped_ptr<SortElement> el(new
SortElement());
Think using "el" for a variable name violates Google style.  I'd suggest
renaming SortElement (Which is also too vague, in my opinion) to
DestinationSortInfo or DestinationInfo, and using something like "dest_info" or
"sort_info", or even just "dest" here.

http://codereview.chromium.org/10442098/diff/77004/net/dns/address_sorter_pos...
net/dns/address_sorter_posix.cc:361: // Filter out unsable destinations.
nit:  unuseable

http://codereview.chromium.org/10442098/diff/77004/net/dns/address_sorter_pos...
net/dns/address_sorter_posix.cc:376: el->src = &mock_source_info_;
Wonder if this case could occur often enough that it's worth getting the scope,
label, and precedence.

[szym, 2012-07-30 20:55:52]

http://codereview.chromium.org/10442098/diff/77004/net/dns/address_sorter_pos...
File net/dns/address_sorter_posix.cc (right):

http://codereview.chromium.org/10442098/diff/77004/net/dns/address_sorter_pos...
net/dns/address_sorter_posix.cc:44: //
http://www.akkadia.org/drepper/linux-rfc3484.html
On 2012/07/30 19:40:34, Matt Menke wrote:
> Is this still needed?  Looks like the latest 3484bis includes a similar fix.

You're right. All problems raised there are already addressed in the RFC'bis,
but I just realized there is still a difference between the current glibc
implementation and the RFC in how NATed IPv4 addresses are treated.

This is also the difference between glibc and bionic implementations. In glibc,
NATed IPv4 addresses (192.168.0.0/16, 172.16.0.0/12, 10.0.0.0/8) are assigned
site-local scope as in the original
http://tools.ietf.org/html/rfc3484#section-3.2 and the problem is fixed by using
the label policy table:
/* The next two entries differ from RFC 3484.  We need to treat
   IPv6 site-local addresses special because they are never NATed,
   unlike site-locale IPv4 addresses.  If this would not happen, on
   machines which have only IPv4 and IPv6 site-local addresses, the
   sorting would prefer the IPv6 site-local addresses, causing
   unnecessary delays when trying to connect to a global IPv6 address
   through a site-local IPv6 address.  */
//fec0::/10  label = 5
//fc00::/7   label = 6

In contrast, in bionic's _get_scope:
/*
 * According to draft-ietf-6man-rfc3484-revise-01 section 2.3,
 * it is best not to treat the private IPv4 ranges
 * (10.0.0.0/8, 172.16.0.0/12 and 192.168.0.0/16) as being
 * in a special scope, so we don't.
 */
return IPV6_ADDR_SCOPE_GLOBAL;


RFC'bis also suggests simply assigning global scope, so I think it makes more
sense to use that.

http://codereview.chromium.org/10442098/diff/77004/net/dns/address_sorter_pos...
net/dns/address_sorter_posix.cc:146: // fc00::/7 -- multicast
not "multicast", but "unique local address"

http://codereview.chromium.org/10442098/diff/77004/net/dns/address_sorter_pos...
net/dns/address_sorter_posix.cc:155: { { 0x20, 0x01, 0, 0 }, 32, 10 },
On 2012/07/30 19:40:34, Matt Menke wrote:
> The tables look a bit different from the tables in the RFC (Both the 2.1 and
> 10.4 tables) and the glibc ones you also link to.  Am I missing something?

Ah. As bionic, this is following:
http://tools.ietf.org/html/draft-ietf-6man-rfc3484-revise-01#section-2.1.4
         Prefix        Precedence Label
         ::1/128               60     0
         fc00::/7              50     1
         ::/0                  40     2
         ::ffff:0:0/96         30     3
         2002::/16             20     4
         2001::/32             10     5
         ::/96                  1    10
         fec::/16               1    11
         3ffe::/16              1    12

I didn't realize it myself until now, but this table is slightly different in
RFC3484bis:
      Prefix        Precedence Label
      ::1/128               50     0
      ::/0                  40     1
      ::ffff:0:0/96         35     4
      2002::/16             30     2
      2001::/32              5     5
      fc00::/7               3    13   // ULA degraded from second precedence
      ::/96                  1     3
      fec0::/10              1    11   // Site-local expanded scope (reduced
prefix length)
      3ffe::/16              1    12

[The Label column is a bit perplexing to me in both tables, since the label does
not imply any order. It is compared only as "matching label".]

And the default glibc table is (contrary to man gai.conf):
     Prefix        Precedence Label
     ::1/128       50         0
     ::/0          40         1
     2002::/16     30         2
     ::/96         20         3
     ::ffff:0:0/96 10         4
     fec0::/10     x          5
     fc00::/7      x          6
     2001:0::/32   x          7
(where 'x' implies that precedence for ::/0 [40] is used)

I think I'll go with the RFC'bis table for now.

http://codereview.chromium.org/10442098/diff/77004/net/dns/address_sorter_pos...
net/dns/address_sorter_posix.cc:186: PolicyEntry kDefaultScopeTable[] = {
On 2012/07/30 19:40:34, Matt Menke wrote:
> On 2012/07/30 16:28:45, szym wrote:
> > On 2012/07/30 15:37:00, Matt Menke wrote:
> > > Think this name is very confusing.  May be worth just inlining the logic. 
> > > Alternatively, explicitly call it kIPv4ScopeTable, and call it
> > ipv4_scope_table
> > > everywhere.
> > 
> > The reason why this exists (and has 'Default' in name) is because
> /etc/gai.conf
> > (see scopev4 in `man gai.conf`) can override it. If we want to support that
> then
> > it needs to be explicit.
> > 
> > I can add 'IPv4' to the name, here and where it is used.
> 
> Curiously, man gai.conf didn't mention the field, but the file itself did have
> comments about it.

My "man gai.conf" includes:
====
scopev4 mask value
    Add another rule to the RFC 3484 scope table for IPv4 address.  By default
the scope IDs described in section 3.2 in RFC 3438 are used.  Changing these
defaults should hardly ever be necessary.
====
This is referring to the original RFC3484 which assigned NATed IPv4 addresses
site-local scope.

http://codereview.chromium.org/10442098/diff/77004/net/dns/address_sorter_pos...
net/dns/address_sorter_posix.cc:258: return !a1->src->home;
On 2012/07/30 19:40:34, Matt Menke wrote:
> Shouldn't this be "return a1->src->home;"? (No negation)

Yes.

http://codereview.chromium.org/10442098/diff/77004/net/dns/address_sorter_pos...
net/dns/address_sorter_posix.cc:272: return !a1->src->native;
On 2012/07/30 19:40:34, Matt Menke wrote:
> Believe the negation here is also wrong.

Yes.

http://codereview.chromium.org/10442098/diff/77004/net/dns/address_sorter_pos...
net/dns/address_sorter_posix.cc:376: el->src = &mock_source_info_;
On 2012/07/30 19:40:34, Matt Menke wrote:
> Wonder if this case could occur often enough that it's worth getting the
scope,
> label, and precedence.

If the AddressTrackerLinux implementation is trustworthy, then the time during
which SourceAddressMap does not have the address should be essentially none and
the results quickly eradicated by a flush in HostResolverImpl. However, since I
cannot guarantee the ordering of AddressTrackerLinux::OnIPAddressChanged and
HostResolverImpl::::OnIPAddressChanged, I need to gracefully handle the
staleness to avoid dispatching getaddrinfo (which cannot be cancelled).

If it's more comforting to provide the accurate policy values, I'll change this
to add new entry in |source_info_| if none present.

[mmenke, 2012-07-30 21:07:16]

http://codereview.chromium.org/10442098/diff/77004/net/dns/address_sorter_pos...
File net/dns/address_sorter_posix.cc (right):

http://codereview.chromium.org/10442098/diff/77004/net/dns/address_sorter_pos...
net/dns/address_sorter_posix.cc:376: el->src = &mock_source_info_;
On 2012/07/30 20:55:52, szym wrote:
> On 2012/07/30 19:40:34, Matt Menke wrote:
> > Wonder if this case could occur often enough that it's worth getting the
> scope,
> > label, and precedence.
> 
> If the AddressTrackerLinux implementation is trustworthy, then the time during
> which SourceAddressMap does not have the address should be essentially none
and
> the results quickly eradicated by a flush in HostResolverImpl. However, since
I
> cannot guarantee the ordering of AddressTrackerLinux::OnIPAddressChanged and
> HostResolverImpl::::OnIPAddressChanged, I need to gracefully handle the
> staleness to avoid dispatching getaddrinfo (which cannot be cancelled).
> 
> If it's more comforting to provide the accurate policy values, I'll change
this
> to add new entry in |source_info_| if none present.

That's probably more effort than it's worth...  Assuming the AddressTracker
doesn't spam us with address changes, and that when this happens we do have an
address change pending.

[szym, 2012-08-06 23:22:20]

Summary of changes since last review:
- Made AddressSorter::Sort asynchronous to facilitate WSAIoctl on WorkerPool.
Decided to call through synchronously on AddressSorterPosix.
- Added AddressSorterTest and AddressSorterPosixTest.
- Updated policy tables to match rfc3484bis.
- Addressed nits.

http://codereview.chromium.org/10442098/diff/77004/net/base/host_resolver_imp...
File net/base/host_resolver_impl.cc (right):

http://codereview.chromium.org/10442098/diff/77004/net/base/host_resolver_imp...
net/base/host_resolver_impl.cc:1052: DnsTask(DnsClient* client,
On 2012/07/30 15:53:43, cbentzel wrote:
> Wondering if it would be better to pass in DnsTransactionFactory and
> AddressSorter rather than DnsClient. Since this is not a public interface, I
> don't lean too strongly one way or another. 

It seems simpler to use DnsClient.

http://codereview.chromium.org/10442098/diff/77004/net/base/host_resolver_imp...
net/base/host_resolver_impl.cc:1120: dns_protocol::kTypeAAAA,
On 2012/07/30 15:53:43, cbentzel wrote:
> Why don't we issue A and AAAA in parallel for all search suffixes? Is there a
> plan to eventually do that?

To avoid the "I'm a smart NAT and I'm going to protect the DNS server from DoS
attack by black-holing any packets over 7 simultaneous requests"-issue.

This initial approach aims to be conservative (<=1 outstanding query per Job).

http://codereview.chromium.org/10442098/diff/77004/net/dns/address_sorter_pos...
File net/dns/address_sorter_posix.cc (right):

http://codereview.chromium.org/10442098/diff/77004/net/dns/address_sorter_pos...
net/dns/address_sorter_posix.cc:376: el->src = &mock_source_info_;
On 2012/07/30 21:07:17, Matt Menke wrote:
> On 2012/07/30 20:55:52, szym wrote:
> > On 2012/07/30 19:40:34, Matt Menke wrote:
> > > Wonder if this case could occur often enough that it's worth getting the
> > scope,
> > > label, and precedence.
> > 
> > If the AddressTrackerLinux implementation is trustworthy, then the time
during
> > which SourceAddressMap does not have the address should be essentially none
> and
> > the results quickly eradicated by a flush in HostResolverImpl. However,
since
> I
> > cannot guarantee the ordering of AddressTrackerLinux::OnIPAddressChanged and
> > HostResolverImpl::::OnIPAddressChanged, I need to gracefully handle the
> > staleness to avoid dispatching getaddrinfo (which cannot be cancelled).
> > 
> > If it's more comforting to provide the accurate policy values, I'll change
> this
> > to add new entry in |source_info_| if none present.
> 
> That's probably more effort than it's worth...  Assuming the AddressTracker
> doesn't spam us with address changes, and that when this happens we do have an
> address change pending.

I figured the code looks cleaner without mock_source_info_. The price is the
necessary "mutable" on |source_map_|.

[mmenke, 2012-08-07 18:30:27]

A couple minor comments.  I still want to do a really careful pass or two over
the code, and look more closely at the unit tests.

http://codereview.chromium.org/10442098/diff/71025/net/dns/address_sorter_pos...
File net/dns/address_sorter_posix.cc (right):

http://codereview.chromium.org/10442098/diff/71025/net/dns/address_sorter_pos...
net/dns/address_sorter_posix.cc:74: bool IsMulticast(const IPAddressNumber&
address) {
Should probably call this IsIPv6Multicast.

http://codereview.chromium.org/10442098/diff/71025/net/dns/address_sorter_pos...
net/dns/address_sorter_posix.cc:74: bool IsMulticast(const IPAddressNumber&
address) {
May want to DCHECK on address's length.

http://codereview.chromium.org/10442098/diff/71025/net/dns/address_sorter_pos...
net/dns/address_sorter_posix.cc:78: AddressScope GetMulticastScope(const
IPAddressNumber& address) {
And this GetIPv6MulticastScope.

http://codereview.chromium.org/10442098/diff/71025/net/dns/address_sorter_pos...
net/dns/address_sorter_posix.cc:78: AddressScope GetMulticastScope(const
IPAddressNumber& address) {
Suggest you DCHECK on address's length and address[0] != 0xFF.

http://codereview.chromium.org/10442098/diff/71025/net/dns/address_sorter_pos...
net/dns/address_sorter_posix.cc:87: };
nit:  The style for C-style initialization seems to be to use only two space
indent.  Not mentioned in the style guide, but it seems to be the most
frequently used style in net/ and chrome/.

http://codereview.chromium.org/10442098/diff/71025/net/dns/address_sorter_pos...
net/dns/address_sorter_posix.cc:91: bool IsLinkLocal(const IPAddressNumber&
address) {
Suggest IsIPv6LinkLocal, and IsIPv6SiteLocal below.  Should also DCHECK that the
address is an IPv6 address.

http://codereview.chromium.org/10442098/diff/71025/net/dns/address_sorter_pos...
net/dns/address_sorter_posix.cc:121: // Default policy table. RFC 3484, Section
2.1.
Actually RFC 3484, Httpbis draft 06, Section 2.1.  Also suggest a line break
here, to indicate it applies to more than the next code block.

http://codereview.chromium.org/10442098/diff/71025/net/dns/address_sorter_pos...
net/dns/address_sorter_posix.cc:185: }
NOTREACHED()?

http://codereview.chromium.org/10442098/diff/71025/net/dns/address_sorter_pos...
net/dns/address_sorter_posix.cc:191: unsigned MaskPrefixLength(const
IPAddressNumber& addr) {
Still think you might want to rename addr, since it's not actually an address.

http://codereview.chromium.org/10442098/diff/71025/net/dns/address_sorter_pos...
net/dns/address_sorter_posix.cc:355: #elif defined(OS_MACOSX) || defined(OS_BSD)
Hmm...We don't even have a NetworkChangeNotifier that works on BSD, do we?

http://codereview.chromium.org/10442098/diff/71025/net/dns/address_sorter_pos...
net/dns/address_sorter_posix.cc:362: }
nit:  Brackets not needed

http://codereview.chromium.org/10442098/diff/71025/net/dns/address_sorter_pos...
File net/dns/address_sorter_posix.h (right):

http://codereview.chromium.org/10442098/diff/71025/net/dns/address_sorter_pos...
net/dns/address_sorter_posix.h:44: unsigned label;
nit:  Suggest a blank line here, for legibility.

http://codereview.chromium.org/10442098/diff/71025/net/dns/address_sorter_pos...
net/dns/address_sorter_posix.h:54: class NET_EXPORT_PRIVATE AddressSorterPosix
Suggest a comment that this is non-threadsafe and always completes
synchronously.

http://codereview.chromium.org/10442098/diff/71025/net/dns/address_sorter_pos...
net/dns/address_sorter_posix.h:74: mutable SourceAddressMap source_map_;
Suggest a comment that this is mutable so unrecognized source addresses can be
added with default values.

[szym, 2012-08-07 20:02:48]

I addressed Matt's comments.

http://codereview.chromium.org/10442098/diff/71025/net/dns/address_sorter_pos...
File net/dns/address_sorter_posix.cc (right):

http://codereview.chromium.org/10442098/diff/71025/net/dns/address_sorter_pos...
net/dns/address_sorter_posix.cc:185: }
On 2012/08/07 18:30:27, Matt Menke wrote:
> NOTREACHED()?

Will be reached if |a1 == a2|.

http://codereview.chromium.org/10442098/diff/71025/net/dns/address_sorter_pos...
net/dns/address_sorter_posix.cc:355: #elif defined(OS_MACOSX) || defined(OS_BSD)
On 2012/08/07 18:30:27, Matt Menke wrote:
> Hmm...We don't even have a NetworkChangeNotifier that works on BSD, do we?

We don't. The libc on FreeBSD does not use getifaddrs. Instead, it calls
ioctl(SIOCGIFAFLAG_IN6) on every address in the list:
http://fxr.watson.org/fxr/source/net/getaddrinfo.c?v=FREEBSD8-LIBC;im=bigexce...
(Note the hard-coded "lo0" for interface name.)

[mmenke1, 2012-08-07 20:08:50]

http://codereview.chromium.org/10442098/diff/71025/net/dns/address_sorter_pos...
File net/dns/address_sorter_posix.cc (right):

http://codereview.chromium.org/10442098/diff/71025/net/dns/address_sorter_pos...
net/dns/address_sorter_posix.cc:185: }
On 2012/08/07 20:02:48, szym wrote:
> On 2012/08/07 18:30:27, Matt Menke wrote:
> > NOTREACHED()?
> 
> Will be reached if |a1 == a2|.

No it won't - then diff is 0, and you hit the continue statement before the
inner loop.

http://codereview.chromium.org/10442098/diff/71025/net/dns/address_sorter_pos...
net/dns/address_sorter_posix.cc:355: #elif defined(OS_MACOSX) || defined(OS_BSD)
On 2012/08/07 20:02:48, szym wrote:
> On 2012/08/07 18:30:27, Matt Menke wrote:
> > Hmm...We don't even have a NetworkChangeNotifier that works on BSD, do we?
> 
> We don't. The libc on FreeBSD does not use getifaddrs. Instead, it calls
> ioctl(SIOCGIFAFLAG_IN6) on every address in the list:
>
http://fxr.watson.org/fxr/source/net/getaddrinfo.c?v=FREEBSD8-LIBC;im=bigexce...
> (Note the hard-coded "lo0" for interface name.)

May make sense to call into this just after we see a new source address on BSD,
then.  (Or when we do, though in that case we'd have to protect against doing it
more than once, and infinite loops).

[szym, 2012-08-07 20:13:33]

http://codereview.chromium.org/10442098/diff/71025/net/dns/address_sorter_pos...
File net/dns/address_sorter_posix.cc (right):

http://codereview.chromium.org/10442098/diff/71025/net/dns/address_sorter_pos...
net/dns/address_sorter_posix.cc:185: }
On 2012/08/07 20:08:50, mmenke (incorrect) wrote:
> On 2012/08/07 20:02:48, szym wrote:
> > On 2012/08/07 18:30:27, Matt Menke wrote:
> > > NOTREACHED()?
> > 
> > Will be reached if |a1 == a2|.
> 
> No it won't - then diff is 0, and you hit the continue statement before the
> inner loop.

Sorry, I didn't notice that you're asking for NOTREACHED before the brace, not
after.

[mmenke, 2012-08-09 19:51:11]

I'm pretty happy with this.  Is there a single RFC that talks about scopes?  I
thought I found something last time I looked at the code, but can't remember
what it was.

Also, any thoughts about link-local zones?  I hadn't even been aware of them
until quite recently.

http://codereview.chromium.org/10442098/diff/80068/net/base/host_resolver_imp...
File net/base/host_resolver_impl.cc (right):

http://codereview.chromium.org/10442098/diff/80068/net/base/host_resolver_imp...
net/base/host_resolver_impl.cc:1089:
DNS_HISTOGRAM("AsyncDNS.TransactionFailure",
Random comment for the future - may want to separate out some of these
histograms to separate out IPv6 and IPv4.

http://codereview.chromium.org/10442098/diff/80068/net/base/host_resolver_imp...
net/base/host_resolver_impl.cc:1144: OnFailure(ERR_NAME_NOT_RESOLVED,
DnsResponse::DNS_NO_ADDRESSES);
DnsResponse::DNS_NO_ADDRESSES is no longer actually being used by DnsResponse. 
Seems a little weird to only use it here.

[completely optional] I'd suggest you add a comment to dns_response.h that it's
obsolete, and just use DnsResponse::DNS_PARSE_OK here for consistency.

http://codereview.chromium.org/10442098/diff/80068/net/base/host_resolver_imp...
File net/base/host_resolver_impl_unittest.cc (right):

http://codereview.chromium.org/10442098/diff/80068/net/base/host_resolver_imp...
net/base/host_resolver_impl_unittest.cc:1248: AddDnsRule("6ok",
dns_protocol::kTypeAAAA, MockDnsClientRule::OK);
How about one that succeeds with ipv4 and fails asynchronously with ipv6?  Could
also have one the other way around, but don't think it's too important.

http://codereview.chromium.org/10442098/diff/80068/net/dns/address_sorter_pos...
File net/dns/address_sorter_posix_unittest.cc (right):

http://codereview.chromium.org/10442098/diff/80068/net/dns/address_sorter_pos...
net/dns/address_sorter_posix_unittest.cc:52: virtual int
GetPeerAddress(IPEndPoint* address) const {
nit:  OVERRIDE

http://codereview.chromium.org/10442098/diff/80068/net/dns/address_sorter_pos...
net/dns/address_sorter_posix_unittest.cc:52: virtual int
GetPeerAddress(IPEndPoint* address) const {
Do we even need this function?

http://codereview.chromium.org/10442098/diff/80068/net/dns/address_sorter_pos...
net/dns/address_sorter_posix_unittest.cc:58: virtual int
GetLocalAddress(IPEndPoint* address) const {
nit:  OVERRIDE

http://codereview.chromium.org/10442098/diff/80068/net/dns/address_sorter_pos...
net/dns/address_sorter_posix_unittest.cc:77: virtual const BoundNetLog& NetLog()
const {
nit:  OVERRIDE

http://codereview.chromium.org/10442098/diff/80068/net/dns/address_sorter_pos...
net/dns/address_sorter_posix_unittest.cc:86: IPEndPoint local_endpoint_;
nit:  Last two can be const.

http://codereview.chromium.org/10442098/diff/80068/net/dns/address_sorter_pos...
net/dns/address_sorter_posix_unittest.cc:193: const char* addresses[] = {
"1.2.3.4", "10.0.0.231", "127.0.0.1", NULL };
Just for completeness, might want an IPv6 ip that's not usable here.

http://codereview.chromium.org/10442098/diff/80068/net/dns/address_sorter_pos...
net/dns/address_sorter_posix_unittest.cc:205: AddMapping("fec1::2", "fe81::10");
     // site-local vs. link-local
Suggest you put the bottom 3 in the same order that the tests use them, to make
things more readable.

http://codereview.chromium.org/10442098/diff/80068/net/dns/address_sorter_pos...
net/dns/address_sorter_posix_unittest.cc:210: Verify(addresses, order);
Suggest just using different names (addresses1, addresses2, etc) and getting
ride of the code blocks, for readability.

http://codereview.chromium.org/10442098/diff/80068/net/dns/address_sorter_pos...
net/dns/address_sorter_posix_unittest.cc:285: AddMapping("fe81::1", "fe81::10");
     // matching link-local
nit:  You're generally ordering mappings by sort order, so the matching
link-local should be first.

http://codereview.chromium.org/10442098/diff/80068/net/dns/address_sorter_pos...
net/dns/address_sorter_posix_unittest.cc:285: AddMapping("fe81::1", "fe81::10");
     // matching link-local
By matching, you mean matching everything from previous rules, and the others
match everything except scope?

http://codereview.chromium.org/10442098/diff/80068/net/dns/address_sorter_pos...
net/dns/address_sorter_posix_unittest.cc:300: TEST_F(AddressSorterPosixTest,
Rule9) {
I suggest adding one more entry here:
AddMapping("4080::1", "4000::10");
(And add to addresses as well, of course).

This is just to make sure the inner loop of CommonPrefixLength works - both that
it checks bits in the right order, and that it hits both endpoints.

Alternatively, could just expose CommonPrefixLength and check it directly.

http://codereview.chromium.org/10442098/diff/80068/net/dns/address_sorter_pos...
net/dns/address_sorter_posix_unittest.cc:302:
GetSourceInfo("3000:ffff::10")->prefix_length = 16;
In other tests you put GetSourceInfo just after the corresponding mapping. 
Suggest the same here.  Also suggest flipping the order of the mappings, for
consistency with earlier tests.

http://codereview.chromium.org/10442098/diff/80068/net/dns/address_sorter_pos...
net/dns/address_sorter_posix_unittest.cc:304: AddMapping("4000::1", "4000::10");
      // 120 bit match, limited to 15
Isn't this a 123-bit match?

http://codereview.chromium.org/10442098/diff/80068/net/dns/address_sorter_pos...
net/dns/address_sorter_posix_unittest.cc:324:
GetSourceInfo("fe81::20")->deprecated = true;
nit:  Move down a line, for consistency.

http://codereview.chromium.org/10442098/diff/80068/net/dns/address_sorter_pos...
net/dns/address_sorter_posix_unittest.cc:326: AddMapping("ff3e::1", "4000::10");
 // global multicast, mismatched label
This isn't actually mismatched label, right?  Otherwise, it would be sorted
after "4000::1", rather than just before it.

http://codereview.chromium.org/10442098/diff/80068/net/dns/address_sorter_pos...
net/dns/address_sorter_posix_unittest.cc:327: AddMapping("4000::1", "4000::10");
 // global unicast
Suggest you sort these in order of precedence like in the others.

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

http://codereview.chromium.org/10442098/diff/80068/net/dns/address_sorter_win...
net/dns/address_sorter_win.cc:50: 
nit:  Remove blank line.  The prevailing style is not to have a blank line at
the start of function bodies, even with initializer lists.

http://codereview.chromium.org/10442098/diff/80068/net/dns/address_sorter_win...
net/dns/address_sorter_win.cc:91: buffer_size_, buffer_.get(), buffer_size_,
Are you sure it's safe to use the same input and output buffers?

Also, it's not clear if input buffer size should include the SOCKADDR_STORAGEs
on the end of the list.

http://codereview.chromium.org/10442098/diff/80068/net/dns/address_sorter_win...
net/dns/address_sorter_win.cc:123: size_t buffer_size_;
buffer_size_ and callback_ can both be const.

http://codereview.chromium.org/10442098/diff/80068/net/dns/address_sorter_win...
net/dns/address_sorter_win.cc:168: if (!list_ipv6.empty()) {
NOTREACHED()?  Currently shouldn't reach this when we don't have any IPv6
addresses.

http://codereview.chromium.org/10442098/diff/80068/net/dns/dns_test_util.h
File net/dns/dns_test_util.h (right):

http://codereview.chromium.org/10442098/diff/80068/net/dns/dns_test_util.h#ne...
net/dns/dns_test_util.h:181: } result;
Don't think the style guide allows declaring an enum like this.  Enums are
supposed to be declared first, data members last.

[szym, 2012-08-10 05:56:01]

Thanks for the review. Some comments up-front before I upload the next patch.

http://codereview.chromium.org/10442098/diff/80068/net/base/host_resolver_imp...
File net/base/host_resolver_impl.cc (right):

http://codereview.chromium.org/10442098/diff/80068/net/base/host_resolver_imp...
net/base/host_resolver_impl.cc:1089:
DNS_HISTOGRAM("AsyncDNS.TransactionFailure",
On 2012/08/09 19:51:11, Matt Menke wrote:
> Random comment for the future - may want to separate out some of these
> histograms to separate out IPv6 and IPv4.

Right, and because currently we are querying for FQDN in the second transaction,
we'd also need to differentiate IPv6-only from IPv6-after-IPv4.

I plan to add measurement of DnsUDPAttempt, and in addition to IPv4/IPv6 it will
differentiate attempts using the suffix search (which are likely intranet) and
those which are not.

http://codereview.chromium.org/10442098/diff/80068/net/base/host_resolver_imp...
net/base/host_resolver_impl.cc:1144: OnFailure(ERR_NAME_NOT_RESOLVED,
DnsResponse::DNS_NO_ADDRESSES);
On 2012/08/09 19:51:11, Matt Menke wrote:
> DnsResponse::DNS_NO_ADDRESSES is no longer actually being used by DnsResponse.

> Seems a little weird to only use it here.
> 
> [completely optional] I'd suggest you add a comment to dns_response.h that
it's
> obsolete, and just use DnsResponse::DNS_PARSE_OK here for consistency.

I'd like to keep it for now for the sake of the AsyncDNS.ResultError histogram.

http://codereview.chromium.org/10442098/diff/80068/net/dns/address_sorter_pos...
File net/dns/address_sorter_posix_unittest.cc (right):

http://codereview.chromium.org/10442098/diff/80068/net/dns/address_sorter_pos...
net/dns/address_sorter_posix_unittest.cc:285: AddMapping("fe81::1", "fe81::10");
     // matching link-local
On 2012/08/09 19:51:11, Matt Menke wrote:
> By matching, you mean matching everything from previous rules, and the others
> match everything except scope?

I meant matching scope. Within both groups Rule 8 is the first distinguishing
factor, but the first two go higher due to Rule 2.

http://codereview.chromium.org/10442098/diff/80068/net/dns/address_sorter_pos...
net/dns/address_sorter_posix_unittest.cc:304: AddMapping("4000::1", "4000::10");
      // 120 bit match, limited to 15
On 2012/08/09 19:51:11, Matt Menke wrote:
> Isn't this a 123-bit match?

Right.

http://codereview.chromium.org/10442098/diff/80068/net/dns/address_sorter_pos...
net/dns/address_sorter_posix_unittest.cc:326: AddMapping("ff3e::1", "4000::10");
 // global multicast, mismatched label
On 2012/08/09 19:51:11, Matt Menke wrote:
> This isn't actually mismatched label, right?  Otherwise, it would be sorted
> after "4000::1", rather than just before it.

That's right. I keep forgetting that multicast addresses don't get a different
label.

http://codereview.chromium.org/10442098/diff/80068/net/dns/address_sorter_pos...
net/dns/address_sorter_posix_unittest.cc:327: AddMapping("4000::1", "4000::10");
 // global unicast
On 2012/08/09 19:51:11, Matt Menke wrote:
> Suggest you sort these in order of precedence like in the others.

Do you mean make |order| 4, 3, 2, 1, 0, -1?

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

http://codereview.chromium.org/10442098/diff/80068/net/dns/address_sorter_win...
net/dns/address_sorter_win.cc:91: buffer_size_, buffer_.get(), buffer_size_,
On 2012/08/09 19:51:11, Matt Menke wrote:
> Are you sure it's safe to use the same input and output buffers?
> 
> Also, it's not clear if input buffer size should include the SOCKADDR_STORAGEs
> on the end of the list.

I am fairly certain that the answer is yes to both questions. I have no
documentation confirming the first one so I'll change it to two buffers for
safety. Regarding the second, AFAIK: In ioctl calls, all of the memory must be
contained within the user process and all of it has to be allocated before the
call, so there's no way for the ioctl call to complete if it cannot fit
SOCKADDR_STORAGE inside the buffer.

What I wonder about though is whether using overlapped calls would allow us to
avoid calling the WorkerPool. I've previously experimented with that but
observed that the call would block (measured elapsed time could still reach into
10ms).

http://codereview.chromium.org/10442098/diff/80068/net/dns/address_sorter_win...
net/dns/address_sorter_win.cc:168: if (!list_ipv6.empty()) {
On 2012/08/09 19:51:11, Matt Menke wrote:
> NOTREACHED()?  Currently shouldn't reach this when we don't have any IPv6
> addresses.

Ok, I'll add this requirement to the spec.

[mmenke, 2012-08-10 14:20:11]

http://codereview.chromium.org/10442098/diff/80068/net/dns/address_sorter_pos...
File net/dns/address_sorter_posix_unittest.cc (right):

http://codereview.chromium.org/10442098/diff/80068/net/dns/address_sorter_pos...
net/dns/address_sorter_posix_unittest.cc:285: AddMapping("fe81::1", "fe81::10");
     // matching link-local
On 2012/08/10 05:56:01, szym wrote:
> On 2012/08/09 19:51:11, Matt Menke wrote:
> > By matching, you mean matching everything from previous rules, and the
others
> > match everything except scope?
> 
> I meant matching scope. Within both groups Rule 8 is the first distinguishing
> factor, but the first two go higher due to Rule 2.

Right, but for both sets, earlier rules must not distinguish the entries, too,
for scope to matter.  The "ff3#:1", "4000:10" entries left me wondering about
label and precedence.

http://codereview.chromium.org/10442098/diff/80068/net/dns/address_sorter_pos...
net/dns/address_sorter_posix_unittest.cc:294: NULL };
nit:  You put each entry on its own line here, but not in Rule6 or
MultipleRules.  Suggest you make them consistent, one way or the other.

http://codereview.chromium.org/10442098/diff/80068/net/dns/address_sorter_pos...
net/dns/address_sorter_posix_unittest.cc:327: AddMapping("4000::1", "4000::10");
 // global unicast
On 2012/08/10 05:56:01, szym wrote:
> On 2012/08/09 19:51:11, Matt Menke wrote:
> > Suggest you sort these in order of precedence like in the others.
> 
> Do you mean make |order| 4, 3, 2, 1, 0, -1?

No, I mean the order the AddMapping calls are in, so the deprecated one is last,
since that's how it's expected to be sorted.  That's the order you have the
AddMapping calls in most of the tests.  Actually, think all the others except
the deprecated one are in the correct order (With the order of the two globals
being ambiguous).

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

http://codereview.chromium.org/10442098/diff/80068/net/dns/address_sorter_win...
net/dns/address_sorter_win.cc:91: buffer_size_, buffer_.get(), buffer_size_,
On 2012/08/10 05:56:01, szym wrote:
> On 2012/08/09 19:51:11, Matt Menke wrote:
> > Are you sure it's safe to use the same input and output buffers?
> > 
> > Also, it's not clear if input buffer size should include the
SOCKADDR_STORAGEs
> > on the end of the list.
> 
> I am fairly certain that the answer is yes to both questions. I have no
> documentation confirming the first one so I'll change it to two buffers for
> safety. Regarding the second, AFAIK: In ioctl calls, all of the memory must be
> contained within the user process and all of it has to be allocated before the
> call, so there's no way for the ioctl call to complete if it cannot fit
> SOCKADDR_STORAGE inside the buffer.

I actually meant the input buffer size, not the output one - the output one, I
agree with you, but with the input one, it's not so clear.

[mmenke1, 2012-08-10 14:47:10]

http://codereview.chromium.org/10442098/diff/80068/net/base/host_resolver_imp...
File net/base/host_resolver_impl.cc (right):

http://codereview.chromium.org/10442098/diff/80068/net/base/host_resolver_imp...
net/base/host_resolver_impl.cc:1144: OnFailure(ERR_NAME_NOT_RESOLVED,
DnsResponse::DNS_NO_ADDRESSES);
On 2012/08/10 05:56:01, szym wrote:
> On 2012/08/09 19:51:11, Matt Menke wrote:
> > DnsResponse::DNS_NO_ADDRESSES is no longer actually being used by
DnsResponse.
> 
> > Seems a little weird to only use it here.
> > 
> > [completely optional] I'd suggest you add a comment to dns_response.h that
> it's
> > obsolete, and just use DnsResponse::DNS_PARSE_OK here for consistency.
> 
> I'd like to keep it for now for the sake of the AsyncDNS.ResultError
histogram.

Right, which is why I was thinking just a comment that it was obsolete in the
enum makes sense, without removing it - make it clear it's no longer actually
being used by the DnsResponse object, and no longer being recorded in histograms
(OnFailure doesn't record any histograms, just logs it to NetLog).

Of course, it's weird to mark it as obsolete but still use the code here for
just the NetLog, so think it makes sense to remove it from here in conjunction
with the above change.

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

http://codereview.chromium.org/10442098/diff/80068/net/base/host_resolver_imp...
File net/base/host_resolver_impl.cc (right):

http://codereview.chromium.org/10442098/diff/80068/net/base/host_resolver_imp...
net/base/host_resolver_impl.cc:1144: OnFailure(ERR_NAME_NOT_RESOLVED,
DnsResponse::DNS_NO_ADDRESSES);
On 2012/08/10 14:47:10, mmenke (incorrect) wrote:
> On 2012/08/10 05:56:01, szym wrote:
> > On 2012/08/09 19:51:11, Matt Menke wrote:
> > > DnsResponse::DNS_NO_ADDRESSES is no longer actually being used by
> DnsResponse.
> > 
> > > Seems a little weird to only use it here.
> > > 
> > > [completely optional] I'd suggest you add a comment to dns_response.h that
> > it's
> > > obsolete, and just use DnsResponse::DNS_PARSE_OK here for consistency.
> > 
> > I'd like to keep it for now for the sake of the AsyncDNS.ResultError
> histogram.
> 
> Right, which is why I was thinking just a comment that it was obsolete in the
> enum makes sense, without removing it - make it clear it's no longer actually
> being used by the DnsResponse object, and no longer being recorded in
histograms
> (OnFailure doesn't record any histograms, just logs it to NetLog).
> 
> Of course, it's weird to mark it as obsolete but still use the code here for
> just the NetLog, so think it makes sense to remove it from here in conjunction
> with the above change.

Ah, ResultError does not count DnsResponse::Result. I didn't catch that it is no
longer counted in the ParseToAddressList histogram. The reason why I return
DNS_PARSE_OK vs. DNS_PARSE_NO_ADDRESSES is to make the logic in line 1104
simpler, but this also removes it from the histogram.

I'll remove this value from the enum.

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

http://codereview.chromium.org/10442098/diff/80068/net/dns/address_sorter_win...
net/dns/address_sorter_win.cc:91: buffer_size_, buffer_.get(), buffer_size_,
On 2012/08/10 14:20:11, Matt Menke wrote:
> On 2012/08/10 05:56:01, szym wrote:
> > On 2012/08/09 19:51:11, Matt Menke wrote:
> > > Are you sure it's safe to use the same input and output buffers?
> > > 
> > > Also, it's not clear if input buffer size should include the
> SOCKADDR_STORAGEs
> > > on the end of the list.
> > 
> > I am fairly certain that the answer is yes to both questions. I have no
> > documentation confirming the first one so I'll change it to two buffers for
> > safety. Regarding the second, AFAIK: In ioctl calls, all of the memory must
be
> > contained within the user process and all of it has to be allocated before
the
> > call, so there's no way for the ioctl call to complete if it cannot fit
> > SOCKADDR_STORAGE inside the buffer.
> 
> I actually meant the input buffer size, not the output one - the output one, I
> agree with you, but with the input one, it's not so clear.

I don't think there's any risk in including it for the input. |iAddressCount|
stores the actual count. I'd be concerned that the OS needs to copy data from
the buffer into kernel, so it's safer to include it.

[mmenke, 2012-08-10 15:12:44]

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

http://codereview.chromium.org/10442098/diff/80068/net/dns/address_sorter_win...
net/dns/address_sorter_win.cc:91: buffer_size_, buffer_.get(), buffer_size_,
On 2012/08/10 15:04:39, szym wrote:
> On 2012/08/10 14:20:11, Matt Menke wrote:
> > On 2012/08/10 05:56:01, szym wrote:
> > > On 2012/08/09 19:51:11, Matt Menke wrote:
> > > > Are you sure it's safe to use the same input and output buffers?
> > > > 
> > > > Also, it's not clear if input buffer size should include the
> > SOCKADDR_STORAGEs
> > > > on the end of the list.
> > > 
> > > I am fairly certain that the answer is yes to both questions. I have no
> > > documentation confirming the first one so I'll change it to two buffers
for
> > > safety. Regarding the second, AFAIK: In ioctl calls, all of the memory
must
> be
> > > contained within the user process and all of it has to be allocated before
> the
> > > call, so there's no way for the ioctl call to complete if it cannot fit
> > > SOCKADDR_STORAGE inside the buffer.
> > 
> > I actually meant the input buffer size, not the output one - the output one,
I
> > agree with you, but with the input one, it's not so clear.
> 
> I don't think there's any risk in including it for the input. |iAddressCount|
> stores the actual count. I'd be concerned that the OS needs to copy data from
> the buffer into kernel, so it's safer to include it.

SGTM

[szym, 2012-08-11 22:04:20]

I addressed all the nits. 

One significant change is that in DnsTask, AddressSorter::Sort is no longer used
if the AddressList has only one item. This is to avoid unnecessary effort.
However, recall that AddressSorterPosix will weed out unusable destinations (the
OS implementation does not generally do that). I'm not too concerned about this,
but I'm okay with going back to the previous logic if necessary.

http://codereview.chromium.org/10442098/diff/80068/net/dns/address_sorter_pos...
File net/dns/address_sorter_posix_unittest.cc (right):

http://codereview.chromium.org/10442098/diff/80068/net/dns/address_sorter_pos...
net/dns/address_sorter_posix_unittest.cc:86: IPEndPoint local_endpoint_;
On 2012/08/09 19:51:11, Matt Menke wrote:
> nit:  Last two can be const.

They are set in Connect not in constructor.

http://codereview.chromium.org/10442098/diff/80068/net/dns/address_sorter_pos...
net/dns/address_sorter_posix_unittest.cc:285: AddMapping("fe81::1", "fe81::10");
     // matching link-local
On 2012/08/10 14:20:11, Matt Menke wrote:
> On 2012/08/10 05:56:01, szym wrote:
> > On 2012/08/09 19:51:11, Matt Menke wrote:
> > > By matching, you mean matching everything from previous rules, and the
> others
> > > match everything except scope?
> > 
> > I meant matching scope. Within both groups Rule 8 is the first
distinguishing
> > factor, but the first two go higher due to Rule 2.
> 
> Right, but for both sets, earlier rules must not distinguish the entries, too,
> for scope to matter.  The "ff3#:1", "4000:10" entries left me wondering about
> label and precedence.

All addresses in this test have the same precedence and label (40, 1).

[mmenke, 2012-08-13 20:11:50]

Only found one significant issue.  I'm about ready to sign off on this.

http://codereview.chromium.org/10442098/diff/85053/net/base/host_resolver_imp...
File net/base/host_resolver_impl.cc (right):

http://codereview.chromium.org/10442098/diff/85053/net/base/host_resolver_imp...
net/base/host_resolver_impl.cc:1138: needs_sort = (addr_list.size() > 1);
IMPORTANT:  This should not be in this loop, since we want to sort even when we
don't have any IPv4 addresses.

Could also avoid a sort when we only have IPv4 addresses, which will be the most
common case.

http://codereview.chromium.org/10442098/diff/85053/net/base/net_util.cc
File net/base/net_util.cc (right):

http://codereview.chromium.org/10442098/diff/85053/net/base/net_util.cc#newco...
net/base/net_util.cc:2255: 
nit:  Remove extra linbreak.

http://codereview.chromium.org/10442098/diff/85053/net/dns/address_sorter_pos...
File net/dns/address_sorter_posix.cc (right):

http://codereview.chromium.org/10442098/diff/85053/net/dns/address_sorter_pos...
net/dns/address_sorter_posix.cc:106: AddressScope GetScope(const PolicyTable&
table,
nit:  Suggest you call this "ipv4_scope_table" for consistency and clarity.

http://codereview.chromium.org/10442098/diff/85053/net/dns/address_sorter_pos...
File net/dns/address_sorter_posix_unittest.cc (right):

http://codereview.chromium.org/10442098/diff/85053/net/dns/address_sorter_pos...
net/dns/address_sorter_posix_unittest.cc:280: AddMapping("ff38::1", "4000::10");
     // org-local
nit:  Two spaces before comments.

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

http://codereview.chromium.org/10442098/diff/85053/net/dns/address_sorter_win...
net/dns/address_sorter_win.cc:126: const scoped_ptr_malloc<SOCKET_ADDRESS_LIST>
output_buffer_;
Think having input_buffer_ and output_buffer_ const is a little misleading.  As
it is, you could actually make Run() const, but it's clearly changing things.

http://codereview.chromium.org/10442098/diff/85053/net/dns/dns_response.h
File net/dns/dns_response.h (right):

http://codereview.chromium.org/10442098/diff/85053/net/dns/dns_response.h#new...
net/dns/dns_response.h:93: // Only add new values here.
To avoid messing up histograms, you should probably keep the old error code, and
just have a comment that it's obsolete.  At least that's the general way these
things are done, according to jar.

[szym, 2012-08-13 20:28:26]

All done, except for the IMPORTANT issue, which I clarified.

http://codereview.chromium.org/10442098/diff/85053/net/base/host_resolver_imp...
File net/base/host_resolver_impl.cc (right):

http://codereview.chromium.org/10442098/diff/85053/net/base/host_resolver_imp...
net/base/host_resolver_impl.cc:1138: needs_sort = (addr_list.size() > 1);
On 2012/08/13 20:11:50, Matt Menke wrote:
> IMPORTANT:  This should not be in this loop, since we want to sort even when
we
> don't have any IPv4 addresses.
> 
> Could also avoid a sort when we only have IPv4 addresses, which will be the
most
> common case.

This is already after the lists from the two transactions have been merged. If
after the merge, |addr_list| has only one address then there is really nothing
to sort, although AddresSorterPosix might still conclude that the lone address
is unreachable.

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

http://codereview.chromium.org/10442098/diff/85053/net/dns/address_sorter_win...
net/dns/address_sorter_win.cc:126: const scoped_ptr_malloc<SOCKET_ADDRESS_LIST>
output_buffer_;
On 2012/08/13 20:11:50, Matt Menke wrote:
> Think having input_buffer_ and output_buffer_ const is a little misleading. 
As
> it is, you could actually make Run() const, but it's clearly changing things.

I definitely don't want to make Run const. This is the trouble with const-ness.
The pointers are not affected, although the buffer content surely does.

[mmenke1, 2012-08-13 20:34:32]

http://codereview.chromium.org/10442098/diff/85053/net/base/host_resolver_imp...
File net/base/host_resolver_impl.cc (right):

http://codereview.chromium.org/10442098/diff/85053/net/base/host_resolver_imp...
net/base/host_resolver_impl.cc:1138: needs_sort = (addr_list.size() > 1);
On 2012/08/13 20:28:26, szym wrote:
> On 2012/08/13 20:11:50, Matt Menke wrote:
> > IMPORTANT:  This should not be in this loop, since we want to sort even when
> we
> > don't have any IPv4 addresses.
> > 
> > Could also avoid a sort when we only have IPv4 addresses, which will be the
> most
> > common case.
> 
> This is already after the lists from the two transactions have been merged. If
> after the merge, |addr_list| has only one address then there is really nothing
> to sort, although AddresSorterPosix might still conclude that the lone address
> is unreachable.

If first_addr_list_ is empty (No IPv4 addresses), but addr_list has 2+ IPv6
addresses, we're not sorting.

[szym, 2012-08-13 20:39:35]

Fixed.

http://codereview.chromium.org/10442098/diff/85053/net/base/host_resolver_imp...
File net/base/host_resolver_impl.cc (right):

http://codereview.chromium.org/10442098/diff/85053/net/base/host_resolver_imp...
net/base/host_resolver_impl.cc:1138: needs_sort = (addr_list.size() > 1);
On 2012/08/13 20:34:33, mmenke (incorrect) wrote:
> On 2012/08/13 20:28:26, szym wrote:
> > On 2012/08/13 20:11:50, Matt Menke wrote:
> > > IMPORTANT:  This should not be in this loop, since we want to sort even
when
> > we
> > > don't have any IPv4 addresses.
> > > 
> > > Could also avoid a sort when we only have IPv4 addresses, which will be
the
> > most
> > > common case.
> > 
> > This is already after the lists from the two transactions have been merged.
If
> > after the merge, |addr_list| has only one address then there is really
nothing
> > to sort, although AddresSorterPosix might still conclude that the lone
address
> > is unreachable.
> 
> If first_addr_list_ is empty (No IPv4 addresses), but addr_list has 2+ IPv6
> addresses, we're not sorting.

Oh sorry, I now see the issue is not the |addr_list.size() > 1| but that
need_sort is set only if |!first_addr_list_.empty()|

[mmenke1, 2012-08-13 20:45:10]

http://codereview.chromium.org/10442098/diff/80075/net/base/host_resolver_imp...
File net/base/host_resolver_impl.cc (right):

http://codereview.chromium.org/10442098/diff/80075/net/base/host_resolver_imp...
net/base/host_resolver_impl.cc:1139: needs_sort = (addr_list.size() > 1);
So what do you think of not sorting when we don't have any IPv6 addresses?

We'd need something like:

needs_sort = (addr_list.size() > 1 && addr_list.size() !=
first_addr_list_.size());

Believe you said that the sorting on windows could take up to 10 milliseconds,
throw in the latency of passing messages between threads, and seems like it may
be worth the slightly added complexity.

[szym, 2012-08-13 20:56:23]

http://codereview.chromium.org/10442098/diff/80075/net/base/host_resolver_imp...
File net/base/host_resolver_impl.cc (right):

http://codereview.chromium.org/10442098/diff/80075/net/base/host_resolver_imp...
net/base/host_resolver_impl.cc:1139: needs_sort = (addr_list.size() > 1);
On 2012/08/13 20:45:10, mmenke (incorrect) wrote:
> So what do you think of not sorting when we don't have any IPv6 addresses?
> 
> We'd need something like:
> 
> needs_sort = (addr_list.size() > 1 && addr_list.size() !=
> first_addr_list_.size());
> 
> Believe you said that the sorting on windows could take up to 10 milliseconds,
> throw in the latency of passing messages between threads, and seems like it
may
> be worth the slightly added complexity.

I think it's worth it. I added an explicit bool for readability.

[mmenke1, 2012-08-13 21:16:51]

http://codereview.chromium.org/10442098/diff/80075/net/base/host_resolver_imp...
File net/base/host_resolver_impl.cc (right):

http://codereview.chromium.org/10442098/diff/80075/net/base/host_resolver_imp...
net/base/host_resolver_impl.cc:1139: needs_sort = (addr_list.size() > 1);
On 2012/08/13 20:56:23, szym wrote:
> On 2012/08/13 20:45:10, mmenke (incorrect) wrote:
> > So what do you think of not sorting when we don't have any IPv6 addresses?
> > 
> > We'd need something like:
> > 
> > needs_sort = (addr_list.size() > 1 && addr_list.size() !=
> > first_addr_list_.size());
> > 
> > Believe you said that the sorting on windows could take up to 10
milliseconds,
> > throw in the latency of passing messages between threads, and seems like it
> may
> > be worth the slightly added complexity.
> 
> I think it's worth it. I added an explicit bool for readability.

Think the bool is much clearer.  I'll give this a final skim, and probably sign
off on it tomorrow.

[mmenke, 2012-08-14 15:56:12]

A couple final nits, and I'm ready to sign off on this.

http://codereview.chromium.org/10442098/diff/94032/net/base/host_resolver_imp...
File net/base/host_resolver_impl.cc (right):

http://codereview.chromium.org/10442098/diff/94032/net/base/host_resolver_imp...
net/base/host_resolver_impl.cc:1135: ttl = std::min(ttl, first_ttl_);
Random comment:  Should we take into account the time between when we got the
first ttl and now?  I don't really see it mattering, but thought I'd mention it
anyways.

http://codereview.chromium.org/10442098/diff/94032/net/base/host_resolver_imp...
net/base/host_resolver_impl.cc:1209: base::TimeDelta first_ttl_;
While you're here, this should also have a DISALLOW_COPY_AND_ASSIGN

http://codereview.chromium.org/10442098/diff/94032/net/base/host_resolver_imp...
File net/base/host_resolver_impl_unittest.cc (right):

http://codereview.chromium.org/10442098/diff/94032/net/base/host_resolver_imp...
net/base/host_resolver_impl_unittest.cc:1265: void AddDnsRule(const std::string&
prefix,
I suggest a comment that CreateResolver() must be called after a rule is added
for it to be applied.  While none of the tests do this, this requirement isn't
immediately clear.

Alternatively, you could just make AddDnsRule private.

http://codereview.chromium.org/10442098/diff/94032/net/dns/address_sorter.h
File net/dns/address_sorter.h (right):

http://codereview.chromium.org/10442098/diff/94032/net/dns/address_sorter.h#n...
net/dns/address_sorter.h:35: static scoped_ptr<AddressSorter>
CreateAddressSorter();
private:
  DISALLOW_COPY_AND_ASSIGN

http://codereview.chromium.org/10442098/diff/94032/net/dns/address_sorter_pos...
File net/dns/address_sorter_posix.cc (right):

http://codereview.chromium.org/10442098/diff/94032/net/dns/address_sorter_pos...
net/dns/address_sorter_posix.cc:20: #include <vector>
map and vector are not needed, since they're included in the header.

http://codereview.chromium.org/10442098/diff/94032/net/dns/address_sorter_pos...
net/dns/address_sorter_posix.cc:28: #include
"net/base/network_change_notifier.h"
The above 4 includes are not needed, since they're all included in the header.

Also, other than "network_change_notifier.h", I'm not sure any of them are
actually needed in the header, since you can forward declare AddressList.

http://codereview.chromium.org/10442098/diff/94032/net/dns/address_sorter_pos...
net/dns/address_sorter_posix.cc:365: source_map_.clear();
Is there a reason we clear the map on OSX/BSD if we can't create a socket or if
getifaddrs fails, but we don't clear the map on Linux if there's no
AddressTracker?

http://codereview.chromium.org/10442098/diff/94032/net/dns/address_sorter_pos...
File net/dns/address_sorter_posix.h (right):

http://codereview.chromium.org/10442098/diff/94032/net/dns/address_sorter_pos...
net/dns/address_sorter_posix.h:54: typedef std::map<IPAddressNumber,
SourceAddressInfo> SourceAddressMap;
Rather than making all of the above have global net scope, they should be
internal to AddressSorterPosix.  If we want any of them to be in net/, they
should be declared in another file.

http://codereview.chromium.org/10442098/diff/94032/net/dns/address_sorter_pos...
net/dns/address_sorter_posix.h:66: virtual void Sort(const AddressList& list,
nit:  "// AddressSorter:"

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

http://codereview.chromium.org/10442098/diff/94032/net/dns/address_sorter_win...
net/dns/address_sorter_win.cc:32: virtual void Sort(const AddressList& list,
nit:  // AddressSorter:

http://codereview.chromium.org/10442098/diff/94032/net/dns/address_sorter_win...
net/dns/address_sorter_win.cc:159: virtual void Sort(const AddressList& list,
nit:  // AddressSorter:

[szym, 2012-08-14 20:02:26]

All done.

http://codereview.chromium.org/10442098/diff/94032/net/base/host_resolver_imp...
File net/base/host_resolver_impl.cc (right):

http://codereview.chromium.org/10442098/diff/94032/net/base/host_resolver_imp...
net/base/host_resolver_impl.cc:1135: ttl = std::min(ttl, first_ttl_);
On 2012/08/14 15:56:12, Matt Menke wrote:
> Random comment:  Should we take into account the time between when we got the
> first ttl and now?  I don't really see it mattering, but thought I'd mention
it
> anyways.

I think the complexity isn't worth it. We are talking about precision loss on
the order of several network latencies which should be the expected accuracy of
TTL anyway. glibc doesn't bother either. Once we start issuing the queries in
parallel, the precision loss will be even lower.

http://codereview.chromium.org/10442098/diff/94032/net/dns/address_sorter.h
File net/dns/address_sorter.h (right):

http://codereview.chromium.org/10442098/diff/94032/net/dns/address_sorter.h#n...
net/dns/address_sorter.h:35: static scoped_ptr<AddressSorter>
CreateAddressSorter();
On 2012/08/14 15:56:12, Matt Menke wrote:
> private:
>   DISALLOW_COPY_AND_ASSIGN

Added, although this also requires protected: AddressSorter() {}

[mmenke1, 2012-08-14 20:07:11]

LGTM!

[szym, 2012-08-14 20:08:22]

On 2012/08/14 20:07:11, mmenke (incorrect) wrote:
> LGTM!

Many thanks for the meticulous review!

[commit-bot: I haz the power, 2012-08-14 20:16:00]

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

[commit-bot: I haz the power, 2012-08-14 22:36:37]

Change committed as 151586