HostResolver supports optional CNAME lookups.

HostResolver now adds AI_CANONNAME to the hint flags if a requester needs the information.

Requests which want the canonical name should be treated differently from requests that do not for the same host in both the HostCache as well as in the HostResolver when combining multiple outstanding requests into a job. 

The motivation for this is that Kerberos SPN's for a web server are typically generated using the canonical name of the server rather than a DNS alias (both Firefox and IE have this behavior).

BUG=29862
TEST=net_unittests

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

[cbentzel, 2010-04-02 17:36:40]

I still need to add some more unit-tests and fix some formatting issues - but I
wanted to send this to you to see if this is roughly what you expected.

[wtc, 2010-04-02 22:01:26]

cbentzel: I reviewed the entire changelist.  Please wait for
eroman's review because this is his code.

eroman: please be the primary reviewer of this CL.  Note my
comments marked with "IMPORTANT" and my comments on
address_family.h below.  You can ignore the other comments.

http://codereview.chromium.org/1566012/diff/15/8001
File net/base/address_family.h (right):

http://codereview.chromium.org/1566012/diff/15/8001#newcode20
net/base/address_family.h:20: enum AddressFlags {
Nit: I would name this enum type HostResolverFlags.

Nit: this enum type doesn't need to be named, unlike the
AddressFamily enum above.

http://codereview.chromium.org/1566012/diff/15/8001#newcode21
net/base/address_family.h:21: ADDRESS_FLAGS_NONE = 0x0,
Nit: remove this enum constant.  I prefer just passing 0.
Otherwise, I need to check this header to know if
ADDRESS_FLAGS_NONE is 0.

http://codereview.chromium.org/1566012/diff/15/8002
File net/base/address_list.cc (right):

http://codereview.chromium.org/1566012/diff/15/8002#newcode139
net/base/address_list.cc:139: std::string ret_name(data_->head->ai_canonname);
Replace these two lines with something like
  *canonical_name = data_->head->ai_canonname;

It's not necessary to create a local object and swap with it.

http://codereview.chromium.org/1566012/diff/15/8003
File net/base/address_list.h (right):

http://codereview.chromium.org/1566012/diff/15/8003#newcode51
net/base/address_list.h:51: // Gets the canonical name, if it exists.
If the canonical name doesn't exist, does GetCanonicalName
return false?  This should be documented.

Strictly speaking, GetCanonicalName is not needed because
callers are supposed to look in the struct addrinfo list
for the canonical name.

http://codereview.chromium.org/1566012/diff/15/8004
File net/base/address_list_unittest.cc (right):

http://codereview.chromium.org/1566012/diff/15/8004#newcode22
net/base/address_list_unittest.cc:22: net::AddressFlags address_flags) {
Nit: I'd list address_flags immediately after hostname.
Or perhaps just list addrlist last (because it's the only
output argument).

http://codereview.chromium.org/1566012/diff/15/8004#newcode126
net/base/address_list_unittest.cc:126: // TODO(cbentzel): Any way to avoid
looking up a public address?
We need to do something about this test, because the
canonical name of www.google.com may change in the future,
or even depends on the geographic location.

http://codereview.chromium.org/1566012/diff/15/8005
File net/base/host_cache.h (right):

http://codereview.chromium.org/1566012/diff/15/8005#newcode20
net/base/host_cache.h:20: class HostCache {
IMPORTANT: the changes to this file need to be reviewed
carefully.

A cache entry with the canonical name should be able to
satisfy a lookup that doesn't have the CANONNAME flag.

Similarly, when overwriting an existing cache entry, if
the new entry doesn't have a canonical name, we may want
to keep the canonical name in the old entry.  (This one
is debatable because the old canonical name may be stale.)

http://codereview.chromium.org/1566012/diff/15/8006
File net/base/host_cache_unittest.cc (right):

http://codereview.chromium.org/1566012/diff/15/8006#newcode342
net/base/host_cache_unittest.cc:342: // entries (because the address families
differ).
Typo: address families differ => address flags differ

http://codereview.chromium.org/1566012/diff/15/8007
File net/base/host_resolver.h (right):

http://codereview.chromium.org/1566012/diff/15/8007#newcode90
net/base/host_resolver.h:90: AddressFlags address_flags_;
This member should have the 'int' type, because this member
is a bitwise-OR of the bit flags in general.  This is why I
said the enum for the address flags should not be named.

http://codereview.chromium.org/1566012/diff/15/8011
File net/base/host_resolver_proc.h (right):

http://codereview.chromium.org/1566012/diff/15/8011#newcode33
net/base/host_resolver_proc.h:33: AddressFlags address_flags,
Nit: I would name this parameter just |flags|.

http://codereview.chromium.org/1566012/diff/15/8012
File net/base/mock_host_resolver.cc (right):

http://codereview.chromium.org/1566012/diff/15/8012#newcode112
net/base/mock_host_resolver.cc:112: AddressFlags address_flags;
IMPORTANT: the changes to this file should be reviewed
carefully.  I am not sure if the Rule struct should have
the address_flags member.

[eroman, 2010-04-02 22:17:52]

Nice change!

http://codereview.chromium.org/1566012/diff/15/8001
File net/base/address_family.h (right):

http://codereview.chromium.org/1566012/diff/15/8001#newcode22
net/base/address_family.h:22: ADDRESS_FLAGS_CANONNAME = 0x1, // AI_CANONNAME
nit: I suggest following the convention of writing bitfield values like:

1 << 0,
1 << 1,
1 << 2,

etc..

http://codereview.chromium.org/1566012/diff/15/8002
File net/base/address_list.cc (right):

http://codereview.chromium.org/1566012/diff/15/8002#newcode140
net/base/address_list.cc:140: canonical_name->swap(ret_name);
nit: my preference is canonical_name->assign(...), for some reason i find swap
confusing.

http://codereview.chromium.org/1566012/diff/15/8004
File net/base/address_list_unittest.cc (right):

http://codereview.chromium.org/1566012/diff/15/8004#newcode129
net/base/address_list_unittest.cc:129: CreateAddressList("www.google.com",
&addrlist1, 80,
Would it help to craft the test in terms of GetHostName() ? and then search for
substrings or something.

http://codereview.chromium.org/1566012/diff/15/8005
File net/base/host_cache.h (right):

http://codereview.chromium.org/1566012/diff/15/8005#newcode55
net/base/host_cache.h:55: if (hostname != other.hostname)
Minor: I suggest reversing the ordering here, such that you first do:

if (address_flags != other.address_flags)
  return address_flags < other.address_flags;
return hostname < other.hostname;

The reason I suggest this, is we avoid doing the string != comparison which is
costlier than the int comparison.

http://codereview.chromium.org/1566012/diff/15/8005#newcode62
net/base/host_cache.h:62: AddressFlags address_flags;
Seems like you would want an |int| here if we plan for this to be a bitfield.

http://codereview.chromium.org/1566012/diff/15/8006
File net/base/host_cache_unittest.cc (right):

http://codereview.chromium.org/1566012/diff/15/8006#newcode342
net/base/host_cache_unittest.cc:342: // entries (because the address families
differ).
On 2010/04/02 22:01:28, wtc wrote:
> Typo: address families differ => address flags differ

:)

I imagine this was a note for you ?

http://codereview.chromium.org/1566012/diff/15/8009
File net/base/host_resolver_impl_unittest.cc (right):

http://codereview.chromium.org/1566012/diff/15/8009#newcode82
net/base/host_resolver_impl_unittest.cc:82: AddressFlags address_flags;
Same comment as earlier, these should probably be int throughout.

http://codereview.chromium.org/1566012/diff/15/8009#newcode154
net/base/host_resolver_impl_unittest.cc:154: ADDRESS_FLAGS_NONE,
I suggest adding a comment that |address_flags| is not used here intentionally.

[cbentzel, 2010-04-05 14:57:03]

The latest patchset changes type names and does numerous cosmetic fixes.

http://codereview.chromium.org/1566012/diff/15/8001
File net/base/address_family.h (right):

http://codereview.chromium.org/1566012/diff/15/8001#newcode20
net/base/address_family.h:20: enum AddressFlags {
On 2010/04/02 22:01:28, wtc wrote:
> Nit: I would name this enum type HostResolverFlags.

I made this a nameless enum, and made HostResolverFlags a typedef for int.

http://codereview.chromium.org/1566012/diff/15/8001#newcode21
net/base/address_family.h:21: ADDRESS_FLAGS_NONE = 0x0,
On 2010/04/02 22:01:28, wtc wrote:
> Nit: remove this enum constant.  I prefer just passing 0.
> Otherwise, I need to check this header to know if
> ADDRESS_FLAGS_NONE is 0.

Done - although I prefer the ADDRESS_FLAGS_NONE so when I look at the call sites
I know what is going on (compared to the fourth argument being a 0).

http://codereview.chromium.org/1566012/diff/15/8001#newcode22
net/base/address_family.h:22: ADDRESS_FLAGS_CANONNAME = 0x1, // AI_CANONNAME
On 2010/04/02 22:17:52, eroman wrote:
> nit: I suggest following the convention of writing bitfield values like:
> 
> 1 << 0,
> 1 << 1,
> 1 << 2,
> 
> etc..

Done.

http://codereview.chromium.org/1566012/diff/15/8002
File net/base/address_list.cc (right):

http://codereview.chromium.org/1566012/diff/15/8002#newcode139
net/base/address_list.cc:139: std::string ret_name(data_->head->ai_canonname);
On 2010/04/02 22:01:28, wtc wrote:
> Replace these two lines with something like
>   *canonical_name = data_->head->ai_canonname;
> 
> It's not necessary to create a local object and swap with it.

Moved to assign member.

http://codereview.chromium.org/1566012/diff/15/8002#newcode140
net/base/address_list.cc:140: canonical_name->swap(ret_name);
On 2010/04/02 22:17:52, eroman wrote:
> nit: my preference is canonical_name->assign(...), for some reason i find swap
> confusing.

Done.

http://codereview.chromium.org/1566012/diff/15/8003
File net/base/address_list.h (right):

http://codereview.chromium.org/1566012/diff/15/8003#newcode51
net/base/address_list.h:51: // Gets the canonical name, if it exists.
On 2010/04/02 22:01:28, wtc wrote:
> If the canonical name doesn't exist, does GetCanonicalName
> return false?  This should be documented.
> 
> Strictly speaking, GetCanonicalName is not needed because
> callers are supposed to look in the struct addrinfo list
> for the canonical name.

I added more documentation.

The method is not strictly needed, but I'm worried that callers might assume
that the canonical name is present. Also, I only introduced this after I had
duplicate logic in three places for finding the canonical name.

http://codereview.chromium.org/1566012/diff/15/8004
File net/base/address_list_unittest.cc (right):

http://codereview.chromium.org/1566012/diff/15/8004#newcode22
net/base/address_list_unittest.cc:22: net::AddressFlags address_flags) {
On 2010/04/02 22:01:28, wtc wrote:
> Nit: I'd list address_flags immediately after hostname.
> Or perhaps just list addrlist last (because it's the only
> output argument).

Done.

http://codereview.chromium.org/1566012/diff/15/8004#newcode126
net/base/address_list_unittest.cc:126: // TODO(cbentzel): Any way to avoid
looking up a public address?
On 2010/04/02 22:01:28, wtc wrote:
> We need to do something about this test, because the
> canonical name of http://www.google.com may change in the future,
> or even depends on the geographic location.

I'm open to suggestions for an address which always has a CNAME.

http://codereview.chromium.org/1566012/diff/15/8006
File net/base/host_cache_unittest.cc (right):

http://codereview.chromium.org/1566012/diff/15/8006#newcode342
net/base/host_cache_unittest.cc:342: // entries (because the address families
differ).
On 2010/04/02 22:17:52, eroman wrote:
> On 2010/04/02 22:01:28, wtc wrote:
> > Typo: address families differ => address flags differ
> 
> :)
> 
> I imagine this was a note for you ?
> 

Fixed the typo.

http://codereview.chromium.org/1566012/diff/15/8007
File net/base/host_resolver.h (right):

http://codereview.chromium.org/1566012/diff/15/8007#newcode90
net/base/host_resolver.h:90: AddressFlags address_flags_;
On 2010/04/02 22:01:28, wtc wrote:
> This member should have the 'int' type, because this member
> is a bitwise-OR of the bit flags in general.  This is why I
> said the enum for the address flags should not be named.

HostResolverFlags is a now an int typedef.

http://codereview.chromium.org/1566012/diff/15/8009
File net/base/host_resolver_impl_unittest.cc (right):

http://codereview.chromium.org/1566012/diff/15/8009#newcode82
net/base/host_resolver_impl_unittest.cc:82: AddressFlags address_flags;
On 2010/04/02 22:17:52, eroman wrote:
> Same comment as earlier, these should probably be int throughout.

Done.

[cbentzel, 2010-04-05 15:07:30]

http://codereview.chromium.org/1566012/diff/15/8005
File net/base/host_cache.h (right):

http://codereview.chromium.org/1566012/diff/15/8005#newcode20
net/base/host_cache.h:20: class HostCache {
On 2010/04/02 22:01:28, wtc wrote:
> IMPORTANT: the changes to this file need to be reviewed
> carefully.
> 
> A cache entry with the canonical name should be able to
> satisfy a lookup that doesn't have the CANONNAME flag.
> 
> Similarly, when overwriting an existing cache entry, if
> the new entry doesn't have a canonical name, we may want
> to keep the canonical name in the old entry.  (This one
> is debatable because the old canonical name may be stale.)

I'll focus on the first request (letting CANONNAME cached entries satisfy
non-CANONNAME requests).

Ideally this will be done for both consolidating outgoing requests into a Job,
as well as for handling the cache entries.

To do this, I can remove the flags from the Key structure. When we see if there
is a cache hit, the entry can contain the flags used for it and we can test that


test_flags & entry->flags == test_flags

For the job, we can simply or in the flags for a particular request for a job.

[cbentzel, 2010-04-05 19:27:54]

I ended up adopting the approach described in the previous comment (moving the
flag out of keys and into the entry).

At this point I've added unit tests for the HostCache and am working on adding a
unit test for the HostResolverImpl to make sure multiple outstanding requests
are combined into the same Job even when the flags differ (and that the flags
for the Job are the bitwise-or of the requests contained).

[wtc, 2010-04-06 01:26:51]

cbentzel: I haven't reviewed the latest Patch Set 7 yet.
I will do that tomorrow.  I have one minor comment and one
important comment below.

http://codereview.chromium.org/1566012/diff/15/8001
File net/base/address_family.h (right):

http://codereview.chromium.org/1566012/diff/15/8001#newcode22
net/base/address_family.h:22: ADDRESS_FLAGS_CANONNAME = 0x1, // AI_CANONNAME
On 2010/04/02 22:17:52, eroman wrote:
> nit: I suggest following the convention of writing bitfield values like:
> 
> 1 << 0,
> 1 << 1,
> 1 << 2,
> 
> etc..

I suggest that we do both:

Use the 1 << 1 form in the actual definition, and use
0x1 in a comment.

The value in the comment helps during debugging because
when you print a value in a debugger, the debugger won't
show it in the 1 << k format, so I always need to count
the bit position.

http://codereview.chromium.org/1566012/diff/15/8005
File net/base/host_cache.h (right):

http://codereview.chromium.org/1566012/diff/15/8005#newcode20
net/base/host_cache.h:20: class HostCache {
On 2010/04/02 22:01:28, wtc wrote:
> IMPORTANT: the changes to this file need to be reviewed
> carefully.
> 
> A cache entry with the canonical name should be able to
> satisfy a lookup that doesn't have the CANONNAME flag.
> 
> Similarly, when overwriting an existing cache entry, if
> the new entry doesn't have a canonical name, we may want
> to keep the canonical name in the old entry.  (This one
> is debatable because the old canonical name may be stale.)

Chris,

I talked to eroman about these two suggestions of mine.

He convinced me that these suggestions could potentially
complicate the code, but would only benefit some corner
cases.

So you can ignore these two suggestions.  Let's check
in something that we can easily verify to be correct.
Please add a comment to note that we intentionally
create multiple cache entries (that differ in flags)
for the same host for simplicity.

Is it too late for you to go back to the original simple
approach?

[cbentzel, 2010-04-06 13:58:20]

> Is it too late for you to go back to the original simple
> approach?

I rolled this back to the simpler approach (thank goodness for git), and fixed
comments/formatting/unit tests.

This is ready for review.

[cbentzel, 2010-04-06 14:36:40]

Hold off on review - I need to fix the build breaks reported by the trybots.

[cbentzel, 2010-04-06 18:52:37]

Ready for review.

[eroman, 2010-04-06 20:27:39]

LGTM.

Please also update:

HostResolverCacheSubSection::OutputBody() in
[chrome/browser/net/view_net_internals_job_factory.cc], to add a column for the
canonical name.

http://codereview.chromium.org/1566012/diff/23001/24001
File net/base/address_family.h (right):

http://codereview.chromium.org/1566012/diff/23001/24001#newcode21
net/base/address_family.h:21: HOST_RESOLVER_FLAGS_CANONNAME = 1 << 0, //
AI_CANONNAME
nit: two spaces before //

also, judging byt the name of this file, this would better live in either
host_resolver.h or some sort of host_resolver_flags.h

http://codereview.chromium.org/1566012/diff/36002/30005#newcode18
net/base/address_family.h:18: // HostResoverFlags is a bitflag enum wrapper
around the addrinfo.ai_flags
Doesn't seem like this belongs in "address_family.h".

Can you either rename "address_family.h" to something more general, or add a new
file, say "host_resolver_flags.h" ?

Or could maybe put this into host_resolver.h, but that might create a cyclical
dependency between hostresolver <--> cache, dunno.

http://codereview.chromium.org/1566012/diff/36002/30008
File net/base/address_list_unittest.cc (right):

http://codereview.chromium.org/1566012/diff/36002/30008#newcode131
net/base/address_list_unittest.cc:131: TEST(AddressListTest, Canonical) {
I suggest also testing the case where GetCanonicalName() returns false (real
simple -- just EXPECT_FALSE(GetCanonicalName) on an address list with no
canonical name).

[cbentzel, 2010-04-06 20:32:31]

Thanks. I'll upload a patchset with the suggested changes.

I was lazy with the host_resolver_flags.h change because I wanted to
minimize the amount of GYP changes and other files including it. Now that
this is stable I'll make that change.

On Tue, Apr 6, 2010 at 4:27 PM, <eroman@chromium.org> wrote:

> LGTM.
>
> Please also update:
>
> HostResolverCacheSubSection::OutputBody() in
> [chrome/browser/net/view_net_internals_job_factory.cc], to add a column for
> the
> canonical name.
>
>
> http://codereview.chromium.org/1566012/diff/23001/24001
>
> File net/base/address_family.h (right):
>
> http://codereview.chromium.org/1566012/diff/23001/24001#newcode21
> net/base/address_family.h:21: HOST_RESOLVER_FLAGS_CANONNAME = 1 << 0, //
> AI_CANONNAME
> nit: two spaces before //
>
> also, judging byt the name of this file, this would better live in
> either host_resolver.h or some sort of host_resolver_flags.h
>
> http://codereview.chromium.org/1566012/diff/36002/30005#newcode18
> net/base/address_family.h:18: // HostResoverFlags is a bitflag enum
> wrapper around the addrinfo.ai_flags
> Doesn't seem like this belongs in "address_family.h".
>
> Can you either rename "address_family.h" to something more general, or
> add a new file, say "host_resolver_flags.h" ?
>
> Or could maybe put this into host_resolver.h, but that might create a
> cyclical dependency between hostresolver <--> cache, dunno.
>
> http://codereview.chromium.org/1566012/diff/36002/30008
> File net/base/address_list_unittest.cc (right):
>
> http://codereview.chromium.org/1566012/diff/36002/30008#newcode131
> net/base/address_list_unittest.cc:131: TEST(AddressListTest, Canonical)
> {
> I suggest also testing the case where GetCanonicalName() returns false
> (real simple -- just EXPECT_FALSE(GetCanonicalName) on an address list
> with no canonical name).
>
>
> http://codereview.chromium.org/1566012
>

[wtc, 2010-04-06 20:33:56]

cbentzel: I may not have time to review this CL today.  Please
commit this after getting eroman's approval.

http://codereview.chromium.org/1566012/diff/36002/30003
File chrome/browser/intranet_redirect_detector.h (right):

http://codereview.chromium.org/1566012/diff/36002/30003#newcode103
chrome/browser/intranet_redirect_detector.h:103: net::HostResolverFlags
host_resolver_flags,
Nit: consider renaming the parameter |flags| because the
type HostResolverFlags already describes what it is.

[cbentzel, 2010-04-07 15:00:09]

net_unittests ended up crashing/timing out on the main buildbot twice with this
CL, so I reverted. Curiously, the trybots never complained o win and I think
they are running XP as well. Trying to figure it out but I just have a vista
machine.