I expect this will end up being very similar to the NetworkChangeNotifier work in crbug.com/821009 and  crbug.com/882610 . ericorth@ is working on mojoifying HostResolver (crbug.com/821021), which will basically expose the HostResolver functionality as a mojo interface in the network service.

Until that's done, we'll have to have a HostResolver instance running in both the network and browser processes. This will be problematic on ChromeOS because HostResolver needs to know the current ip address, which you get from Shill, but only the browser process can listen to changes from Shill.

I'm currently planning on refactoring HostResolverImplChromeos in the same way I refactored NetworkChangeNotifierChromeos in crrev.com/c/1274445. The basic idea would be to make HostResolverImplChromeos expose a SetIpAddresses method (or something like that), and have a separate class in the browser process that listens to Shill, calls HostResolverImplChromeos::SetIpAddresses on the browser's instance, and notifies the network service of the change via the NetworkChangeManager. The network service can then notify its instance of the class.

We can simplify this longer term once there's a Shill mojo interface (crbug.com/862420), since then the HostResolverImplChromeos will be able to listen for network changes directly.

To provide some more background on this change, the only reason HostResolverImplChromeOS exists is because  crbug.com/175652 , where we wanted myIpAddress() to return the real IP address rather than "localhost" in PAC scripts (note that this is different than Firefox's behavior according to MDN - https://developer.mozilla.org/en-US/docs/Web/HTTP/Proxy_servers_and_tunneling/Proxy_Auto-Configuration_(PAC)_file#myIpAddress()_2).

While looking into this I discovered we have no test coverage beyond unit tests that myIpAddress actually returns the correct value, so I'll try to write a browser test for that first.

If that's the only reason we have the separate HostResolverImplChromeOS, I wonder if it makes more sense to integrate the SetIpAddress() functionality directly into the normal HostResolverImpl instead of continuing to use a separate impl class.  Simplifies the network service code if it doesn't have to worry about the internal HostResolver sometimes having that method available and sometimes not.

ChromeOS browser process, on detecting the change via Shill, would make some SetIpAddress() mojo call.  Network service then calls it on its internal resolver(s).  Internal HostResolverImpl then provides the relevant behavior.

Non-ChromeOS browser processes, wouldn't ever call the SetIpAddress() mojo call, so it would never get called on the internal resolver, and the internal resolver would just provide the normal default behavior.

If you're ok with that then yes, I think that would be much better. I assume the SetIpAddress call will eventually live in the new HostResolver mojo interface you're working on? In the meantime, assuming that's not ready, we can put it in NetworkChangeManager.

My HostResolver mojo interface (see host_resolver.mojom) is already "done" and in use other than support for some of the more obscure cornercases, so I think it's ready enough to support the SetIpAddress behavior.

But since local IP address is more of a global thing and HostResolver instances can be separate for different DNS configurations, permissions, etc, I don't think it makes sense for the SetIpAddress() method to be directly on the HostResolver mojo interface.  We would want setting it to apply to any internal resolvers, not just the one you had the mojo interface for.  So I think NetworkChangeManager or maybe directly on NetworkService or NetworkContext makes more sense.  It could then be passed on to all the internal resolvers.

I think the cleanest solution would be to remove HostResolverImplChromeOS all together, and then implement  Issue 735637 .

For  Issue 735637 , connecting a UDP socket and reading back the source address should be good enough to address the PAC use case in a cross-platform manner, and then we won't need any Chrome OS specific code.

Discussed a bit off-thread: I will give 735637 a go this week. If that is successful then we can nuke this ChromeOS code and not have to port it.

Otherwise, I will toss this bug back :)

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7193c9faf699e4fbb3e713b6f9d1adea59680a0e

commit 7193c9faf699e4fbb3e713b6f9d1adea59680a0e
Author: Eric Roman <eroman@chromium.org>
Date: Thu Nov 22 01:39:58 2018

Re-implement myIpAddress() and myIpAddressEx().

The previous implementation was effectively:
  getaddrinfo(gethostname())

The new approach uses a similar strategy to Firefox, by first prioritizing the default local address used for connecting to the internet, followed by various fallbacks including testing routes to private IPs.

This also deletes the ChromeOS specific implementation, as the generic one addresses the original problem it was fixing.

Bug:  735637 ,  827533 
Change-Id: I31a5d1a58af48398afad787aefc1edd6cc4510ac
Reviewed-on: https://chromium-review.googlesource.com/c/1336063
Commit-Queue: Eric Roman <eroman@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#610278}
[modify] https://crrev.com/7193c9faf699e4fbb3e713b6f9d1adea59680a0e/chrome/browser/io_thread.cc
[modify] https://crrev.com/7193c9faf699e4fbb3e713b6f9d1adea59680a0e/chrome/browser/net/proxy_browsertest.cc
[add] https://crrev.com/7193c9faf699e4fbb3e713b6f9d1adea59680a0e/chrome/test/data/my_ip_address.pac
[modify] https://crrev.com/7193c9faf699e4fbb3e713b6f9d1adea59680a0e/chromeos/network/BUILD.gn
[delete] https://crrev.com/cb59771be7f119f04b1a3b4fe2ed55be724fecb5/chromeos/network/host_resolver_impl_chromeos.cc
[delete] https://crrev.com/cb59771be7f119f04b1a3b4fe2ed55be724fecb5/chromeos/network/host_resolver_impl_chromeos.h
[delete] https://crrev.com/cb59771be7f119f04b1a3b4fe2ed55be724fecb5/chromeos/network/host_resolver_impl_chromeos_unittest.cc
[modify] https://crrev.com/7193c9faf699e4fbb3e713b6f9d1adea59680a0e/net/BUILD.gn
[modify] https://crrev.com/7193c9faf699e4fbb3e713b6f9d1adea59680a0e/net/base/ip_address.cc
[modify] https://crrev.com/7193c9faf699e4fbb3e713b6f9d1adea59680a0e/net/base/ip_address.h
[modify] https://crrev.com/7193c9faf699e4fbb3e713b6f9d1adea59680a0e/net/dns/BUILD.gn
[modify] https://crrev.com/7193c9faf699e4fbb3e713b6f9d1adea59680a0e/net/dns/host_resolver.h
[add] https://crrev.com/7193c9faf699e4fbb3e713b6f9d1adea59680a0e/net/proxy_resolution/pac_library.cc
[add] https://crrev.com/7193c9faf699e4fbb3e713b6f9d1adea59680a0e/net/proxy_resolution/pac_library.h
[add] https://crrev.com/7193c9faf699e4fbb3e713b6f9d1adea59680a0e/net/proxy_resolution/pac_library_unittest.cc
[modify] https://crrev.com/7193c9faf699e4fbb3e713b6f9d1adea59680a0e/net/proxy_resolution/proxy_bypass_rules.cc
[modify] https://crrev.com/7193c9faf699e4fbb3e713b6f9d1adea59680a0e/net/proxy_resolution/proxy_resolver_v8.cc
[modify] https://crrev.com/7193c9faf699e4fbb3e713b6f9d1adea59680a0e/net/proxy_resolution/proxy_resolver_v8_tracing.cc
[modify] https://crrev.com/7193c9faf699e4fbb3e713b6f9d1adea59680a0e/services/network/proxy_resolver_factory_mojo.cc

HostResolverImplChromeOS is gone, so nothing left to port :)