[net/dns] Isolate DnsConfigWatcher from DnsConfigService.

[net/dns] Isolate DnsConfigWatcher from DnsConfigService.

DnsConfigWatcher is installed at NetworkChangeNotifier and provides signals to DNSObservers.
DnsConfigService becomes a DNSObserver.

BUG=114827, 114223, 128166
TEST=./net_unittests --gtest_filter=DnsConfigService*

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

[szym, 2012-05-11 19:03:16]

mmenke: PTAL.
eroman: FYI.

This is now ready for review. I decided to keep the code in net/dns rather than
cramming it into NetworkChangeNotifier. Coincidentally, having a dedicated
thread allows for removal of ScopedAllowIO.
I'm still not clear when and how often could a watch fail. For
--enable-async-dns it was non-essential, but maybe it's time to measure that or
default to crash.

[mmenke, 2012-05-14 18:40:01]

http://codereview.chromium.org/10377092/diff/10032/net/dns/dns_config_service.cc
File net/dns/dns_config_service.cc (right):

http://codereview.chromium.org/10377092/diff/10032/net/dns/dns_config_service...
net/dns/dns_config_service.cc:85: void DnsConfigService::Read(const
CallbackType& callback) {
If we're going to keep this function, you should have some DCHECKs that Read and
Watch are not used on the same DnsConfigService.

http://codereview.chromium.org/10377092/diff/10032/net/dns/dns_config_service...
net/dns/dns_config_service.cc:93: void DnsConfigService::Watch(const
CallbackType& callback) {
In practice, do we expect to call this more than once?  If we recover from a
watch failing, it may make sense to do all that at the ConfigWatcher level,
right?  Or is there some other signal we have here that we don't have there?

http://codereview.chromium.org/10377092/diff/10032/net/dns/dns_config_service.h
File net/dns/dns_config_service.h (right):

http://codereview.chromium.org/10377092/diff/10032/net/dns/dns_config_service...
net/dns/dns_config_service.h:83: public NetworkChangeNotifier::DNSObserver {
nit:  Fix indent.

http://codereview.chromium.org/10377092/diff/10032/net/dns/dns_config_service...
net/dns/dns_config_service.h:96: virtual void Read(const CallbackType&
callback);
The dual-use nature of this class strikes me as potentially trouble-prone.  Do
we even need Read()?  The comments about their compatibility, or lack thereof,
aren't terribly clear, either.

http://codereview.chromium.org/10377092/diff/10032/net/dns/dns_config_service...
net/dns/dns_config_service.h:101: // unavailable.
I find this comment hard to follow.  I actually think the old comment is a
little clearer.

http://codereview.chromium.org/10377092/diff/10032/net/dns/dns_config_service...
net/dns/dns_config_service.h:102: virtual void Watch(const CallbackType&
callback);
Neither Watch nor Read needs to be virtual.

http://codereview.chromium.org/10377092/diff/10032/net/dns/dns_config_service...
net/dns/dns_config_service.h:107: // Resets state.
Think this is a little vague.  "Clears cached DNSConfig", maybe?

http://codereview.chromium.org/10377092/diff/10032/net/dns/dns_config_watcher.cc
File net/dns/dns_config_watcher.cc (right):

http://codereview.chromium.org/10377092/diff/10032/net/dns/dns_config_watcher...
net/dns/dns_config_watcher.cc:14: if (detail &
NetworkChangeNotifier::CHANGE_DNS_WATCH_STARTED) {
Think it would make a little more sense to have NetworkChangeNotifier do this
itself, and maybe rename "NotifyObserversOfDNSChange" to "OnDnsConfigChanged" or
some such.

http://codereview.chromium.org/10377092/diff/10032/net/dns/dns_test_util.h
File net/dns/dns_test_util.h (right):

http://codereview.chromium.org/10377092/diff/10032/net/dns/dns_test_util.h#ne...
net/dns/dns_test_util.h:178: virtual void OnDNSChanged(unsigned detail) OVERRIDE
{}
Please move all these into the cc file.  The Chrome style guide pretty strongly
discourages inling of functions, beyond getters and setters.

[szym, 2012-05-14 19:31:13]

Thanks for the review. I updated the interface to only allow Read or Watch once.

That means if someone needs to re-read the config on demand, they'll have to
destroy the old service, create a new one and call Read again. I think that's
should be sufficient.

http://codereview.chromium.org/10377092/diff/10032/net/dns/dns_config_service.cc
File net/dns/dns_config_service.cc (right):

http://codereview.chromium.org/10377092/diff/10032/net/dns/dns_config_service...
net/dns/dns_config_service.cc:93: void DnsConfigService::Watch(const
CallbackType& callback) {
On 2012/05/14 18:40:01, Matt Menke wrote:
> In practice, do we expect to call this more than once?  If we recover from a
> watch failing, it may make sense to do all that at the ConfigWatcher level,
> right?  Or is there some other signal we have here that we don't have there?

That's right. I don't expect to call Watch more than once.

http://codereview.chromium.org/10377092/diff/10032/net/dns/dns_config_service.h
File net/dns/dns_config_service.h (right):

http://codereview.chromium.org/10377092/diff/10032/net/dns/dns_config_service...
net/dns/dns_config_service.h:96: virtual void Read(const CallbackType&
callback);
On 2012/05/14 18:40:01, Matt Menke wrote:
> The dual-use nature of this class strikes me as potentially trouble-prone.  Do
> we even need Read()?  The comments about their compatibility, or lack thereof,
> aren't terribly clear, either.

I imagine cases where there's no need to for updates:
- GDig tool: http://codereview.chromium.org/10386120/
- Detecting DNS misconfiguration (DNS Changer).

What concerns me about the Watch() API is that it needs to wait until
DnsConfigWatcher is started by NCN. Read() was an attempt at providing a way to
read DNS even if watching doesn't work, but once again, I'm not sure how likely
the watchers would fail.

http://codereview.chromium.org/10377092/diff/10032/net/dns/dns_config_service...
net/dns/dns_config_service.h:101: // unavailable.
On 2012/05/14 18:40:01, Matt Menke wrote:
> I find this comment hard to follow.  I actually think the old comment is a
> little clearer.

In the old implementation, the callers didn't have a choice but to wait. The
purpose of this (perplexing) comment was to make it easier to decide whether to
use Watch or Read, but I'm okay with just going with Watch.

http://codereview.chromium.org/10377092/diff/10032/net/dns/dns_config_service...
net/dns/dns_config_service.h:102: virtual void Watch(const CallbackType&
callback);
On 2012/05/14 18:40:01, Matt Menke wrote:
> Neither Watch nor Read needs to be virtual.

Good point.

http://codereview.chromium.org/10377092/diff/10032/net/dns/dns_config_watcher.cc
File net/dns/dns_config_watcher.cc (right):

http://codereview.chromium.org/10377092/diff/10032/net/dns/dns_config_watcher...
net/dns/dns_config_watcher.cc:14: if (detail &
NetworkChangeNotifier::CHANGE_DNS_WATCH_STARTED) {
On 2012/05/14 18:40:01, Matt Menke wrote:
> Think it would make a little more sense to have NetworkChangeNotifier do this
> itself, and maybe rename "NotifyObserversOfDNSChange" to "OnDnsConfigChanged"
or
> some such.

SGTM.

[szym, 2012-05-14 19:57:19]

http://codereview.chromium.org/10377092/diff/12032/net/dns/dns_config_service...
File net/dns/dns_config_service_win.cc (right):

http://codereview.chromium.org/10377092/diff/12032/net/dns/dns_config_service...
net/dns/dns_config_service_win.cc:532:
NetworkChangeNotifier::AddIPAddressObserver(this);
Ops. Forgot, that this is why Watch needs to be virtual.

[mmenke, 2012-05-15 15:10:24]

http://codereview.chromium.org/10377092/diff/10032/net/dns/dns_config_service.h
File net/dns/dns_config_service.h (right):

http://codereview.chromium.org/10377092/diff/10032/net/dns/dns_config_service...
net/dns/dns_config_service.h:96: virtual void Read(const CallbackType&
callback);
On 2012/05/14 19:31:13, szym wrote:
> On 2012/05/14 18:40:01, Matt Menke wrote:
> > The dual-use nature of this class strikes me as potentially trouble-prone. 
Do
> > we even need Read()?  The comments about their compatibility, or lack
thereof,
> > aren't terribly clear, either.
> 
> I imagine cases where there's no need to for updates:
> - GDig tool: http://codereview.chromium.org/10386120/
> - Detecting DNS misconfiguration (DNS Changer).
> 
> What concerns me about the Watch() API is that it needs to wait until
> DnsConfigWatcher is started by NCN. Read() was an attempt at providing a way
to
> read DNS even if watching doesn't work, but once again, I'm not sure how
likely
> the watchers would fail.

Sounds reasonable.  My main concern was reuse.

http://codereview.chromium.org/10377092/diff/12032/net/base/network_change_no...
File net/base/network_change_notifier.cc (right):

http://codereview.chromium.org/10377092/diff/12032/net/base/network_change_no...
net/base/network_change_notifier.cc:166:
NetworkChangeNotifier::SetWatchingDNS(true);
Might want to add a DCHECK(!(detail &
NetworkChangeNotifier::CHANGE_DNS_WATCH_FAILED)), to make it clear they're
mutually exclusive flags.  Or not.

http://codereview.chromium.org/10377092/diff/12032/net/dns/dns_config_service...
File net/dns/dns_config_service_win.cc (right):

http://codereview.chromium.org/10377092/diff/12032/net/dns/dns_config_service...
net/dns/dns_config_service_win.cc:532:
NetworkChangeNotifier::AddIPAddressObserver(this);
On 2012/05/14 19:57:20, szym wrote:
> Ops. Forgot, that this is why Watch needs to be virtual.

Ahh, right.

http://codereview.chromium.org/10377092/diff/13030/net/base/network_change_no...
File net/base/network_change_notifier.h (right):

http://codereview.chromium.org/10377092/diff/13030/net/base/network_change_no...
net/base/network_change_notifier.h:159: static void SetWatchingDNS(bool value);
This can now be private

http://codereview.chromium.org/10377092/diff/13030/net/base/network_change_no...
File net/base/network_change_notifier_win.cc (right):

http://codereview.chromium.org/10377092/diff/13030/net/base/network_change_no...
net/base/network_change_notifier_win.cc:29: class
NetworkChangeNotifierWin::DnsWatcherThread : public base::Thread {
Why do we need a dedicated thread for this now?  Didn't we used to just do this
on the IO Thread?

http://codereview.chromium.org/10377092/diff/13030/net/dns/dns_config_watcher...
File net/dns/dns_config_watcher_posix.cc (right):

http://codereview.chromium.org/10377092/diff/13030/net/dns/dns_config_watcher...
net/dns/dns_config_watcher_posix.cc:69: if
(!config_watcher_.Watch(base::Bind(&Core::OnConfigChanged,
I think we should be consistent here.  With this code, if we fail to start
watching the config, we don't start watching the hosts file, but if the config
watcher fails later, we continue to watch the hosts file (And vice versa). 
Think we should either always allow watching just one, or never do it.

My own take is that there is definite value in just watching the config, in
terms of broadcasting notifications, so should just always allow half the
watcher to work, which looks to be what we used to do in the
NetworkChangeNotifier, but not the DnsConfigService (Which may want to just stop
watching a config on CHANGE_DNS_WATCH_FAILED, now).

http://codereview.chromium.org/10377092/diff/13030/net/dns/dns_config_watcher...
net/dns/dns_config_watcher_posix.cc:120:
NetworkChangeNotifier::CHANGE_DNS_WATCH_STARTED);
nit:  Add brackets when the body of an if takes up more than one line.

http://codereview.chromium.org/10377092/diff/13030/net/dns/dns_config_watcher...
File net/dns/dns_config_watcher_win.cc (right):

http://codereview.chromium.org/10377092/diff/13030/net/dns/dns_config_watcher...
net/dns/dns_config_watcher_win.cc:38: L"SOFTWARE\\Policies\\Microsoft\\Windows
NT\\DNSClient";
May want to just include these from dns_config_service_win.  Duplicated strings
make me nervous.  While these aren't exactly likely to change, sharing values
makes it more likely that any update willl be applied in both places they are
used.

Same goes for the GetHostsPath.

http://codereview.chromium.org/10377092/diff/13030/net/dns/dns_config_watcher...
net/dns/dns_config_watcher_win.cc:38: L"SOFTWARE\\Policies\\Microsoft\\Windows
NT\\DNSClient";
Random question for the future:  When a network connection is established/lost,
I expect we'll see a bunch of these change at once.  Should we have some
mechanism for reducing redundant messages?

http://codereview.chromium.org/10377092/diff/13030/net/dns/dns_config_watcher...
net/dns/dns_config_watcher_win.cc:104: if (!tcpip_watcher_.Watch(kTcpipPath,
callback)) {
Think there is now value in having only some of these watchers active.

[szym, 2012-05-15 17:48:51]

http://codereview.chromium.org/10377092/diff/13030/net/base/network_change_no...
File net/base/network_change_notifier_win.cc (right):

http://codereview.chromium.org/10377092/diff/13030/net/base/network_change_no...
net/base/network_change_notifier_win.cc:29: class
NetworkChangeNotifierWin::DnsWatcherThread : public base::Thread {
On 2012/05/15 15:10:24, Matt Menke wrote:
> Why do we need a dedicated thread for this now?  Didn't we used to just do
this
> on the IO Thread?

I just re-read my last comment on http://codereview.chromium.org/9540011/
and although FilePathWatcher does not need TYPE_IO loop on Windows, we need
AssertIOAllowed to open Registry keys and to run FilePathWatcher
(FilePathWatcherImpl::OnObjectSignaled). I just tried removing the thread and
caught the AssertIOAllowed check.

So I'm going to keep the DnsWatcherThread with added comment.

http://codereview.chromium.org/10377092/diff/13030/net/dns/dns_config_watcher...
File net/dns/dns_config_watcher_posix.cc (right):

http://codereview.chromium.org/10377092/diff/13030/net/dns/dns_config_watcher...
net/dns/dns_config_watcher_posix.cc:69: if
(!config_watcher_.Watch(base::Bind(&Core::OnConfigChanged,
On 2012/05/15 15:10:24, Matt Menke wrote:
> Think we should either always allow watching just one, or never do it.
> 
> My own take is that there is definite value in just watching the config, in
> terms of broadcasting notifications, so should just always allow half the
> watcher to work, 

Agreed. If Watch() returns false, the watch will never officially start, so
DnsConfigService will not attempt to fully configure DnsConfig, even if it
subsequently receives partial notification. This means that any work
DnsConfigService does in response to partial notification is wasted. To avoid
this, I added CHANGE_DNS_WATCH_FAILED detail when IsWatchingDNS is false.

http://codereview.chromium.org/10377092/diff/13030/net/dns/dns_config_watcher...
File net/dns/dns_config_watcher_win.cc (right):

http://codereview.chromium.org/10377092/diff/13030/net/dns/dns_config_watcher...
net/dns/dns_config_watcher_win.cc:38: L"SOFTWARE\\Policies\\Microsoft\\Windows
NT\\DNSClient";
On 2012/05/15 15:10:24, Matt Menke wrote:
> Random question for the future:  When a network connection is
established/lost,
> I expect we'll see a bunch of these change at once.  Should we have some
> mechanism for reducing redundant messages?

This question came up when the idea of adding DNS-related watches was suggested
and the conclusion was that to filter out redundant messages, we'd need to read
DNS config and determine actual change. DnsConfigService can be used for that
purpose. Besides, I don't think we'll see that much redundancy. Only kTcpipPath
and kTcpip6Path change on actual network change.

[mmenke, 2012-05-16 16:18:03]

LGTM

https://chromiumcodereview.appspot.com/10377092/diff/13033/net/base/network_c...
File net/base/network_change_notifier.cc (right):

https://chromiumcodereview.appspot.com/10377092/diff/13033/net/base/network_c...
net/base/network_change_notifier.cc:174: detail |=
NetworkChangeNotifier::CHANGE_DNS_WATCH_FAILED;
Suggest a:

DCHECK(!(detail & NetworkChangeNotifier::CHANGE_DNS_WATCH_FAILED) ||
       !(detail & NetworkChangeNotifier::CHANGE_DNS_WATCH_STARTED));

Here as a simple sanity check.

https://chromiumcodereview.appspot.com/10377092/diff/13033/net/dns/dns_config...
File net/dns/dns_config_service_posix.cc (right):

https://chromiumcodereview.appspot.com/10377092/diff/13033/net/dns/dns_config...
net/dns/dns_config_service_posix.cc:99: void
DnsConfigServicePosix::OnDNSChanged(unsigned detail) {
This function is identical on Windows and posix.  Think this CL is big enough as
is, but in a future CL, may be nice to have this function in the parent class. 
Then you can also mock out the readers and run tests by mimicking OnDNSChanged
events, and return configs via Gmock.

[commit-bot: I haz the power, 2012-05-16 16:41:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/szym@chromium.org/10377092/1085

[commit-bot: I haz the power, 2012-05-16 18:10:56]

Change committed as 137457