Flag myIpAddress requests for chromeos

Flag myIpAddress requests for chromeos

This is a follow-up to https://codereview.chromium.org/238433003

We need to differentiate between myIpAddress requests and other
localhost requests so that local web servers and tests work correctly.

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

[stevenjb, 2014-06-26 02:42:19]

PTAL at your earliest convenience. As you anticipated, unconditionally resolving
localhost to an externally visible IP address breakes some things (running a
local web server app, some un-automated tests). I'd like to fix this quickly
rather than revert the CL. Thanks!

[eroman, 2014-06-26 03:13:48]

https://codereview.chromium.org/355953002/diff/20001/net/base/address_family.h
File net/base/address_family.h (right):

https://codereview.chromium.org/355953002/diff/20001/net/base/address_family....
net/base/address_family.h:32: HOST_RESOLVER_MY_IP_ADDRESS = 1 << 4
Although convenient, Flags isn't the right place to put this concept (The
description above says it is things that affect addrinfo.ai_flags).

Instead please make this a property of RequestInfo (I have more details on this
in following comments).

https://codereview.chromium.org/355953002/diff/20001/net/proxy/proxy_resolver...
File net/proxy/proxy_resolver_v8_tracing.cc (right):

https://codereview.chromium.org/355953002/diff/20001/net/proxy/proxy_resolver...
net/proxy/proxy_resolver_v8_tracing.cc:972: host_port.set_host(GetHostName());
Lets not set the host in the case of trying to resolve "my ip address". 

How about adding a constructor for RequestInfo() that takes 0 parameters. And
then have some setter for "set_is_my_ip_address()".

This will require adding some logic to HostResolverImpl to test for if
is_my_ip_address(), and if so using an effective host of GetHostName().

(Or in the future, hopefully doing something better on other platforms too).

[stevenjb, 2014-06-26 03:36:45]

PTAL

https://codereview.chromium.org/355953002/diff/20001/net/base/address_family.h
File net/base/address_family.h (right):

https://codereview.chromium.org/355953002/diff/20001/net/base/address_family....
net/base/address_family.h:32: HOST_RESOLVER_MY_IP_ADDRESS = 1 << 4
On 2014/06/26 03:13:47, eroman wrote:
> Although convenient, Flags isn't the right place to put this concept (The
> description above says it is things that affect addrinfo.ai_flags).
> 
> Instead please make this a property of RequestInfo (I have more details on
this
> in following comments).

Done.

https://codereview.chromium.org/355953002/diff/20001/net/proxy/proxy_resolver...
File net/proxy/proxy_resolver_v8_tracing.cc (right):

https://codereview.chromium.org/355953002/diff/20001/net/proxy/proxy_resolver...
net/proxy/proxy_resolver_v8_tracing.cc:972: host_port.set_host(GetHostName());
On 2014/06/26 03:13:47, eroman wrote:
> Lets not set the host in the case of trying to resolve "my ip address". 
> 
> How about adding a constructor for RequestInfo() that takes 0 parameters. And
> then have some setter for "set_is_my_ip_address()".
> 
> This will require adding some logic to HostResolverImpl to test for if
> is_my_ip_address(), and if so using an effective host of GetHostName().
> 
> (Or in the future, hopefully doing something better on other platforms too).

That sounds reasonable, but I'd be worried about breaking existing
(non-chromeos) implementations if we did any of that. Maybe add a TODO instead?

[stevenjb, 2014-06-26 03:40:35]

https://codereview.chromium.org/355953002/diff/20001/net/proxy/proxy_resolver...
File net/proxy/proxy_resolver_v8_tracing.cc (right):

https://codereview.chromium.org/355953002/diff/20001/net/proxy/proxy_resolver...
net/proxy/proxy_resolver_v8_tracing.cc:972: host_port.set_host(GetHostName());
On 2014/06/26 03:36:45, stevenjb wrote:
> On 2014/06/26 03:13:47, eroman wrote:
> > Lets not set the host in the case of trying to resolve "my ip address". 
> > 
> > How about adding a constructor for RequestInfo() that takes 0 parameters.
And
> > then have some setter for "set_is_my_ip_address()".
> > 
> > This will require adding some logic to HostResolverImpl to test for if
> > is_my_ip_address(), and if so using an effective host of GetHostName().
> > 
> > (Or in the future, hopefully doing something better on other platforms too).
> 
> That sounds reasonable, but I'd be worried about breaking existing
> (non-chromeos) implementations if we did any of that. Maybe add a TODO
instead?

To be clear: I would like to keep this as simple as possible so that we can
safely merge this CL to 37. Otherwise I will need to revert the previous CL and
put this on ice for a while longer (I'm swamped for 38).

[eroman, 2014-06-26 19:58:46]

lgtm

https://codereview.chromium.org/355953002/diff/20001/net/proxy/proxy_resolver...
File net/proxy/proxy_resolver_v8_tracing.cc (right):

https://codereview.chromium.org/355953002/diff/20001/net/proxy/proxy_resolver...
net/proxy/proxy_resolver_v8_tracing.cc:972: host_port.set_host(GetHostName());
On 2014/06/26 03:40:35, stevenjb wrote:
> On 2014/06/26 03:36:45, stevenjb wrote:
> > On 2014/06/26 03:13:47, eroman wrote:
> > > Lets not set the host in the case of trying to resolve "my ip address". 
> > > 
> > > How about adding a constructor for RequestInfo() that takes 0 parameters.
> And
> > > then have some setter for "set_is_my_ip_address()".
> > > 
> > > This will require adding some logic to HostResolverImpl to test for if
> > > is_my_ip_address(), and if so using an effective host of GetHostName().
> > > 
> > > (Or in the future, hopefully doing something better on other platforms
too).
> > 
> > That sounds reasonable, but I'd be worried about breaking existing
> > (non-chromeos) implementations if we did any of that. Maybe add a TODO
> instead?
> 
> To be clear: I would like to keep this as simple as possible so that we can
> safely merge this CL to 37. Otherwise I will need to revert the previous CL
and
> put this on ice for a while longer (I'm swamped for 38).

ok that's reasonable. How about making set_is_my_ip_address() internally call
set_host(GetHostName()), and attaching a TODO() to check for is_my_ip_address()
in the implementation?

(Then you can remove the set_host(GetHostName()) from this file).

https://codereview.chromium.org/355953002/diff/40001/net/dns/host_resolver.h
File net/dns/host_resolver.h (right):

https://codereview.chromium.org/355953002/diff/40001/net/dns/host_resolver.h#...
net/dns/host_resolver.h:111: // for localhost, currently used by Chrome OS).
TODO(eroman): Consider
Can you remove the TODO(eroman) ?

[stevenjb, 2014-06-26 22:04:36]

https://codereview.chromium.org/355953002/diff/20001/net/proxy/proxy_resolver...
File net/proxy/proxy_resolver_v8_tracing.cc (right):

https://codereview.chromium.org/355953002/diff/20001/net/proxy/proxy_resolver...
net/proxy/proxy_resolver_v8_tracing.cc:972: host_port.set_host(GetHostName());
On 2014/06/26 19:58:46, eroman wrote:
> On 2014/06/26 03:40:35, stevenjb wrote:
> > On 2014/06/26 03:36:45, stevenjb wrote:
> > > On 2014/06/26 03:13:47, eroman wrote:
> > > > Lets not set the host in the case of trying to resolve "my ip address". 
> > > > 
> > > > How about adding a constructor for RequestInfo() that takes 0
parameters.
> > And
> > > > then have some setter for "set_is_my_ip_address()".
> > > > 
> > > > This will require adding some logic to HostResolverImpl to test for if
> > > > is_my_ip_address(), and if so using an effective host of GetHostName().
> > > > 
> > > > (Or in the future, hopefully doing something better on other platforms
> too).
> > > 
> > > That sounds reasonable, but I'd be worried about breaking existing
> > > (non-chromeos) implementations if we did any of that. Maybe add a TODO
> > instead?
> > 
> > To be clear: I would like to keep this as simple as possible so that we can
> > safely merge this CL to 37. Otherwise I will need to revert the previous CL
> and
> > put this on ice for a while longer (I'm swamped for 38).
> 
> ok that's reasonable. How about making set_is_my_ip_address() internally call
> set_host(GetHostName()), and attaching a TODO() to check for
is_my_ip_address()
> in the implementation?
> 
> (Then you can remove the set_host(GetHostName()) from this file).

I *think* I understand what you are suggesting, but I would still want to test
that carefully, and at that point I think your original suggestion probably
makes more sense.

I will move the TODO here and leave it unassigned since there does not appear to
be a good owner.

https://codereview.chromium.org/355953002/diff/40001/net/dns/host_resolver.h
File net/dns/host_resolver.h (right):

https://codereview.chromium.org/355953002/diff/40001/net/dns/host_resolver.h#...
net/dns/host_resolver.h:111: // for localhost, currently used by Chrome OS).
TODO(eroman): Consider
On 2014/06/26 19:58:46, eroman wrote:
> Can you remove the TODO(eroman) ?

Done.

[stevenjb, 2014-06-26 22:04:54]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2014-06-26 22:05:50]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/stevenjb@chromium.org/355953002/60001

[eroman, 2014-06-26 22:14:22]

https://codereview.chromium.org/355953002/diff/60001/net/proxy/proxy_resolver...
File net/proxy/proxy_resolver_v8_tracing.cc (right):

https://codereview.chromium.org/355953002/diff/60001/net/proxy/proxy_resolver...
net/proxy/proxy_resolver_v8_tracing.cc:978: // TODO: Provide a RequestInfo
construction mechanism that does not
TODO(stevnjb):

Note that the name in parens is NOT the person assigned the task. It is the
person who wrote the comment.

If you prefer you can put my name in there instead.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=TODO_C...

[stevenjb, 2014-06-26 22:21:24]

https://codereview.chromium.org/355953002/diff/60001/net/proxy/proxy_resolver...
File net/proxy/proxy_resolver_v8_tracing.cc (right):

https://codereview.chromium.org/355953002/diff/60001/net/proxy/proxy_resolver...
net/proxy/proxy_resolver_v8_tracing.cc:978: // TODO: Provide a RequestInfo
construction mechanism that does not
On 2014/06/26 22:14:22, eroman wrote:
> TODO(stevnjb):
> 
> Note that the name in parens is NOT the person assigned the task. It is the
> person who wrote the comment.
> 
> If you prefer you can put my name in there instead.
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=TODO_C...

Well, actually it says "identifier of the person who can best provide context
about the problem referenced by the TODO." I was hesitant to put my name there
because all of the changes here outside of src/chromeos are still a bit beyond
my purview, although I suppose I know just enough to be dangerous at this point.

[commit-bot: I haz the power, 2014-06-27 03:44:28]

Change committed as 280230