Provide Shill IP Address to myIpAddress()

Provide Shill IP Address to myIpAddress()

On Chrome OS we need to get the IP Address to use for myIpAddress() from Shill since gethostname() returns localhost.

BUG=175652
TBR=sky@chromium.org

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

[stevenjb, 2014-04-15 00:45:52]

This looks like it might be a reasonable way to do this. We need to add tests,
but I'm not actually sure how to test this.

[eroman, 2014-04-16 02:12:52]

This change does not work, please test it.

The function that you need to change is:

ProxyResolverV8Tracing::Job::MakeDnsRequestInfo()

In particular this line:

  if (op == MY_IP_ADDRESS || op == MY_IP_ADDRESS_EX) {
    host_port.set_host(GetHostName());
  }

Setting the IP address as part of the proxy_config is not the right abstraction.
There are many configs that can be created (for different profiles, set by
extensions, etc.), so it isn't expected that each config can be touched up in
this fashion before using it.

A better approach would be to change net::HostResolver() to directly support
answering "what is my ip address". In other words, rather than calling into
HostResolver() with requestinfo for host=GetHostName(), it should call in with a
requestinfo specifying "my ip address".

The particular platform-specific mechanism for retrieving the "current ip
address" (whatever exactly that means) would then be abstracted in
HostResolverImpl.

To test this, you should create a simple PAC script which looks something like:

function FindProxyForURL(url, host) {
  alert('myIpAddress:' + myIpAddress());
  alert('myIpAddressEx:' + myIpAddressEx());
}

And then run chrome pointing at that PAC script (either as a file:// url, or as
some HTTP url).

Then go to about:net-internals to see what the outputs from alert() were.

Cheers

https://codereview.chromium.org/238433003/diff/1/net/proxy/proxy_resolver_v8.cc
File net/proxy/proxy_resolver_v8.cc (right):

https://codereview.chromium.org/238433003/diff/1/net/proxy/proxy_resolver_v8....
net/proxy/proxy_resolver_v8.cc:648:
args.GetReturnValue().Set(ASCIILiteralToV8String(
This is not correct. Note that this is the error handling.

[stevenjb, 2014-04-16 17:05:50]

On 2014/04/16 02:12:52, eroman wrote:
> This change does not work, please test it.
> 
> The function that you need to change is:
> 
> ProxyResolverV8Tracing::Job::MakeDnsRequestInfo()
> 
> In particular this line:
> 
>   if (op == MY_IP_ADDRESS || op == MY_IP_ADDRESS_EX) {
>     host_port.set_host(GetHostName());
>   }
> 
That looks reasonable.

> Setting the IP address as part of the proxy_config is not the right
abstraction.
> There are many configs that can be created (for different profiles, set by
> extensions, etc.), so it isn't expected that each config can be touched up in
> this fashion before using it.
> 
> A better approach would be to change net::HostResolver() to directly support
> answering "what is my ip address". In other words, rather than calling into
> HostResolver() with requestinfo for host=GetHostName(), it should call in with
a
> requestinfo specifying "my ip address".
> 
> The particular platform-specific mechanism for retrieving the "current ip
> address" (whatever exactly that means) would then be abstracted in
> HostResolverImpl.
> 
The challenge is that src/net includes a lot of very low level code, so
src/chromeos depends on it, therefore src/net can not depend on src/chromeos. I
looked through the code for a while but couldn't find any good existing hooks.
It sounds like we may need a delegate or Chrome specific implementation of
HostResolverImpl. I'm not sure that I have the bandwidth to get up to speed
sufficiently on that code to make the change.

> To test this, you should create a simple PAC script which looks something
like:
> 
> function FindProxyForURL(url, host) {
>   alert('myIpAddress:' + myIpAddress());
>   alert('myIpAddressEx:' + myIpAddressEx());
> }
> 
> And then run chrome pointing at that PAC script (either as a file:// url, or
as
> some HTTP url).

Unfortunately I know nothing about PAC, so I would need more detailed test
steps. How exactly does one "run chrome pointing at a PAC script"? i.e. how does
chrome know it's looking at a PAC script?

> 
> Then go to about:net-internals to see what the outputs from alert() were.
> 
> Cheers
> 

I appreciate the feedback. It may be more efficient for me to punt this over to
you or someone else. The Chrome OS specific calls, as you can see, are pretty
simple. The challenge is finding somewhere outside of src/net to make them so
that we can provide the IP address.

Alternatively, the NetworkStateHandler code knows (or can easily be made to
recognize) when the IP address changes, so if there was a convenient global
location, or singleton that could be passed to NetworkStateHandler, then
NetworkStateHandler could keep the default IP address up to date that way.

Cheers

[eroman, 2014-04-16 17:16:31]

On Wed, Apr 16, 2014 at 10:05 AM, <stevenjb@chromium.org> wrote:

> On 2014/04/16 02:12:52, eroman wrote:
>
>> This change does not work, please test it.
>>
>
>  The function that you need to change is:
>>
>
>  ProxyResolverV8Tracing::Job::MakeDnsRequestInfo()
>>
>
>  In particular this line:
>>
>
>     if (op == MY_IP_ADDRESS || op == MY_IP_ADDRESS_EX) {
>>      host_port.set_host(GetHostName());
>>    }
>>
>
> That looks reasonable.
>
>
>  Setting the IP address as part of the proxy_config is not the right
>>
> abstraction.
>
>> There are many configs that can be created (for different profiles, set by
>> extensions, etc.), so it isn't expected that each config can be touched
>> up in
>> this fashion before using it.
>>
>
>  A better approach would be to change net::HostResolver() to directly
>> support
>> answering "what is my ip address". In other words, rather than calling
>> into
>> HostResolver() with requestinfo for host=GetHostName(), it should call in
>> with
>>
> a
>
>> requestinfo specifying "my ip address".
>>
>
>  The particular platform-specific mechanism for retrieving the "current ip
>> address" (whatever exactly that means) would then be abstracted in
>> HostResolverImpl.
>>
>
> The challenge is that src/net includes a lot of very low level code, so
> src/chromeos depends on it, therefore src/net can not depend on
> src/chromeos. I
> looked through the code for a while but couldn't find any good existing
> hooks.
> It sounds like we may need a delegate or Chrome specific implementation of
> HostResolverImpl. I'm not sure that I have the bandwidth to get up to speed
> sufficiently on that code to make the change.
>
>
>  To test this, you should create a simple PAC script which looks something
>>
> like:
>
>  function FindProxyForURL(url, host) {
>>    alert('myIpAddress:' + myIpAddress());
>>    alert('myIpAddressEx:' + myIpAddressEx());
>> }
>>
>
>  And then run chrome pointing at that PAC script (either as a file:// url,
>> or
>>
> as
>
>> some HTTP url).
>>
>
> Unfortunately I know nothing about PAC, so I would need more detailed test
> steps. How exactly does one "run chrome pointing at a PAC script"? i.e.
> how does
> chrome know it's looking at a PAC script?


Easiest way is to launch chrome with --proxy-pac-url=file://XXXXX  where
XXXX is the path to the PAC script above (simple FindProxyForURL).\


> Then go to about:net-internals to see what the outputs from alert() were.
>>
>
>  Cheers
>>
>
>
> I appreciate the feedback. It may be more efficient for me to punt this
> over to
> you or someone else. The Chrome OS specific calls, as you can see, are
> pretty
> simple. The challenge is finding somewhere outside of src/net to make them
> so
> that we can provide the IP address.
>
> Alternatively, the NetworkStateHandler code knows (or can easily be made to
> recognize) when the IP address changes, so if there was a convenient global
> location, or singleton that could be passed to NetworkStateHandler, then
> NetworkStateHandler could keep the default IP address up to date that way.
>
> Cheers
>

Ideally the code would go inside of net/ as part of the HostResolverImpl.

If that is not possible, then we can add a delegate interface for
retrieving the current system's IP address to HostResolver, and dependency
inject it into the HostResolverImpl.

The location where the HostResolverImpl is constructed is:

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/io_...

So that is where the ChromeOS specific delegate would be passed in.

If that approach sounds like it would work for you, but you are not
comfortable making the change, we can find someone on network team to set
that interface up, and then punt back to you to implement the concrete
chromeos implementation.

Cheers, thanks for working on this!


>
>
> https://codereview.chromium.org/238433003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[stevenjb, 2014-04-17 00:02:40]

OK, on the Chrome/ChromeOS side of things, after a few fumbled efforts, I
realized that we really need a dedicated class in src/chrome to communicate this
between src/chromeos and src/net using the correct threads (despite the existing
barrage of observers and notifiers). I think I may have another use for the new
class in the near future.

I am less confident about putting my_ip_address_ directly in HostResolver, but
it wasn't clear where else it belongs, and adding a delegate seemed like
overkill. Suggestions welcome.

[stevenjb, 2014-04-17 00:02:50]

(PTAL)

[stevenjb, 2014-04-21 19:25:23]

ping

[eroman, 2014-04-21 22:54:47]

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

https://codereview.chromium.org/238433003/diff/20001/net/proxy/proxy_resolver...
net/proxy/proxy_resolver_v8_tracing.cc:930: if (op == MY_IP_ADDRESS || op ==
MY_IP_ADDRESS_EX) {
There is a difference between MY_IP_ADDRESS and MY_IP_ADDRESS_EX -- the former
is supposed to return ONLY ipv4 addresses. Whereas the latter can return IPv6
addresses. What is the IP address string that chrome os is setting?

https://codereview.chromium.org/238433003/diff/20001/net/proxy/proxy_resolver...
net/proxy/proxy_resolver_v8_tracing.cc:933: std::string my_ip_address =
host_resolver()->my_ip_address();
This is not correct. The HostResolver should only be operated on the IO thread,
however here it is running on a worker thread (see the assertion at the top of
this function).

You should probably be doing this as part of DoDnsOperation() or as part of the
HostResolver itself.

[stevenjb, 2014-04-22 22:32:55]

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

https://codereview.chromium.org/238433003/diff/20001/net/proxy/proxy_resolver...
net/proxy/proxy_resolver_v8_tracing.cc:930: if (op == MY_IP_ADDRESS || op ==
MY_IP_ADDRESS_EX) {
On 2014/04/21 22:54:47, eroman wrote:
> There is a difference between MY_IP_ADDRESS and MY_IP_ADDRESS_EX -- the former
> is supposed to return ONLY ipv4 addresses. Whereas the latter can return IPv6
> addresses. What is the IP address string that chrome os is setting?
Currently we only have ipv4 address available. I will add a comment and a link
to a bug tracking ipv6 support.

https://codereview.chromium.org/238433003/diff/20001/net/proxy/proxy_resolver...
net/proxy/proxy_resolver_v8_tracing.cc:933: std::string my_ip_address =
host_resolver()->my_ip_address();
On 2014/04/21 22:54:47, eroman wrote:
> This is not correct. The HostResolver should only be operated on the IO
thread,
> however here it is running on a worker thread (see the assertion at the top of
> this function).
> 
> You should probably be doing this as part of DoDnsOperation() or as part of
the
> HostResolver itself.

Ah, you are right. I will move this to DoDnsOperation() as you suggest. I
started doing that originally, but thought this would be simpler, not
considering the threading concerns.

[stevenjb, 2014-04-23 18:25:35]

PTAL

It wasn't entirely clear to me whether MY_IP_ADDRESS_EX wants just the ipv6
address (if available) or both (I implemented with both, please let me know if I
should change that).

Also note, currently Chrome OS does not supply an ipv6 address, but there is now
an issue tracking that (see comments).

[stevenjb, 2014-04-28 18:29:06]

ping

[eroman, 2014-05-01 22:56:48]

Looking again, apologies for delay!

[eroman, 2014-05-02 01:48:17]

https://codereview.chromium.org/238433003/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/net/chrome_network_watcher.cc (right):

https://codereview.chromium.org/238433003/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/net/chrome_network_watcher.cc:23:
g_browser_process->io_thread()->globals()->host_resolver->SetMyIpAddresses(
This is not correct: g_browser_process is non-threadsafe and bound to the UI
thread.

In which case g_browser_process->io_thread() will give an assertion failure in
debug builds.

I presume the guy who calls PostTask for this function runs on the UI thread? in
that case pull out IO thread at that point.

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

https://codereview.chromium.org/238433003/diff/60001/net/dns/host_resolver.h#...
net/dns/host_resolver.h:187: bool ResolveFromMyIpAddress(bool include_ipv6,
I am still unhappy with this design. Adding string overrides that are set
externally, and then tested by calling this function is subtle. The name of this
function sounds like it will get you the current IPs, however it is really just
getting the "override" ones.

My view for the "right way" of layering this is to add a new virtual method to
this interface such as:

  virtual int ResolveMyIpAddress(AddressFamily address_family, RequestPriority
priority, AddressList* addresses, const CompletionCallback callback,
RequestHandle* out_req, const BoundNetLog& net_log) = 0;

Which essentially mirrors how Resolve() works, but instead of a RequestInfo
parameter it takes a simplified AddressFamily.

The default implementation of ResolveMyIpAddress() can simply forward requests
to Resolve(), by constructing a RequestInfo() using gethostname().

I would accept the patch if that default implementation lived in HostResolver
itself along with a method to provide a static override. However I would prefer
if instead on the chrome side of things, for ChromeOS when instantiating the
"global host resovler"
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/io_...,
it wrapped this resolver with some sort of ChromeOSHostResolver which overrides
ResolveMyIpAddress() and provides that synchronously using the other hooks you
added.

Thanks for working on this, I know it is important! Apologies for not being
faster on these reviews. Please ping me over IM in the future if I don't respond
quickly :)

https://codereview.chromium.org/238433003/diff/60001/net/dns/host_resolver.h#...
net/dns/host_resolver.h:204: std::string my_ipv4_address_;
Please store these instead as IpAddressNumber

[stevenjb, 2014-05-02 16:56:12]

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

https://codereview.chromium.org/238433003/diff/60001/net/dns/host_resolver.h#...
net/dns/host_resolver.h:187: bool ResolveFromMyIpAddress(bool include_ipv6,
On 2014/05/02 01:48:18, eroman wrote:
> I am still unhappy with this design. Adding string overrides that are set
> externally, and then tested by calling this function is subtle. The name of
this
> function sounds like it will get you the current IPs, however it is really
just
> getting the "override" ones.
> 
> My view for the "right way" of layering this is to add a new virtual method to
> this interface such as:
> 
>   virtual int ResolveMyIpAddress(AddressFamily address_family, RequestPriority
> priority, AddressList* addresses, const CompletionCallback callback,
> RequestHandle* out_req, const BoundNetLog& net_log) = 0;
> 
> Which essentially mirrors how Resolve() works, but instead of a RequestInfo
> parameter it takes a simplified AddressFamily.
> 
> The default implementation of ResolveMyIpAddress() can simply forward requests
> to Resolve(), by constructing a RequestInfo() using gethostname().
> 
> I would accept the patch if that default implementation lived in HostResolver
> itself along with a method to provide a static override. However I would
prefer
> if instead on the chrome side of things, for ChromeOS when instantiating the
> "global host resovler"
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/io_...,
> it wrapped this resolver with some sort of ChromeOSHostResolver which
overrides
> ResolveMyIpAddress() and provides that synchronously using the other hooks you
> added.
> 
> Thanks for working on this, I know it is important! Apologies for not being
> faster on these reviews. Please ping me over IM in the future if I don't
respond
> quickly :)

I could certainly make the name more clear and would be happy to do that.

WRT using a virtual method, while I understand the perspective, I avoided that
specifically because I was concerned about adding complexity to code that I
still only peripherally understand.

I think I need to put this on ice for a while until I have more time to wrap my
head around it. If you know anyone willing to pick it up, I will be more than
happy to review the chromeos side of things :)

[stevenjb, 2014-06-03 23:32:31]

OK, this is pretty much a completely new implementation, which should hopefully
be a significant improvement. I'm still working on a unit test, but I've pretty
thoroughly tested the changes on a device already.

PTAL.

[eroman, 2014-06-03 23:45:30]

Thanks Steven, I will review it tomorrow!

[stevenjb, 2014-06-05 15:54:07]

Friendly ping :)

[eroman, 2014-06-05 22:58:24]

Thanks, good improvement over earlier version!

Basically L G T M  after some comments.

https://codereview.chromium.org/238433003/diff/140001/chromeos/network/host_r...
File chromeos/network/host_resolver_impl_chromeos.cc (right):

https://codereview.chromium.org/238433003/diff/140001/chromeos/network/host_r...
chromeos/network/host_resolver_impl_chromeos.cc:129: ProcTaskParams(NULL,
options.max_retry_attempts),
Can you use composition rather than inheritance, to avoid duplicating these
construction details, and needing to expose the dispatcher limits?

i.e. HostResolveImplChromeOS implements HostResolver, and owns a HostResolver
passed into it (which will likely have been constructed via
net::HostResolver::CreateSystemResolver().

https://codereview.chromium.org/238433003/diff/140001/chromeos/network/host_r...
chromeos/network/host_resolver_impl_chromeos.cc:167:
ui_message_loop_->DeleteSoon(
Note that this pattern can cause leaks during shutdown.

(1) browser shutdown starts, stopping UI message loop
(2) IO thread is stopped and joined
(3) Host resolver is destroyed from IO thread
(4) A task is posted to the UI loop to delete network_observer_ --> This doesn't
run.

https://codereview.chromium.org/238433003/diff/140001/chromeos/network/host_r...
chromeos/network/host_resolver_impl_chromeos.cc:194: if (info.hostname() !=
GetHostName() || !network_observer_)
Is there going to be any other consequences of this change?

GetHostName() will be 127.0.0.1 right? So if a user was navigating to
http://127.0.0.1/ directly, that would now have a different meaning then before,
where an AddressList of 127.0.0.1 would have been used.

The alternative would be to have some indication in info on whether localhost is
requested.

https://codereview.chromium.org/238433003/diff/140001/chromeos/network/host_r...
File chromeos/network/host_resolver_impl_chromeos.h (right):

https://codereview.chromium.org/238433003/diff/140001/chromeos/network/host_r...
chromeos/network/host_resolver_impl_chromeos.h:21: namespace net {
Is this a typical namespace for chromeos/network code?

My expectation for this would be to be in the namespace:
  chromeos::

or alternately
  chromeos::net::

I thought net:: was reserved for code in src/net/*

[stevenjb, 2014-06-06 16:06:04]

https://codereview.chromium.org/238433003/diff/140001/chromeos/network/host_r...
File chromeos/network/host_resolver_impl_chromeos.cc (right):

https://codereview.chromium.org/238433003/diff/140001/chromeos/network/host_r...
chromeos/network/host_resolver_impl_chromeos.cc:129: ProcTaskParams(NULL,
options.max_retry_attempts),
On 2014/06/05 22:58:24, eroman wrote:
> Can you use composition rather than inheritance, to avoid duplicating these
> construction details, and needing to expose the dispatcher limits?
> 
> i.e. HostResolveImplChromeOS implements HostResolver, and owns a HostResolver
> passed into it (which will likely have been constructed via
> net::HostResolver::CreateSystemResolver().

The problem with that is that HostResolveImplChromeOS would have to implement
all eight HostResolver virtual methods (all but one of which would just be
forwarded to the owned HostResolverImpl).

While I agree that this is a bit clunky, I'd rather make a TODO to clean up the
HostResolver construction methods which I think could simplify this a lot (but I
didn't particularly want to stomp around in that code for this CL).

https://codereview.chromium.org/238433003/diff/140001/chromeos/network/host_r...
chromeos/network/host_resolver_impl_chromeos.cc:167:
ui_message_loop_->DeleteSoon(
On 2014/06/05 22:58:24, eroman wrote:
> Note that this pattern can cause leaks during shutdown.
> 
> (1) browser shutdown starts, stopping UI message loop
> (2) IO thread is stopped and joined
> (3) Host resolver is destroyed from IO thread
> (4) A task is posted to the UI loop to delete network_observer_ --> This
doesn't
> run.

Ugh, I didn't realize that IOThread::globals_ was getting destroyed after the
message loops were stopped. Nice catch. This is going to have to be a bit more
complicated in order to be robust, I think I need to add a Shutdown message from
NetworkHandler, but that seems worthwhile anyway. Will fix.

https://codereview.chromium.org/238433003/diff/140001/chromeos/network/host_r...
chromeos/network/host_resolver_impl_chromeos.cc:194: if (info.hostname() !=
GetHostName() || !network_observer_)
On 2014/06/05 22:58:24, eroman wrote:
> Is there going to be any other consequences of this change?
> 
> GetHostName() will be 127.0.0.1 right? So if a user was navigating to
> http://127.0.0.1/ directly, that would now have a different meaning then
before,
> where an AddressList of 127.0.0.1 would have been used.
> 
> The alternative would be to have some indication in info on whether localhost
is
> requested.

I'm not familiar enough with network internals to know what the right thing to
do here is. I am happy to add the boolean if that is more correct. Who would
know?

https://codereview.chromium.org/238433003/diff/140001/chromeos/network/host_r...
File chromeos/network/host_resolver_impl_chromeos.h (right):

https://codereview.chromium.org/238433003/diff/140001/chromeos/network/host_r...
chromeos/network/host_resolver_impl_chromeos.h:21: namespace net {
On 2014/06/05 22:58:24, eroman wrote:
> Is this a typical namespace for chromeos/network code?
> 
> My expectation for this would be to be in the namespace:
>   chromeos::
> 
> or alternately
>   chromeos::net::
> 
> I thought net:: was reserved for code in src/net/*

Yes, it should be in chromeos::. I did this initially for convenience, but
should really just put the net:: everywhere, will do.

[stevenjb, 2014-06-06 17:24:18]

https://codereview.chromium.org/238433003/diff/140001/chromeos/network/host_r...
File chromeos/network/host_resolver_impl_chromeos.cc (right):

https://codereview.chromium.org/238433003/diff/140001/chromeos/network/host_r...
chromeos/network/host_resolver_impl_chromeos.cc:167:
ui_message_loop_->DeleteSoon(
On 2014/06/06 16:06:03, stevenjb wrote:
> On 2014/06/05 22:58:24, eroman wrote:
> > Note that this pattern can cause leaks during shutdown.
> > 
> > (1) browser shutdown starts, stopping UI message loop
> > (2) IO thread is stopped and joined
> > (3) Host resolver is destroyed from IO thread
> > (4) A task is posted to the UI loop to delete network_observer_ --> This
> doesn't
> > run.
> 
> Ugh, I didn't realize that IOThread::globals_ was getting destroyed after the
> message loops were stopped. Nice catch. This is going to have to be a bit more
> complicated in order to be robust, I think I need to add a Shutdown message
from
> NetworkHandler, but that seems worthwhile anyway. Will fix.
> 
I realized quickly that a Shutdown message after the MessageLoop is stopped is
not going to be very helpful :)

It turns out that DeleteSoon returns true or false, so I think the simplest /
safest thing to do is to delete network_observer_ here if DeleteSoon returns
false. If the message loops are stopped, that will be safe.

[stevenjb, 2014-06-06 17:34:11]

https://codereview.chromium.org/238433003/diff/140001/chromeos/network/host_r...
File chromeos/network/host_resolver_impl_chromeos.cc (right):

https://codereview.chromium.org/238433003/diff/140001/chromeos/network/host_r...
chromeos/network/host_resolver_impl_chromeos.cc:194: if (info.hostname() !=
GetHostName() || !network_observer_)
On 2014/06/06 16:06:03, stevenjb wrote:
> On 2014/06/05 22:58:24, eroman wrote:
> > Is there going to be any other consequences of this change?
> > 
> > GetHostName() will be 127.0.0.1 right? So if a user was navigating to
> > http://127.0.0.1/ directly, that would now have a different meaning then
> before,
> > where an AddressList of 127.0.0.1 would have been used.
> > 
> > The alternative would be to have some indication in info on whether
localhost
> is
> > requested.
> 
> I'm not familiar enough with network internals to know what the right thing to
> do here is. I am happy to add the boolean if that is more correct. Who would
> know?
> 

So, on systems other than Chrome OS, isn't a localhost lookup going to resolve
to the actual IP address? I thought that was the whole point of this; I don't
see any special handling of the request in ProxyResolverV8Tracing other than
host_port.set_host(GetHostName()).

[stevenjb, 2014-06-06 23:06:24]

PTAL

(Note: asan failures are not specific to this CL)

[eroman, 2014-06-09 22:37:11]

https://codereview.chromium.org/238433003/diff/220001/chromeos/network/host_r...
File chromeos/network/host_resolver_impl_chromeos.cc (right):

https://codereview.chromium.org/238433003/diff/220001/chromeos/network/host_r...
chromeos/network/host_resolver_impl_chromeos.cc:115: // construction to reduce
duplicate code.
I still think the right abstraction is to compose. There are 8 virtual methods
that need to be forwarded, but the code to do so is very straight forward.

I am concerned that duplicating the construction parameters for HostResolverImpl
is a mistake waiting to happen. Someone might refactor HostResolverImpl and not
know to update HostResovlerImplChromeOS causing it to now behave differently.
Rather than have to rely on someone doing the right thing, I prefer if we are
using HostResolverImpl directly.

https://codereview.chromium.org/238433003/diff/220001/chromeos/network/host_r...
chromeos/network/host_resolver_impl_chromeos.cc:169: delete network_observer;
I don't think this is correct. Deleting network_observer will cause
NetworkStateHandler::RemoveObserver() to be called, which calls into a
non-thread-safe observer list, from potentially the wrong thread.

Not sure how to best handle this. A destruction observer on
network_handler_message_loop_ could work (although gross). Or maybe add an
explicit shutdown method from which it can de-register.

What is the network_handler_message_loop anyway? Is it the UI thread?

[stevenjb, 2014-06-10 22:04:55]

PTAL

https://codereview.chromium.org/238433003/diff/220001/chromeos/network/host_r...
File chromeos/network/host_resolver_impl_chromeos.cc (right):

https://codereview.chromium.org/238433003/diff/220001/chromeos/network/host_r...
chromeos/network/host_resolver_impl_chromeos.cc:115: // construction to reduce
duplicate code.
On 2014/06/09 22:37:11, eroman wrote:
> I still think the right abstraction is to compose. There are 8 virtual methods
> that need to be forwarded, but the code to do so is very straight forward.
> 
> I am concerned that duplicating the construction parameters for
HostResolverImpl
> is a mistake waiting to happen. Someone might refactor HostResolverImpl and
not
> know to update HostResovlerImplChromeOS causing it to now behave differently.
> Rather than have to rely on someone doing the right thing, I prefer if we are
> using HostResolverImpl directly.

While I understand your concern, I think composition here would be a mistake. If
a virtual method were added to HostResolver with a default implementation (it is
already not a pure interface), but with a different implementation in
HostResolverImpl, this class would compile but do the wrong thing (calling the
default implementation instead of forwarding).

I would rather attempt to re-factor the constructor instead of using composition
here, I will take a pass at that and see what you think.

https://codereview.chromium.org/238433003/diff/220001/chromeos/network/host_r...
chromeos/network/host_resolver_impl_chromeos.cc:169: delete network_observer;
On 2014/06/09 22:37:11, eroman wrote:
> I don't think this is correct. Deleting network_observer will cause
> NetworkStateHandler::RemoveObserver() to be called, which calls into a
> non-thread-safe observer list, from potentially the wrong thread.
> 
> Not sure how to best handle this. A destruction observer on
> network_handler_message_loop_ could work (although gross). Or maybe add an
> explicit shutdown method from which it can de-register.
> 
> What is the network_handler_message_loop anyway? Is it the UI thread?

This will only occur if the message loops were no longer running, in which case
it should be safe to remove the observer across thread boundaries, but I agree
it's not a great solution.

I considered a message loop destruction observer, but I also thought it seemed
pretty ugly and awkward.

I guess the best thing to do is to add an explicit Shutdown() method for this
class. I can add it to the interface and have the default implementation do
nothing (other wise we would have to do an ugly static cast).

Yes, network_handler_message_loop is the UI thread, but I didn't think there was
any reason this class should need to know that.

[eroman, 2014-06-12 04:13:37]

https://codereview.chromium.org/238433003/diff/220001/chromeos/network/host_r...
File chromeos/network/host_resolver_impl_chromeos.cc (right):

https://codereview.chromium.org/238433003/diff/220001/chromeos/network/host_r...
chromeos/network/host_resolver_impl_chromeos.cc:169: delete network_observer;
On 2014/06/10 22:04:55, stevenjb wrote:
> On 2014/06/09 22:37:11, eroman wrote:
> > I don't think this is correct. Deleting network_observer will cause
> > NetworkStateHandler::RemoveObserver() to be called, which calls into a
> > non-thread-safe observer list, from potentially the wrong thread.
> > 
> > Not sure how to best handle this. A destruction observer on
> > network_handler_message_loop_ could work (although gross). Or maybe add an
> > explicit shutdown method from which it can de-register.
> > 
> > What is the network_handler_message_loop anyway? Is it the UI thread?
> 
> This will only occur if the message loops were no longer running, in which
case
> it should be safe to remove the observer across thread boundaries, but I agree
> it's not a great solution.
> 
> I considered a message loop destruction observer, but I also thought it seemed
> pretty ugly and awkward.
> 
> I guess the best thing to do is to add an explicit Shutdown() method for this
> class. I can add it to the interface and have the default implementation do
> nothing (other wise we would have to do an ugly static cast).
> 
> Yes, network_handler_message_loop is the UI thread, but I didn't think there
was
> any reason this class should need to know that.

This is still a problem, let me lay out my concerns in some more detail:

(A) Since the network handler message loop is the UI message loop, this "delete"
statement is not reachable. The only time DeleteSoon() returns false is if the
underlying message loop was already deleted. And the UI message loop will always
outlive the IO thread so this will not happen.

(B) Even if it were possible for DeleteSoon() to return false, it is still
incorrect to delete the observer here, since it causes a non-threadsafe observer
list (owned by NetworkHandler) to be entered. Imagine if other code was doing
the same from another thread and deciding it is safe to remove observers because
the message loop that networkhandler runs on is gone -- there would now be a
race inside that observerlist. Relying on no other code doing this is not safe.
(That said, this is entirely theoretical since we know this can't happen).

(C) The leak that happens during shutdown should be easy to reproduce. Here is
how it would happen (I have had to debug similar things in the past so I can
tell from experience why this is an anti-pattern):

  (1) The UI's run loop finishes running, starting shutdown
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/br...

  (2) The IO thread is stopped.
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/br...

  (3) As part of the IO thread's destruction, your new destructor
~HostResolverImplChromeOS() is invoked.

  (4) The IO thread posts a Delete task to the IO thread
   ====> THIS TASK IS NOT GOING TO BE RUN
   The UI message loop already stopped running in step (1). Any new tasks enqued
to it will simply be deleted by step (5). Moreover, the way that DeleteSoon() is
implemented, the object is only deleted if the task is run, and not when
destroying the callback.
(https://code.google.com/p/chromium/codesearch#chromium/src/base/message_loop/...)

  (5) Back on the UI thread, once the IO thread has finished running the main
message loop is deleted. Deleting the message loop does not run the tasks, it
simply deletes them
(https://code.google.com/p/chromium/codesearch#chromium/src/base/message_loop/...).
Deleting the DeleteSoon task causes the object to be leaked.

============================

OK, so if we agree on the problem, then there are a couple of solutions I can
suggest:

  (A) Make NetworkStateHandler use a ObserverListThreadSafe rather than an
ObserverList. You could now register/unregister from the calling thread and not
have to bounce things around with the ::NetworkObserver subclass.

  (B) Make your own version of DeleteSoon() which actually owns the object, for
instance:
         PostTask(Bind(&NoOpFunction, Owned(network_obsever));
  The advantage of this incantation is that network_observer is now deleted when
the underlying Callback is deleted, so even if it isn't run it will still delete
network_observer. The iffy part about this solution, is if the PostTask were to
return false for some reason, the Callback will be deleted from the wrong thread
(the current one).

Unfortunately these kinds of dependencies between IO and UI thread are pretty
gnarly when considering shutdown. Don't mean to be annoying on this issue, it is
just that I have personally debugged some subtle bugs that can result from it,
and wouldn't wish it on anyone else :)

[eroman, 2014-06-12 04:18:23]

To be clear, the final suggestions A and B are exclusive - either of them should
be sufficient to address the issue.

[stevenjb, 2014-06-12 19:19:20]

On 2014/06/12 04:18:23, eroman wrote:
> To be clear, the final suggestions A and B are exclusive - either of them
should
> be sufficient to address the issue.

I do get all that, and I appreciate the careful review. I misunderstood the
use/purpose of DeleteSoon.

I don't want to do (A) because it is not safe to call
NetworkStateHandlerObserver methods on non-UI threads and enforcing/documenting
that would become complicated.

(B) is clever but a bit subtle for my taste.

Instead I am going to make NetworkObserver unowned and add an IsShuttingDown
method to NetworkStateHandlerObserver so that NetworkObserver can safely delete
itself on the UI thread. I believe that will be safe and robust in all
scenarios.

PTAL

[stevenjb, 2014-06-16 21:58:47]

Ping :)

[eroman, 2014-06-16 22:37:48]

LGTM.

I didn't read the unittest refactors too carefully, but the rest looks good.

https://codereview.chromium.org/238433003/diff/300001/chromeos/network/host_r...
File chromeos/network/host_resolver_impl_chromeos.cc (right):

https://codereview.chromium.org/238433003/diff/300001/chromeos/network/host_r...
chromeos/network/host_resolver_impl_chromeos.cc:39: : owner_(owner),
nit: "owner" and "owner_message_loop" aren't the best names anymore, since it is
now owned by the NetworkStateHandler. I might suggest "parent" and
"parent_message_loop" instead.

https://codereview.chromium.org/238433003/diff/300001/chromeos/network/host_r...
File chromeos/network/host_resolver_impl_chromeos.h (right):

https://codereview.chromium.org/238433003/diff/300001/chromeos/network/host_r...
chromeos/network/host_resolver_impl_chromeos.h:53: void SetIPAddresses(const
std::string& ipv4_address,
I believe this can be private, since NetworkObserver is an inner class and can
therefore access private members of HostResolverImplChromeOS.

[stevenjb, 2014-06-16 23:03:53]

Thanks a ton for the reviews!

TBR: sky@ for relatively minor changes to io_thread.cc.

https://codereview.chromium.org/238433003/diff/300001/chromeos/network/host_r...
File chromeos/network/host_resolver_impl_chromeos.cc (right):

https://codereview.chromium.org/238433003/diff/300001/chromeos/network/host_r...
chromeos/network/host_resolver_impl_chromeos.cc:39: : owner_(owner),
On 2014/06/16 22:37:48, eroman wrote:
> nit: "owner" and "owner_message_loop" aren't the best names anymore, since it
is
> now owned by the NetworkStateHandler. I might suggest "parent" and
> "parent_message_loop" instead.

Done. (owner -> resolver)

https://codereview.chromium.org/238433003/diff/300001/chromeos/network/host_r...
File chromeos/network/host_resolver_impl_chromeos.h (right):

https://codereview.chromium.org/238433003/diff/300001/chromeos/network/host_r...
chromeos/network/host_resolver_impl_chromeos.h:53: void SetIPAddresses(const
std::string& ipv4_address,
On 2014/06/16 22:37:48, eroman wrote:
> I believe this can be private, since NetworkObserver is an inner class and can
> therefore access private members of HostResolverImplChromeOS.

Done.

[stevenjb, 2014-06-16 23:04:59]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2014-06-16 23:07:48]

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

[commit-bot: I haz the power, 2014-06-17 09:21:06]

Change committed as 277679