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

(note: I had to revert http://codereview.chromium.org/1566012/show because net_unittests were crashing/hanging on the main buildbot, even though they weren't on the trybots. Trying to figure out why). 
BUG=29862
TEST=net_unittests

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

[cbentzel, 2010-04-07 17:03:17]

If either of you run XP, could you try patching this in and running
net_unittests? 

The buildbot was failing and apparently the majority of the win trybot slaves
are running Vista. 

I'm trying to find an XP VMWare image to use to test this myself.

[cbentzel, 2010-04-07 18:36:48]

OK, I managed to test on XP.

It turns out that inet_pton was only introduced in Vista. Unfortunately for the
buildbots, running the old net_unittests.exe simply brings up a dialog box with
a complaint that inet_pton can't be found in WS2_32.dll, so there's really
nothing to report on stdout, and no great way to diagnose.

I'll run on all trybots again (including linux_valgrind).

[wtc, 2010-04-07 19:16:28]

LGTM.  I have some nits below.

After seeing the actual code, I now agree
HOST_RESOLVER_FLAGS_NONE would be more readable than 0.
If eroman and willchan also agree, we can start this new
convention in our API.  For now, just use 0.

http://codereview.chromium.org/1593015/diff/1/2
File chrome/browser/intranet_redirect_detector.cc (right):

http://codereview.chromium.org/1593015/diff/1/2#newcode161
chrome/browser/intranet_redirect_detector.cc:161: net::HostResolverFlags flags,
Nit: please change this back to host_resolver_flags for
consistency.  Sorry to make you go back and forth.

http://codereview.chromium.org/1593015/diff/1/4
File chrome/browser/net/view_net_internals_job_factory.cc (right):

http://codereview.chromium.org/1593015/diff/1/4#newcode397
chrome/browser/net/view_net_internals_job_factory.cc:397: if
(entry->addrlist.GetCanonicalName(&canonical_name)) {
Nit: don't need the curly braces.

http://codereview.chromium.org/1593015/diff/1/6
File net/base/address_family.h (right):

http://codereview.chromium.org/1593015/diff/1/6#newcode21
net/base/address_family.h:21: HOST_RESOLVER_FLAGS_CANONNAME = 1 << 0, //
AI_CANONNAME, 0x1
Nit: omit "FLAGS" in the enum constant name.

Nit: Put 0x1 first, before AI_CANONNAME, otherwise it seems to
imply that AI_CANONNAME is 0x1.

http://codereview.chromium.org/1593015/diff/1/9
File net/base/address_list_unittest.cc (right):

http://codereview.chromium.org/1593015/diff/1/9#newcode129
net/base/address_list_unittest.cc:129: static const char* kCanonicalHost =
"canonical.bar.com";
Nit: rename this constant kCanonicalName or kCanonicalHostname.

http://codereview.chromium.org/1593015/diff/1/9#newcode174
net/base/address_list_unittest.cc:174: EXPECT_EQ("blah", canon_name3);
In general I think it's not necessary to guarantee that a
function does not modify its output arguments on failure.
Sometimes it's not easy to do that.

http://codereview.chromium.org/1593015/diff/1/10
File net/base/host_cache.h (right):

http://codereview.chromium.org/1593015/diff/1/10#newcode46
net/base/host_cache.h:46: bool operator==(const Key& other) const {
Add a comment to document the non-obvious ordering in which
you compare the members (do the expensive string comparison
last) in operator== and operator<.

http://codereview.chromium.org/1593015/diff/1/12
File net/base/host_resolver.h (right):

http://codereview.chromium.org/1593015/diff/1/12#newcode91
net/base/host_resolver.h:91: // Flags for the address.
Nit: address => "host resolver" or "host resolution"

http://codereview.chromium.org/1593015/diff/1/14
File net/base/host_resolver_impl_unittest.cc (right):

http://codereview.chromium.org/1593015/diff/1/14#newcode76
net/base/host_resolver_impl_unittest.cc:76: : hostname(hostname),
Nit: it is fine to list multiple member initializers on one
line.  (I checked with Mark Mentovai, who maintains our C++
style guide.)

http://codereview.chromium.org/1593015/diff/1/20
File net/socket/socket_test_util.cc (right):

http://codereview.chromium.org/1593015/diff/1/20#newcode55
net/socket/socket_test_util.cc:55: 0,
Nit: put 0 and address on the same line.

[wtc, 2010-04-07 19:21:56]

Patch Set 2 looks good to me.

http://codereview.chromium.org/1593015/diff/3001/4008
File net/base/address_list_unittest.cc (right):

http://codereview.chromium.org/1593015/diff/3001/4008#newcode130
net/base/address_list_unittest.cc:130: memset(&address, 0x0, sizeof(address));
Please document that the contents of |address| don't matter
to this unit test.  Or just initialze |address| to some
meaningful values.  You can use eroman's
AddressList::CreateIPv6Address method.

[eroman, 2010-04-07 19:59:20]

LGTM.

(I only reviewed the modified unit-test since I assume the rest is unchanged
from what you committed earlier).

[cbentzel, 2010-04-07 20:09:31]

wtc: Addressed nits. Sorry for the "done"'s. Rietveld doesn't seem to offer a
way for me to mark off raised issues as resolved without also including the
"done" comments in the email.

eroman: Your assumption is correct, with the exception of the nit-fixing I just
did.

http://codereview.chromium.org/1593015/diff/1/2
File chrome/browser/intranet_redirect_detector.cc (right):

http://codereview.chromium.org/1593015/diff/1/2#newcode161
chrome/browser/intranet_redirect_detector.cc:161: net::HostResolverFlags flags,
On 2010/04/07 19:16:28, wtc wrote:
> Nit: please change this back to host_resolver_flags for
> consistency.  Sorry to make you go back and forth.

Done.

http://codereview.chromium.org/1593015/diff/1/4
File chrome/browser/net/view_net_internals_job_factory.cc (right):

http://codereview.chromium.org/1593015/diff/1/4#newcode397
chrome/browser/net/view_net_internals_job_factory.cc:397: if
(entry->addrlist.GetCanonicalName(&canonical_name)) {
On 2010/04/07 19:16:28, wtc wrote:
> Nit: don't need the curly braces.

Done.

http://codereview.chromium.org/1593015/diff/1/6
File net/base/address_family.h (right):

http://codereview.chromium.org/1593015/diff/1/6#newcode21
net/base/address_family.h:21: HOST_RESOLVER_FLAGS_CANONNAME = 1 << 0, //
AI_CANONNAME, 0x1
On 2010/04/07 19:16:28, wtc wrote:
> Nit: omit "FLAGS" in the enum constant name.
> 
> Nit: Put 0x1 first, before AI_CANONNAME, otherwise it seems to
> imply that AI_CANONNAME is 0x1.

Done. Not sure I fully like the removal of FLAGS, but I understand it.

http://codereview.chromium.org/1593015/diff/1/9
File net/base/address_list_unittest.cc (right):

http://codereview.chromium.org/1593015/diff/1/9#newcode129
net/base/address_list_unittest.cc:129: static const char* kCanonicalHost =
"canonical.bar.com";
On 2010/04/07 19:16:28, wtc wrote:
> Nit: rename this constant kCanonicalName or kCanonicalHostname.

Done.

http://codereview.chromium.org/1593015/diff/1/9#newcode174
net/base/address_list_unittest.cc:174: EXPECT_EQ("blah", canon_name3);
On 2010/04/07 19:16:28, wtc wrote:
> In general I think it's not necessary to guarantee that a
> function does not modify its output arguments on failure.
> Sometimes it's not easy to do that.

I document this behavior in the class header - but am willing to remove this
expectation for consistency sake.

http://codereview.chromium.org/1593015/diff/1/10
File net/base/host_cache.h (right):

http://codereview.chromium.org/1593015/diff/1/10#newcode46
net/base/host_cache.h:46: bool operator==(const Key& other) const {
On 2010/04/07 19:16:28, wtc wrote:
> Add a comment to document the non-obvious ordering in which
> you compare the members (do the expensive string comparison
> last) in operator== and operator<.

Done.

http://codereview.chromium.org/1593015/diff/1/12
File net/base/host_resolver.h (right):

http://codereview.chromium.org/1593015/diff/1/12#newcode91
net/base/host_resolver.h:91: // Flags for the address.
On 2010/04/07 19:16:28, wtc wrote:
> Nit: address => "host resolver" or "host resolution"

Done.

http://codereview.chromium.org/1593015/diff/1/14
File net/base/host_resolver_impl_unittest.cc (right):

http://codereview.chromium.org/1593015/diff/1/14#newcode76
net/base/host_resolver_impl_unittest.cc:76: : hostname(hostname),
On 2010/04/07 19:16:28, wtc wrote:
> Nit: it is fine to list multiple member initializers on one
> line.  (I checked with Mark Mentovai, who maintains our C++
> style guide.)

Done.

http://codereview.chromium.org/1593015/diff/1/20
File net/socket/socket_test_util.cc (right):

http://codereview.chromium.org/1593015/diff/1/20#newcode55
net/socket/socket_test_util.cc:55: 0,
On 2010/04/07 19:16:28, wtc wrote:
> Nit: put 0 and address on the same line.

Done.

http://codereview.chromium.org/1593015/diff/3001/4008
File net/base/address_list_unittest.cc (right):

http://codereview.chromium.org/1593015/diff/3001/4008#newcode130
net/base/address_list_unittest.cc:130: memset(&address, 0x0, sizeof(address));
On 2010/04/07 19:21:56, wtc wrote:
> Please document that the contents of |address| don't matter
> to this unit test.  Or just initialze |address| to some
> meaningful values.  You can use eroman's
> AddressList::CreateIPv6Address method.

Done.

[willchan no longer on Chromium, 2010-04-07 20:29:03]

I only looked at HostResolverFlags.

http://codereview.chromium.org/1593015/diff/22002/25005
File net/base/address_family.h (right):

http://codereview.chromium.org/1593015/diff/22002/25005#newcode23
net/base/address_family.h:23: typedef int HostResolverFlags;
Why is HostResolverFlags in address_family.h?  Why not put this in
host_resolver.h?  Is it only ever used with HostResolver?  If so, I'd expect it
be be something like

enum Flags {
  NONE = 0,
  CANONNAME = 1 << 0,
};

scoped within HostResolver.

That's assuming it's only used within HostResolver.  I haven't read through the
whole changelist to ascertain this.

[cbentzel, 2010-04-07 20:41:06]

http://codereview.chromium.org/1593015/diff/22002/25005
File net/base/address_family.h (right):

http://codereview.chromium.org/1593015/diff/22002/25005#newcode23
net/base/address_family.h:23: typedef int HostResolverFlags;
Circular references - this is also used by HostCache (as part of the Key
structure) and HostResolverProc (where it is ultimately turned into
AI_ADDRINFO).

I originally called it AddressFlags.

[cbentzel, 2010-04-07 20:56:47]

> I originally called it AddressFlags.

You know, I could go back to my very original implementation and just pass along
a bool for canonname. I wanted an enum so the caller sites would have some
symbolic context instead of simply true/false. But it seems like this particular
enum and where it is located is a bit contentious.

[wtc, 2010-04-07 21:33:56]

I prefer enum flags over bool arguments.  A true/false
argument always requires me look up the documentation.

We can adjust the header in which HostResolverFlags is
defined later.  In the worse case, we can create its own
header host_resolver_flags.h.

http://codereview.chromium.org/1593015/diff/1/6
File net/base/address_family.h (right):

http://codereview.chromium.org/1593015/diff/1/6#newcode21
net/base/address_family.h:21: HOST_RESOLVER_FLAGS_CANONNAME = 1 << 0, //
AI_CANONNAME, 0x1
On 2010/04/07 20:09:31, cbentzel wrote:
>
> Done. Not sure I fully like the removal of FLAGS, but I understand it.

Understood.  The reason I suggested removing _FLAGS is for
consistency with the LOAD_xxx and CERT_STATUS_xxx flags
defined in load_flags.h and cert_status_flags.h.

If we include FLAGS, it seems that the singular FLAG would
be more correct.  Want to do that instead?

[willchan no longer on Chromium, 2010-04-07 21:39:00]

On Wed, Apr 7, 2010 at 2:33 PM,  <wtc@chromium.org> wrote:
> I prefer enum flags over bool arguments.  A true/false
> argument always requires me look up the documentation.

Agreed

>
> We can adjust the header in which HostResolverFlags is
> defined later.  In the worse case, we can create its own
> header host_resolver_flags.h.

It sounds like there was already more discussion on this which I had
not read.  I am content to let others figure out the appropriate
location.

>
>
> http://codereview.chromium.org/1593015/diff/1/6
> File net/base/address_family.h (right):
>
> http://codereview.chromium.org/1593015/diff/1/6#newcode21
> net/base/address_family.h:21: HOST_RESOLVER_FLAGS_CANONNAME = 1 << 0, //
> AI_CANONNAME, 0x1
> On 2010/04/07 20:09:31, cbentzel wrote:
>
>> Done. Not sure I fully like the removal of FLAGS, but I understand it.
>
> Understood.  The reason I suggested removing _FLAGS is for
> consistency with the LOAD_xxx and CERT_STATUS_xxx flags
> defined in load_flags.h and cert_status_flags.h.
>
> If we include FLAGS, it seems that the singular FLAG would
> be more correct.  Want to do that instead?
>
> http://codereview.chromium.org/1593015/show
>

[cbentzel, 2010-04-07 23:22:16]

Thanks for feedback.

I'm going to submit this as is and make any name/header changes in a
subsequent CL.

On Apr 7, 2010 5:38 PM, "William Chan (陈智昌)" <willchan@chromium.org> wrote:

On Wed, Apr 7, 2010 at 2:33 PM, <wtc@chromium.org> wrote:
> I prefer enum flags over bool arguments...
Agreed


>
> We can adjust the header in which HostResolverFlags is
> defined later.  In the worse case, we ...
It sounds like there was already more discussion on this which I had
not read.  I am content to let others figure out the appropriate
location.


>
>
> http://codereview.chromium.org/1593015/diff/1/6
> File net/base/address_family.h (right):
>
>...

[wtc, 2010-04-08 15:09:40]

Chris, Eric, I have a question.

http://codereview.chromium.org/1593015/diff/22002/25006
File net/base/address_list.cc (right):

http://codereview.chromium.org/1593015/diff/22002/25006#newcode108
net/base/address_list.cc:108: void AddressList::Append(const struct addrinfo*
head) {
Since we assume ai_cannonname is in the first element
of the linked list, if the head of the current list
doesn't have ai_cannonname but the |head| argument does,
should we move its ai_cannonname to the head of the
current list?

Alternatively, we can consider modifying GetCanonicalName
to walk the linked list until it finds an element with
ai_cannonname.

[eroman, 2010-04-08 19:10:50]

http://codereview.chromium.org/1593015/diff/22002/25006
File net/base/address_list.cc (right):

http://codereview.chromium.org/1593015/diff/22002/25006#newcode108
net/base/address_list.cc:108: void AddressList::Append(const struct addrinfo*
head) {
On 2010/04/08 15:09:40, wtc wrote:
> Since we assume ai_cannonname is in the first element
> of the linked list, if the head of the current list
> doesn't have ai_cannonname but the |head| argument does,
> should we move its ai_cannonname to the head of the
> current list?
> 
> Alternatively, we can consider modifying GetCanonicalName
> to walk the linked list until it finds an element with
> ai_cannonname.

Append() was only exposed for use by a unittest
(http://codereview.chromium.org/1593015) in constructing fake lists.

We could add DCHECKs for this situation since we don't expect it to happen.

Additionally we could make Append() private, and only exposed through #ifdef
UNITTESTS.

[wtc, 2010-04-08 20:11:11]

http://codereview.chromium.org/1593015/diff/22002/25006
File net/base/address_list.cc (right):

http://codereview.chromium.org/1593015/diff/22002/25006#newcode108
net/base/address_list.cc:108: void AddressList::Append(const struct addrinfo*
head) {
On 2010/04/08 19:10:50, eroman wrote:
>
> Append() was only exposed for use by a unittest
> (http://codereview.chromium.org/1593015) in constructing fake lists.
> 
> We could add DCHECKs for this situation since we don't expect it to happen.
> 
> Additionally we could make Append() private, and only exposed through #ifdef
> UNITTESTS.

This sounds good.