Don't resolve IP literals.

Don't resolve IP literals.

For each resolution request this checks to see if this 'host' is a literal ip address.  If so, it synthesises a struct addrinfo and returns it without adding it to the cache.

BUG=39830
TEST=unit tests, new and old

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

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

[eroman, 2010-08-11 22:17:09]

You also need to update mock_host_resolver.{h,cc}.

In particular, see the AddIPLiteralRule().

That code is no longer reachable since it will be resolved by HostResolverImpl
before it reaches the HostResolverProc.

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

http://codereview.chromium.org/3023048/diff/7001/8002#newcode146
net/base/address_list.cc:146: void AddressList::SetToAddress(const
IPAddressNumber& address,
Can this be a constructor instead of a setter?

http://codereview.chromium.org/3023048/diff/7001/8002#newcode191
net/base/address_list.cc:191: AddressList
AddressList::CreateIPv4Address(unsigned char data[4],
Can you delete this function in favor of the new combined one?

http://codereview.chromium.org/3023048/diff/7001/8002#newcode204
net/base/address_list.cc:204: AddressList
AddressList::CreateIPv6Address(unsigned char data[16],
Can you delete this function in favor of the new combined one?

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

http://codereview.chromium.org/3023048/diff/7001/8004#newcode188
net/base/address_list_unittest.cc:188: continue;
Can you ONLY skip on failure for the IPv6 literals? (since those are the only
ones which should have possibility of failing on some buildbots).

http://codereview.chromium.org/3023048/diff/7001/8004#newcode204
net/base/address_list_unittest.cc:204: EXPECT_TRUE(memcmp(good_ai->ai_addr,
test_ai->ai_addr, sockaddr_size) == 0);
how about EXPECT_EQ(0, ...)

http://codereview.chromium.org/3023048/diff/7001/8005
File net/base/host_resolver_impl.cc (right):

http://codereview.chromium.org/3023048/diff/7001/8005#newcode774
net/base/host_resolver_impl.cc:774: IPAddressNumber ip_literal;
I would call this ip_number rather than ip_literal.

(since the function is called IPLiteralToIpNumber, and this is the out
parameter).

http://codereview.chromium.org/3023048/diff/7001/8005#newcode777
net/base/host_resolver_impl.cc:777: if (info.host_resolver_flags() &
HOST_RESOLVER_CANONNAME)
Might want to add a safety-net here for any future flags. (perhaps break out if
the resolver flags isnt 0 or host_resolver_canonname.

http://codereview.chromium.org/3023048/diff/7001/8006
File net/base/host_resolver_impl_unittest.cc (left):

http://codereview.chromium.org/3023048/diff/7001/8006#oldcode405
net/base/host_resolver_impl_unittest.cc:405: if (err == ERR_NAME_NOT_RESOLVED)
good spot with updating this!

[vandebo (ex-Chrome), 2010-08-12 03:13:07]

On 2010/08/11 22:17:09, eroman wrote:
> You also need to update mock_host_resolver.{h,cc}.
> 
> In particular, see the AddIPLiteralRule().
> 
> That code is no longer reachable since it will be resolved by HostResolverImpl
> before it reaches the HostResolverProc.

I don't think that is correct - the host_pattern part of AddIPLiteralRule need
not be an IP literal.  And since my catch for IP literals is before
HostResolverProc, AddIPLiteralRule is still needed.  (I assume it was added
because IPv6 literals wouldn't resolve on systems without IPv6 addresses because
of AI_ADDRCONFIG.)  Though it took me several tries to come to this conclusion,
so I could be wrong.

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

http://codereview.chromium.org/3023048/diff/7001/8002#newcode146
net/base/address_list.cc:146: void AddressList::SetToAddress(const
IPAddressNumber& address,
On 2010/08/11 22:17:09, eroman wrote:
> Can this be a constructor instead of a setter?

Not without either copying the result (wasteful) or restructuring
HostResolver::Resolve - it takes an AddresList*

http://codereview.chromium.org/3023048/diff/7001/8002#newcode191
net/base/address_list.cc:191: AddressList
AddressList::CreateIPv4Address(unsigned char data[4],
On 2010/08/11 22:17:09, eroman wrote:
> Can you delete this function in favor of the new combined one?

Done.

http://codereview.chromium.org/3023048/diff/7001/8002#newcode204
net/base/address_list.cc:204: AddressList
AddressList::CreateIPv6Address(unsigned char data[16],
On 2010/08/11 22:17:09, eroman wrote:
> Can you delete this function in favor of the new combined one?

Done.

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

http://codereview.chromium.org/3023048/diff/7001/8004#newcode188
net/base/address_list_unittest.cc:188: continue;
On 2010/08/11 22:17:09, eroman wrote:
> Can you ONLY skip on failure for the IPv6 literals? (since those are the only
> ones which should have possibility of failing on some buildbots).

Done.

http://codereview.chromium.org/3023048/diff/7001/8004#newcode204
net/base/address_list_unittest.cc:204: EXPECT_TRUE(memcmp(good_ai->ai_addr,
test_ai->ai_addr, sockaddr_size) == 0);
On 2010/08/11 22:17:09, eroman wrote:
> how about EXPECT_EQ(0, ...)

Done.

http://codereview.chromium.org/3023048/diff/7001/8005
File net/base/host_resolver_impl.cc (right):

http://codereview.chromium.org/3023048/diff/7001/8005#newcode774
net/base/host_resolver_impl.cc:774: IPAddressNumber ip_literal;
On 2010/08/11 22:17:09, eroman wrote:
> I would call this ip_number rather than ip_literal.
> 
> (since the function is called IPLiteralToIpNumber, and this is the out
> parameter).

Done.

http://codereview.chromium.org/3023048/diff/7001/8005#newcode777
net/base/host_resolver_impl.cc:777: if (info.host_resolver_flags() &
HOST_RESOLVER_CANONNAME)
On 2010/08/11 22:17:09, eroman wrote:
> Might want to add a safety-net here for any future flags. (perhaps break out
if
> the resolver flags isnt 0 or host_resolver_canonname.

Done.

http://codereview.chromium.org/3023048/diff/7001/8006
File net/base/host_resolver_impl_unittest.cc (left):

http://codereview.chromium.org/3023048/diff/7001/8006#oldcode405
net/base/host_resolver_impl_unittest.cc:405: if (err == ERR_NAME_NOT_RESOLVED)
On 2010/08/11 22:17:09, eroman wrote:
> good spot with updating this!

I got lucky - I happened upon it when looking for where to add tests.

[eroman, 2010-08-12 18:19:47]

> I don't think that is correct - the host_pattern part of AddIPLiteralRule need
> not be an IP literal.  And since my catch for IP literals is before
> HostResolverProc, AddIPLiteralRule is still needed.

The particular case where this will break is when trying to substitute a IP
literal by an IP literal.

i.e:

AddIPLiteralRule("dead::beef", "127.0.0.1", string());

Before it would substitute "dead::beef" to "127.0.0.1", whereas now now
substitution will occur since the HostResolverImpl will catch "dead::beef" as an
IP literal.

I am not sure that any test relies on this behavior, but we should mention this
limitation in the comment, and then DCHECK() that the input to this rule is not
an IP literal otherwise consumers may not be getting what they expect.

> (I assume it was added because IPv6 literals wouldn't resolve on systems
without
> IPv6 addresses because of AI_ADDRCONFIG.)  Though it took me several tries to
come
> to this conclusion, I could be wrong.

The need for such function is because support for IPv6 literals is limited
across systems, including our buildbots. Even just calling getaddrinfo() with
NUMERIC flag can fail when passing IPv6 literals on systems that don't enable
IPv6... so this was necessary to write some tests for IPv6 stuff without getting
failures on such systems.

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

http://codereview.chromium.org/3023048/diff/7001/8002#newcode146
net/base/address_list.cc:146: void AddressList::SetToAddress(const
IPAddressNumber& address,
On 2010/08/12 03:13:07, vandebo wrote:
> On 2010/08/11 22:17:09, eroman wrote:
> > Can this be a constructor instead of a setter?
> 
> Not without either copying the result (wasteful) or restructuring
> HostResolver::Resolve - it takes an AddresList*

AddressList uses a refcounted core, so copying should be cheap. But am OK if you
want to keep it as a setter.

http://codereview.chromium.org/3023048/diff/14001/15003
File net/base/address_list.h (right):

http://codereview.chromium.org/3023048/diff/14001/15003#newcode66
net/base/address_list.h:66: // ai_canonname field of the addrinfo struct.
May want to mention that port will be set to 0.

http://codereview.chromium.org/3023048/diff/14001/15004
File net/base/address_list_unittest.cc (right):

http://codereview.chromium.org/3023048/diff/14001/15004#newcode22
net/base/address_list_unittest.cc:22: bool ignore_failures) {
This parameter seems kinda funky to me.
How about returning an int instead?

EXPECT_EQ(OK, CreateAddressList(...));

That gives all the same functionality as this "ignore_failures" boolean, but
with less awkwardness.

http://codereview.chromium.org/3023048/diff/14001/15005
File net/base/host_resolver_impl.cc (right):

http://codereview.chromium.org/3023048/diff/14001/15005#newcode777
net/base/host_resolver_impl.cc:777: ~(HOST_RESOLVER_CANONNAME |
HOST_RESOLVER_LOOPBACK_ONLY)), 0);
Can you add a log message explaining what this means? Maybe something like:

<< " Unhandled flag";

http://codereview.chromium.org/3023048/diff/14001/15005#newcode780
net/base/host_resolver_impl.cc:780: canonical_name = info.hostname();
Are you sure this is the same behavior that AI_CANONNAME gives with an IP
literal?

In particular, what is the expectation for IPv6 literals which can be normalized
in lots of different ways. (is the assumption here that the input literal is
already canonicalized?)

[vandebo (ex-Chrome), 2010-08-13 00:55:00]

Good points all around, I think I've addressed them.

On 2010/08/12 18:19:47, eroman wrote:
> The particular case where this will break is when trying to substitute a IP
> literal by an IP literal.
> 
> i.e:
> 
> AddIPLiteralRule("dead::beef", "127.0.0.1", string());
> 
> Before it would substitute "dead::beef" to "127.0.0.1", whereas now now
> substitution will occur since the HostResolverImpl will catch "dead::beef" as
an
> IP literal.
> 
> I am not sure that any test relies on this behavior, but we should mention
this
> limitation in the comment, and then DCHECK() that the input to this rule is
not
> an IP literal otherwise consumers may not be getting what they expect.

Ahh.  It seems a bit strange to try to map one IP to another.  I added the
DCHECK to make sure it doesn't happen.

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

http://codereview.chromium.org/3023048/diff/7001/8002#newcode146
net/base/address_list.cc:146: void AddressList::SetToAddress(const
IPAddressNumber& address,
On 2010/08/12 18:19:47, eroman wrote:
> On 2010/08/12 03:13:07, vandebo wrote:
> > On 2010/08/11 22:17:09, eroman wrote:
> > > Can this be a constructor instead of a setter?
> > 
> > Not without either copying the result (wasteful) or restructuring
> > HostResolver::Resolve - it takes an AddresList*
> 
> AddressList uses a refcounted core, so copying should be cheap. But am OK if
you
> want to keep it as a setter.

Fair enough. Done.

http://codereview.chromium.org/3023048/diff/14001/15003
File net/base/address_list.h (right):

http://codereview.chromium.org/3023048/diff/14001/15003#newcode66
net/base/address_list.h:66: // ai_canonname field of the addrinfo struct.
On 2010/08/12 18:19:47, eroman wrote:
> May want to mention that port will be set to 0.

I changed the new constructor to take the port.

http://codereview.chromium.org/3023048/diff/14001/15004
File net/base/address_list_unittest.cc (right):

http://codereview.chromium.org/3023048/diff/14001/15004#newcode22
net/base/address_list_unittest.cc:22: bool ignore_failures) {
On 2010/08/12 18:19:47, eroman wrote:
> This parameter seems kinda funky to me.
> How about returning an int instead?
> 
> EXPECT_EQ(OK, CreateAddressList(...));
> 
> That gives all the same functionality as this "ignore_failures" boolean, but
> with less awkwardness.

Done.

http://codereview.chromium.org/3023048/diff/14001/15005
File net/base/host_resolver_impl.cc (right):

http://codereview.chromium.org/3023048/diff/14001/15005#newcode777
net/base/host_resolver_impl.cc:777: ~(HOST_RESOLVER_CANONNAME |
HOST_RESOLVER_LOOPBACK_ONLY)), 0);
On 2010/08/12 18:19:47, eroman wrote:
> Can you add a log message explaining what this means? Maybe something like:
> 
> << " Unhandled flag";

Done.

http://codereview.chromium.org/3023048/diff/14001/15005#newcode780
net/base/host_resolver_impl.cc:780: canonical_name = info.hostname();
On 2010/08/12 18:19:47, eroman wrote:
> Are you sure this is the same behavior that AI_CANONNAME gives with an IP
> literal?
> 
> In particular, what is the expectation for IPv6 literals which can be
normalized
> in lots of different ways. (is the assumption here that the input literal is
> already canonicalized?)

You're right.  I added explicit code to canonicalize the IP address.

[eroman, 2010-08-13 01:26:08]

LGTM after looking at my CanonicalizeIPAddress() comments.

http://codereview.chromium.org/3023048/diff/9002/32003
File net/base/address_list.h (right):

http://codereview.chromium.org/3023048/diff/9002/32003#newcode25
net/base/address_list.h:25: // Construct an address list for a single IP
literal.  If |canonical_name| is
nit: Construct --> Constructs.

http://codereview.chromium.org/3023048/diff/9002/32007
File net/base/mock_host_resolver.cc (right):

http://codereview.chromium.org/3023048/diff/9002/32007#newcode157
net/base/mock_host_resolver.cc:157: // Literals will now always be resolved to
themselves.  As a consequence,
nit: I suggest wording this in the present "Literals are always resolved to
themselves by HostResolverImpl, consequently we do not support remapping them
via the HostResolverProc.".

http://codereview.chromium.org/3023048/diff/9002/32008
File net/base/net_util.cc (right):

http://codereview.chromium.org/3023048/diff/9002/32008#newcode1855
net/base/net_util.cc:1855: std::string CanonicalizeIPAddress(const std::string&
ip_literal) {
This is a rather unfortunate function.

We have already parsed the IP literal to a number in HostResolverImpl, which
could be fed directly into GURL (although that function is not currently
public).

Can you simply call NetAddressToString() instead of adding
CanonicalizeIPAddress? This has the advantage of using the system's
canonicalizer, so it should give the same value as if getaddrinfo() had been
called.

Assuming we do keep CanonicalizeIPAddress, it needs some unittests.

[vandebo (ex-Chrome), 2010-08-13 03:44:57]

http://codereview.chromium.org/3023048/diff/9002/32003
File net/base/address_list.h (right):

http://codereview.chromium.org/3023048/diff/9002/32003#newcode25
net/base/address_list.h:25: // Construct an address list for a single IP
literal.  If |canonical_name| is
On 2010/08/13 01:26:08, eroman wrote:
> nit: Construct --> Constructs.

Done.

http://codereview.chromium.org/3023048/diff/9002/32007
File net/base/mock_host_resolver.cc (right):

http://codereview.chromium.org/3023048/diff/9002/32007#newcode157
net/base/mock_host_resolver.cc:157: // Literals will now always be resolved to
themselves.  As a consequence,
On 2010/08/13 01:26:08, eroman wrote:
> nit: I suggest wording this in the present "Literals are always resolved to
> themselves by HostResolverImpl, consequently we do not support remapping them
> via the HostResolverProc.".

Done.

http://codereview.chromium.org/3023048/diff/9002/32008
File net/base/net_util.cc (right):

http://codereview.chromium.org/3023048/diff/9002/32008#newcode1855
net/base/net_util.cc:1855: std::string CanonicalizeIPAddress(const std::string&
ip_literal) {
On 2010/08/13 01:26:08, eroman wrote:
> This is a rather unfortunate function.
> 
> We have already parsed the IP literal to a number in HostResolverImpl, which
> could be fed directly into GURL (although that function is not currently
> public).
> 
> Can you simply call NetAddressToString() instead of adding
> CanonicalizeIPAddress? This has the advantage of using the system's
> canonicalizer, so it should give the same value as if getaddrinfo() had been
> called.

I hadn't seen/processed that function.  It shouldn't involve any network access,
so it should be ok to use in the context of this change.  There's now a
const_cast in test code, but I think that's ok.

[eroman, 2010-08-13 08:04:44]

LGTM

http://codereview.chromium.org/3023048/diff/22002/27006
File net/base/address_list.h (right):

http://codereview.chromium.org/3023048/diff/22002/27006#newcode26
net/base/address_list.h:26: // |canonicalize_name| is true, fill out the
ai_canonname field.
Please expand this comment to mention it fills ai_cannonname with the IP
literal. (Wasn't clear to me reading this).

http://codereview.chromium.org/3023048/diff/22002/27007
File net/base/address_list_unittest.cc (right):

http://codereview.chromium.org/3023048/diff/22002/27007#newcode20
net/base/address_list_unittest.cc:20: int port,
please fix indentation.