[net] Move DnsConfigService to NetworkChangeNotifier.

net/dns/dns_config_service_win.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/dns/dns_config_service_win.h"
 
 #include <algorithm>
 #include <string>
 
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/compiler_specific.h"
+#include "base/files/file_path_watcher.h"
 #include "base/file_path.h"
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/metrics/histogram.h"
 #include "base/string_split.h"
 #include "base/string_util.h"
 #include "base/synchronization/lock.h"
 #include "base/threading/non_thread_safe.h"
 #include "base/threading/thread_restrictions.h"
 #include "base/time.h"
 #include "base/utf_string_conversions.h"
+#include "base/win/object_watcher.h"
 #include "base/win/registry.h"
 #include "base/win/windows_version.h"
 #include "googleurl/src/url_canon.h"
 #include "net/base/net_util.h"
-#include "net/base/network_change_notifier.h"
 #include "net/dns/dns_hosts.h"
 #include "net/dns/dns_protocol.h"
 #include "net/dns/serial_worker.h"
 
 #pragma comment(lib, "iphlpapi.lib")
 
 namespace net {
 
 namespace internal {
 
@@ skipping 226 matching lines @@
         (*hosts)[DnsHostsKey(localname, ADDRESS_FAMILY_IPV4)] = ipe.address();
       } else if (!have_ipv6 && (ipe.GetFamily() == AF_INET6)) {
         have_ipv6 = true;
         (*hosts)[DnsHostsKey(localname, ADDRESS_FAMILY_IPV6)] = ipe.address();
       }
     }
   }
   return HOSTS_PARSE_WIN_OK;
 }
 
+// Watches a single registry key for changes.
+class RegistryWatcher : public base::win::ObjectWatcher::Delegate,
+                        public base::NonThreadSafe {
+ public:
+  typedef base::Callback<void(bool succeeded)> CallbackType;
+  RegistryWatcher() {}
+
+  bool Watch(const wchar_t* key, const CallbackType& callback) {
+    DCHECK(CalledOnValidThread());
+    DCHECK(!callback.is_null());
+    DCHECK(callback_.is_null());
+    callback_ = callback;
+    if (key_.Open(HKEY_LOCAL_MACHINE, key, KEY_NOTIFY) != ERROR_SUCCESS)
+      return false;
+    if (key_.StartWatching() != ERROR_SUCCESS)
+      return false;
+    if (!watcher_.StartWatching(key_.watch_event(), this))
+      return false;
+    return true;
+  }
+
+  virtual void OnObjectSignaled(HANDLE object) OVERRIDE {
+    DCHECK(CalledOnValidThread());
+    bool succeeded = (key_.StartWatching() == ERROR_SUCCESS) &&
+                      watcher_.StartWatching(key_.watch_event(), this);
+    if (!succeeded && key_.Valid()) {
+      watcher_.StopWatching();
+      key_.StopWatching();
+      key_.Close();
+    }
+    if (!callback_.is_null())
+      callback_.Run(succeeded);
+  }
+
+ private:
+  CallbackType callback_;
+  base::win::RegKey key_;
+  base::win::ObjectWatcher watcher_;
+
+  DISALLOW_COPY_AND_ASSIGN(RegistryWatcher);
+};
+
 }  // namespace
 
 FilePath GetHostsPath() {
   TCHAR buffer[MAX_PATH];
   UINT rc = GetSystemDirectory(buffer, MAX_PATH);
   DCHECK(0 < rc && rc < MAX_PATH);
   return FilePath(buffer).Append(FILE_PATH_LITERAL("drivers\\etc\\hosts"));
 }
 
 bool ParseSearchList(const string16& value, std::vector<std::string>* output) {
@@ skipping 158 matching lines @@
   unsigned num_dots = std::count(primary_suffix.begin(),
                                  primary_suffix.end(), '.');
 
   for (size_t offset = 0; num_dots >= devolution.level.value; --num_dots) {
     offset = primary_suffix.find('.', offset + 1);
     config->search.push_back(primary_suffix.substr(offset + 1));
   }
   return CONFIG_PARSE_WIN_OK;
 }
 
+// Watches registry and HOSTS file for changes. Setting up watches requires
+// IO loop.

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

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.

+class DnsConfigServiceWin::Watcher {
+ public:
+  explicit Watcher(DnsConfigServiceWin* service) : service_(service) {}
+  ~Watcher() {}
+
+  bool Watch() {
+    RegistryWatcher::CallbackType callback =
+        base::Bind(&DnsConfigServiceWin::OnConfigChanged,
+                   base::Unretained(service_));
+
+    bool success = true;
+
+    // The Tcpip key must be present.
+    if (!tcpip_watcher_.Watch(kTcpipPath, callback)) {
+      LOG(ERROR) << "DNS registry watch failed to start.";
+      success = false;
+    }
+
+    // Watch for IPv6 nameservers.
+    tcpip6_watcher_.Watch(kTcpip6Path, callback);
+
+    // DNS suffix search list and devolution can be configured via group
+    // policy which sets this registry key. If the key is missing, the policy
+    // does not apply, and the DNS client uses Tcpip and Dnscache settings.
+    // If a policy is installed, DnsConfigService will need to be restarted.
+    // BUG=99509
+
+    dnscache_watcher_.Watch(kDnscachePath, callback);
+    policy_watcher_.Watch(kPolicyPath, callback);
+
+    if (!hosts_watcher_.Watch(GetHostsPath(),
+                              base::Bind(&Watcher::OnHostsChanged,
+                                         base::Unretained(this)))) {
+      LOG(ERROR) << "DNS hosts watch failed to start.";
+      success = false;
+    }
+    return success;
+  }
+
+ private:
+  void OnHostsChanged(const FilePath& path, bool error) {
+    service_->OnHostsChanged(!error);
+  }
+
+  DnsConfigServiceWin* service_;
+
+  RegistryWatcher tcpip_watcher_;
+  RegistryWatcher tcpip6_watcher_;
+  RegistryWatcher dnscache_watcher_;
+  RegistryWatcher policy_watcher_;
+  base::files::FilePathWatcher hosts_watcher_;
+
+  DISALLOW_COPY_AND_ASSIGN(Watcher);
+};
+
 // Reads config from registry and IpHelper. All work performed on WorkerPool.
 class DnsConfigServiceWin::ConfigReader : public SerialWorker {
  public:
   explicit ConfigReader(DnsConfigServiceWin* service)
       : service_(service),
         success_(false) {}
 
  private:
   virtual ~ConfigReader() {}
 
@@ skipping 71 matching lines @@
   const FilePath path_;
   DnsConfigServiceWin* service_;
   // Written in DoWork, read in OnWorkFinished, no locking necessary.
   DnsHosts hosts_;
   bool success_;
 
   DISALLOW_COPY_AND_ASSIGN(HostsReader);
 };
 
 DnsConfigServiceWin::DnsConfigServiceWin()
-    : config_reader_(new ConfigReader(this)),
+    : watcher_(new Watcher(this)),
+      config_reader_(new ConfigReader(this)),
       hosts_reader_(new HostsReader(this)) {}
 
 DnsConfigServiceWin::~DnsConfigServiceWin() {
   config_reader_->Cancel();
   hosts_reader_->Cancel();
   NetworkChangeNotifier::RemoveIPAddressObserver(this);
 }
 
-void DnsConfigServiceWin::Watch(const CallbackType& callback) {
-  DnsConfigService::Watch(callback);
+void DnsConfigServiceWin::ReadNow() {
+  config_reader_->WorkNow();
+  hosts_reader_->WorkNow();
+}
+
+bool DnsConfigServiceWin::StartWatching() {
   // Also need to observe changes to local non-loopback IP for DnsHosts.
   NetworkChangeNotifier::AddIPAddressObserver(this);
-}
-
-void DnsConfigServiceWin::OnDNSChanged(unsigned detail) {
-  if (detail & NetworkChangeNotifier::CHANGE_DNS_WATCH_FAILED) {
-    InvalidateConfig();
-    InvalidateHosts();
-    // We don't trust a config that we cannot watch in the future.
-    config_reader_->Cancel();
-    hosts_reader_->Cancel();
-    return;
-  }
-  if (detail & NetworkChangeNotifier::CHANGE_DNS_WATCH_STARTED)
-    detail = ~0;  // Assume everything changed.
-  if (detail & NetworkChangeNotifier::CHANGE_DNS_SETTINGS) {
-    InvalidateConfig();
-    config_reader_->WorkNow();
-  }
-  if (detail & NetworkChangeNotifier::CHANGE_DNS_HOSTS) {
-    InvalidateHosts();
-    hosts_reader_->WorkNow();
-  }
+  // TODO(szym): re-start watcher if that makes sense. http://crbug.com/116139
+  return watcher_->Watch();
 }
 
 void DnsConfigServiceWin::OnIPAddressChanged() {
   // Need to update non-loopback IP of local host.
-  if (NetworkChangeNotifier::IsWatchingDNS())
-    OnDNSChanged(NetworkChangeNotifier::CHANGE_DNS_HOSTS);
+  OnHostsChanged(true);

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

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]

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.
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]

But you don't stop watching if watcher_->Watch(); fails.
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.

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

Right, I think having a broken Watcher makes everything much more complex. 
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]

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.
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.

+}
+
+void DnsConfigServiceWin::OnConfigChanged(bool succeeded) {
+  InvalidateConfig();
+  if (succeeded) {
+    config_reader_->WorkNow();
+  } else {
+    LOG(ERROR) << "DNS config watch failed.";
+    // Don't trust the result if the watcher failed.
+    // Note: The SerialWorker will need to be re-constructed to accept new jobs.
+    config_reader_->Cancel();
+  }
+}
+
+void DnsConfigServiceWin::OnHostsChanged(bool succeeded) {
+  InvalidateHosts();
+  if (succeeded) {
+    hosts_reader_->WorkNow();
+  } else {
+    LOG(ERROR) << "DNS hosts watch failed.";
+    // Don't trust the result if the watcher failed.
+    // Note: The SerialWorker will need to be re-constructed to accept new jobs.
+    hosts_reader_->Cancel();
+    NetworkChangeNotifier::RemoveIPAddressObserver(this);
+  }
 }
 
 }  // namespace internal
 
 // static
 scoped_ptr<DnsConfigService> DnsConfigService::CreateSystemService() {
   return scoped_ptr<DnsConfigService>(new internal::DnsConfigServiceWin());
 }
 
 }  // namespace net