[net] Move DnsConfigService to NetworkChangeNotifier.

[net] Move DnsConfigService to NetworkChangeNotifier.

This merges DnsConfigWatcher back into DnsConfigService and installs
DnsConfigService at NetworkChangeNotifier. It removes |detail| from
OnDNSChanged callback, and exposes NetworkChangeNotifier::GetDnsConfig.

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

[szym, 2012-08-27 19:34:08]

mmenke: please review
pauljensen: FYI changes to network_change_notifier.h

[pauljensen, 2012-08-27 21:44:10]

NetworkChangeNotifier changes look fine to me except the little thing I
commented on.  I had a couple other minor questions but they're not very
important.

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

http://codereview.chromium.org/10873018/diff/10004/net/base/network_change_no...
net/base/network_change_notifier.cc:220:
g_network_change_notifier->network_state_->GetDnsConfig(config);
This derefs g_network_change_notifier if its NULL...need an else or return.

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

http://codereview.chromium.org/10873018/diff/10004/net/dns/dns_config_service...
net/dns/dns_config_service.cc:99: this->StartWatching();
|StartWatching|'s return value is never used...doesn't really matter; I guess it
just allows future use.

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

http://codereview.chromium.org/10873018/diff/10004/net/dns/dns_config_service...
net/dns/dns_config_service_win.cc:498: class DnsConfigServiceWin::Watcher {
Why is DnsConfigServiceWin::Watcher a separate class?  To keep the code more
organized?

[szym, 2012-08-27 21:49:20]

Thanks for catching that null-dereference bug!

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

http://codereview.chromium.org/10873018/diff/10004/net/base/network_change_no...
net/base/network_change_notifier.cc:220:
g_network_change_notifier->network_state_->GetDnsConfig(config);
On 2012/08/27 21:44:10, pauljensen wrote:
> This derefs g_network_change_notifier if its NULL...need an else or return.

Good catch!

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

http://codereview.chromium.org/10873018/diff/10004/net/dns/dns_config_service...
net/dns/dns_config_service.cc:99: this->StartWatching();
On 2012/08/27 21:44:10, pauljensen wrote:
> |StartWatching|'s return value is never used...doesn't really matter; I guess
it
> just allows future use.

Right. http://crbug.com/116139. I guess I should figure out how to measure how
often the watchers actually fail.

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

http://codereview.chromium.org/10873018/diff/10004/net/dns/dns_config_service...
net/dns/dns_config_service_win.cc:498: class DnsConfigServiceWin::Watcher {
On 2012/08/27 21:44:10, pauljensen wrote:
> Why is DnsConfigServiceWin::Watcher a separate class?  To keep the code more
> organized?

The primary reason is that this is a direct copy from the old DnsConfigWatcher.
That said, I'd like to keep it as DnsConfigServiceWin::Watcher so that I can
address http://crbug.com/116139 if needed. I think it'd be simpler to restart
the big Watcher rather than each individual RegistryWatchers.

[mmenke, 2012-08-28 15:18:40]

http://codereview.chromium.org/10873018/diff/8007/net/base/host_resolver_impl.cc
File net/base/host_resolver_impl.cc (right):

http://codereview.chromium.org/10873018/diff/8007/net/base/host_resolver_impl...
net/base/host_resolver_impl.cc:2109:
NetworkChangeNotifier::GetDnsConfig(&dns_config);
Hmm...  I don't see a clean way to avoid the double copy of the DnsClient here. 
:(

Suppose we could make it ref-counted, but that's kinda ugly, too.

http://codereview.chromium.org/10873018/diff/8007/net/base/network_change_not...
File net/base/network_change_notifier.h (right):

http://codereview.chromium.org/10873018/diff/8007/net/base/network_change_not...
net/base/network_change_notifier.h:117: // system has a large HOSTS file.
"currently known" is a little unclear.  Maybe "last read"?

http://codereview.chromium.org/10873018/diff/8007/net/base/network_change_not...
net/base/network_change_notifier.h:180: // TODO(szym): retrieve AddressMap from
NetworkState. http://crbug.com/144212
nit:  Capitalized "Retrieve".

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

http://codereview.chromium.org/10873018/diff/8007/net/dns/dns_config_service....
net/dns/dns_config_service.cc:91: this->ReadNow();
You don't need "this->" here, or below.

http://codereview.chromium.org/10873018/diff/8007/net/dns/dns_config_service_...
File net/dns/dns_config_service_win.cc (right):

http://codereview.chromium.org/10873018/diff/8007/net/dns/dns_config_service_...
net/dns/dns_config_service_win.cc:497: // IO loop.
nit:  I think "Must live on a thread which allows IO." would be clearer, since
"IO loop" can more easily be confused with IO thread (Naming scheme is a little
unfortunate), and it's not just setting up the watches that requires the IO loop
- the callbacks must take place on the same thread.

http://codereview.chromium.org/10873018/diff/8007/net/dns/dns_config_service_...
net/dns/dns_config_service_win.cc:668: OnHostsChanged(true);
This seems a little weird.  If the hosts_watcher_ failed to start, but we run
this, we can end up getting a full DnsConfig, which seems a little weird, and
seems like it could be a problem if the hosts file changes again.

In general, I'm concerned about cases where starting one watch fails.  It seems
like we could still manage to assemble a DNSConfig based on what we did
successfully start watching, assuming reading still manages to succeed for some
reason, which seems a little odd.

We're also no longer triggering DNSChanged events if we can only read half the
config, and it changes.  I understand the need to suppress extra events, but I
do worry a bit about suppressing events when we can't read the hosts file, for
instance.

[szym, 2012-08-28 23:14:38]

http://codereview.chromium.org/10873018/diff/8007/net/base/host_resolver_impl.cc
File net/base/host_resolver_impl.cc (right):

http://codereview.chromium.org/10873018/diff/8007/net/base/host_resolver_impl...
net/base/host_resolver_impl.cc:2109:
NetworkChangeNotifier::GetDnsConfig(&dns_config);
On 2012/08/28 15:18:40, Matt Menke wrote:
> Hmm...  I don't see a clean way to avoid the double copy of the DnsClient
here. 
> :(
> 
> Suppose we could make it ref-counted, but that's kinda ugly, too.

The copy is needed for thread-safety. See http://crbug.com/144212

http://codereview.chromium.org/10873018/diff/8007/net/dns/dns_config_service_...
File net/dns/dns_config_service_win.cc (right):

http://codereview.chromium.org/10873018/diff/8007/net/dns/dns_config_service_...
net/dns/dns_config_service_win.cc:668: OnHostsChanged(true);
On 2012/08/28 15:18:40, Matt Menke wrote:
> This seems a little weird.  If the hosts_watcher_ failed to start, but we run
> this, we can end up getting a full DnsConfig, which seems a little weird, and
> seems like it could be a problem if the hosts file changes again.

Right, hence I RemoveIPAddressObserver in OnHostsChanged(false). I moved the
DNSObserver implementation to the Watcher class. The current (lazy) intention is
that once any watcher fails, DnsConfig should never be reported as complete.

> We're also no longer triggering DNSChanged events if we can only read half the
> config, and it changes.  I understand the need to suppress extra events, but I
> do worry a bit about suppressing events when we can't read the hosts file, for
> instance.

Correct. DnsConfigService will refuse to re-signal empty config. This could be
changed to allow re-signaling if the part that could be read did in fact change.
I have removed the |detail| field from OnDNSChanged since it was primarily
serving DnsConfigService. Do you think it would still have value? Alternatively,
I think we can ignore changes to HOSTS. In most cases, changes to HOSTS are
manual.

Note, right now parsing HOSTS fails if and only if the file cannot be read into
a 64kB buffer. I plan to make the buffer larger, but also use stat() to check if
the file has actually changed to avoid disk access when FilePathWatcher sends
false positives.

[mmenke, 2012-08-29 16:31:20]

http://codereview.chromium.org/10873018/diff/8007/net/dns/dns_config_service_...
File net/dns/dns_config_service_win.cc (right):

http://codereview.chromium.org/10873018/diff/8007/net/dns/dns_config_service_...
net/dns/dns_config_service_win.cc:668: OnHostsChanged(true);
On 2012/08/28 23:14:38, szym wrote:
> On 2012/08/28 15:18:40, Matt Menke wrote:
> > This seems a little weird.  If the hosts_watcher_ failed to start, but we
run
> > this, we can end up getting a full DnsConfig, which seems a little weird,
and
> > seems like it could be a problem if the hosts file changes again.
> 
> Right, hence I RemoveIPAddressObserver in OnHostsChanged(false). I moved the
> DNSObserver implementation to the Watcher class. The current (lazy) intention
is
> that once any watcher fails, DnsConfig should never be reported as complete.

But you don't stop watching if watcher_->Watch(); fails.

> > We're also no longer triggering DNSChanged events if we can only read half
the
> > config, and it changes.  I understand the need to suppress extra events, but
I
> > do worry a bit about suppressing events when we can't read the hosts file,
for
> > instance.
> 
> Correct. DnsConfigService will refuse to re-signal empty config. This could be
> changed to allow re-signaling if the part that could be read did in fact
change.
> I have removed the |detail| field from OnDNSChanged since it was primarily
> serving DnsConfigService. Do you think it would still have value?
Alternatively,
> I think we can ignore changes to HOSTS. In most cases, changes to HOSTS are
> manual.
> 
> Note, right now parsing HOSTS fails if and only if the file cannot be read
into
> a 64kB buffer. I plan to make the buffer larger, but also use stat() to check
if
> the file has actually changed to avoid disk access when FilePathWatcher sends
> false positives.

My concern here is that we used to clear the cache and restart DNS lookups on
IPAddress changed.  We then replaced this behavior with watching the DNS
configuration.  But with this CL, if we fail to get a valid DNS configuration,
we'll never clear the DNS cache and cancel outstanding requests.

[mmenke, 2012-08-29 16:35:40]

http://codereview.chromium.org/10873018/diff/8007/net/base/host_resolver_impl.cc
File net/base/host_resolver_impl.cc (right):

http://codereview.chromium.org/10873018/diff/8007/net/base/host_resolver_impl...
net/base/host_resolver_impl.cc:2109:
NetworkChangeNotifier::GetDnsConfig(&dns_config);
On 2012/08/28 23:14:38, szym wrote:
> On 2012/08/28 15:18:40, Matt Menke wrote:
> > Hmm...  I don't see a clean way to avoid the double copy of the DnsClient
> here. 
> > :(
> > 
> > Suppose we could make it ref-counted, but that's kinda ugly, too.
> 
> The copy is needed for thread-safety. See http://crbug.com/144212

Right, but the double (To dns_config and then to the dns_client_) is not needed
for thread safety, just cleanliness.

One solution would be to make the DnsConfig constant after construction, and
then to make it ref counted threadsafe.  That would eliminate all the copies
except when the config changes, but that has the obvious downside of ref
counting.

[szym, 2012-08-29 17:08:52]

http://codereview.chromium.org/10873018/diff/8007/net/dns/dns_config_service_...
File net/dns/dns_config_service_win.cc (right):

http://codereview.chromium.org/10873018/diff/8007/net/dns/dns_config_service_...
net/dns/dns_config_service_win.cc:668: OnHostsChanged(true);
On 2012/08/29 16:31:20, Matt Menke wrote:
> But you don't stop watching if watcher_->Watch(); fails.

Right, I think having a broken Watcher makes everything much more complex. 

> My concern here is that we used to clear the cache and restart DNS lookups on
> IPAddress changed.  We then replaced this behavior with watching the DNS
> configuration.  But with this CL, if we fail to get a valid DNS configuration,
> we'll never clear the DNS cache and cancel outstanding requests.

We still restart lookups OnIPAddressChanged. The impact of this CL on
HostResolverImpl is really just to filter out signals from the FilePathWatcher
on /etc/resolv.conf and /etc/hosts. If it's desirable to keep sending those
signals even if one of them failed, then I'll need to rework DnsConfigService to
allow sending invalid DnsConfig when we detected that those files have really
changed, but failure in watchers prevents us from assembling a complete
DnsConfig.

[mmenke, 2012-08-29 17:55:45]

http://codereview.chromium.org/10873018/diff/8007/net/dns/dns_config_service_...
File net/dns/dns_config_service_win.cc (right):

http://codereview.chromium.org/10873018/diff/8007/net/dns/dns_config_service_...
net/dns/dns_config_service_win.cc:668: OnHostsChanged(true);
On 2012/08/29 17:08:52, szym wrote:
> On 2012/08/29 16:31:20, Matt Menke wrote:
> > But you don't stop watching if watcher_->Watch(); fails.
> 
> Right, I think having a broken Watcher makes everything much more complex. 

Seems like we shouldn't start watching OnIPAddressChanged unless watching
succeeds.

Come to think of it, we probably have a similar issue when some of the registry
watchers fail to start, too.

> > My concern here is that we used to clear the cache and restart DNS lookups
on
> > IPAddress changed.  We then replaced this behavior with watching the DNS
> > configuration.  But with this CL, if we fail to get a valid DNS
configuration,
> > we'll never clear the DNS cache and cancel outstanding requests.
> 
> We still restart lookups OnIPAddressChanged. The impact of this CL on
> HostResolverImpl is really just to filter out signals from the FilePathWatcher
> on /etc/resolv.conf and /etc/hosts. If it's desirable to keep sending those
> signals even if one of them failed, then I'll need to rework DnsConfigService
to
> allow sending invalid DnsConfig when we detected that those files have really
> changed, but failure in watchers prevents us from assembling a complete
> DnsConfig.
> 

You're right.  For some reason, I was thinking we stopped listening to
OnIPAddressChanged in favor of DNS config changes.  So was was worrying about
nothing.  Still concerned about the other issue, though.

[mmenke, 2012-08-29 23:43:14]

LGTM.

http://codereview.chromium.org/10873018/diff/20002/net/base/network_change_no...
File net/base/network_change_notifier_mac.cc (right):

http://codereview.chromium.org/10873018/diff/20002/net/base/network_change_no...
net/base/network_change_notifier_mac.cc:41: // to open registry keys and to
handle FilePathWatcher updates.
nit:  Fix comment (No registry on Mac)

[mmenke, 2012-08-29 23:44:41]

(I hadn't noticed you fixed the IPAddressObserver issue)

[szym, 2012-08-30 20:35:37]

PTAL at patch 11. Now once any watcher fails, DnsConfigService will continue
sending notifications when changes are detected, but it will always report empty
DnsConfig(). This is equivalent to timing out when we fail to read a part of the
config.

I added a new test and (hopefully) clarified the old tests.

[mmenke, 2012-08-31 15:05:59]

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

http://codereview.chromium.org/10873018/diff/37001/net/dns/dns_config_service...
net/dns/dns_config_service.cc:212: return;  // Let it time out.
Should we stop the timer if |needs_update_| is false?

http://codereview.chromium.org/10873018/diff/37001/net/dns/dns_config_service...
File net/dns/dns_config_service_unittest.cc (right):

http://codereview.chromium.org/10873018/diff/37001/net/dns/dns_config_service...
net/dns/dns_config_service_unittest.cc:56: DnsConfig config = last_config_;
This doesn't seem to be needed.

http://codereview.chromium.org/10873018/diff/37001/net/dns/dns_config_service...
net/dns/dns_config_service_unittest.cc:113:
EXPECT_TRUE(last_config_.Equals(config));
We're depending an awful lot on Equals returning true only when it should.  May
want to add a couple EXPECT_FALSE(...Equals) to these tests, just to validate
that it's behaving.

[szym, 2012-08-31 15:55:57]

I moved simplified the logic. |watch_failed_| is equivalent to saying we
|have_hosts_| or |have_config_| when deciding whether to OnConfigComplete.

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

http://codereview.chromium.org/10873018/diff/37001/net/dns/dns_config_service...
net/dns/dns_config_service.cc:212: return;  // Let it time out.
On 2012/08/31 15:05:59, Matt Menke wrote:
> Should we stop the timer if |needs_update_| is false?

I see. The idea is to not signal anything if we have not detected a real change.

Sticking with the OnTimeout approach would also require that we don't set
|need_update_ = true| in OnTimeout if watch_failed_, and that we reset
need_update_ to false at some point.

I think it's probably less confusing to just |callback_.Run(DnsConfig())| if
|watch_failed_| is true.

I updated the tests to expect immediate notifications rather than time outs.

[mmenke, 2012-08-31 16:04:03]

LGTM

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

http://codereview.chromium.org/10873018/diff/37001/net/dns/dns_config_service...
net/dns/dns_config_service.cc:212: return;  // Let it time out.
On 2012/08/31 15:55:57, szym wrote:
> On 2012/08/31 15:05:59, Matt Menke wrote:
> > Should we stop the timer if |needs_update_| is false?
> 
> I see. The idea is to not signal anything if we have not detected a real
change.
> 
> Sticking with the OnTimeout approach would also require that we don't set
> |need_update_ = true| in OnTimeout if watch_failed_, and that we reset
> need_update_ to false at some point.
> 
> I think it's probably less confusing to just |callback_.Run(DnsConfig())| if
> |watch_failed_| is true.
> 
> I updated the tests to expect immediate notifications rather than time outs.

I agree - think that this is the less confusing solution.

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

http://codereview.chromium.org/10873018/diff/45005/net/dns/dns_config_service...
net/dns/dns_config_service.cc:217: callback_.Run(DnsConfig());
May want to add a comment along the lines of "If a watch failed, the config may
not be valid".

[mmenke, 2012-08-31 16:04:48]

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

http://codereview.chromium.org/10873018/diff/45005/net/dns/dns_config_service...
net/dns/dns_config_service.cc:217: callback_.Run(DnsConfig());
On 2012/08/31 16:04:03, Matt Menke wrote:
> May want to add a comment along the lines of "If a watch failed, the config
may
> not be valid".

Err...valid -> accurate

[szym, 2012-08-31 16:09:44]

Thanks!

On 2012/08/31 16:04:03, Matt Menke wrote:
> LGTM
>

[commit-bot: I haz the power, 2012-08-31 16:13:15]

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

[commit-bot: I haz the power, 2012-08-31 19:52:43]

Change committed as 154485