[net/dns] Removes locking from DnsConfigServiceWin and adds local computer name.

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/file_path.h"
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.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/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_protocol.h"
 #include "net/dns/file_path_watcher_wrapper.h"
 #include "net/dns/serial_worker.h"
 
 #pragma comment(lib, "iphlpapi.lib")
 
 namespace net {
 
 namespace internal {
 
 namespace {
 
-// Watches a single registry key for changes. Thread-safe.
-class RegistryWatcher : public base::win::ObjectWatcher::Delegate {
+// Registry key paths.
+const wchar_t* const kTcpipPath =
+    L"SYSTEM\\CurrentControlSet\\Services\\Tcpip\\Parameters";
+const wchar_t* const kTcpip6Path =
+    L"SYSTEM\\CurrentControlSet\\Services\\Tcpip6\\Parameters";
+const wchar_t* const kDnscachePath =
+    L"SYSTEM\\CurrentControlSet\\Services\\Dnscache\\Parameters";
+const wchar_t* const kPolicyPath =
+    L"SOFTWARE\\Policies\\Microsoft\\Windows NT\\DNSClient";
+
+// Convenience for reading values using RegKey.
+class RegistryReader : public base::NonThreadSafe {
  public:
-  typedef base::Callback<void(bool succeeded)> CallbackType;
-  RegistryWatcher() {}
-
-  bool Watch(const wchar_t* key, const CallbackType& callback) {
-    base::AutoLock lock(lock_);
-    DCHECK(!callback.is_null());
-    CancelLocked();
-    if (key_.Open(HKEY_LOCAL_MACHINE, key,
-                  KEY_NOTIFY | KEY_QUERY_VALUE) != ERROR_SUCCESS)
-      return false;
-    if (key_.StartWatching() != ERROR_SUCCESS)
-      return false;
-    if (!watcher_.StartWatching(key_.watch_event(), this))
-      return false;
-    callback_ = callback;
-    return true;
-  }
-
-  void Cancel() {
-    base::AutoLock lock(lock_);
-    CancelLocked();
-  }
-
-  virtual void OnObjectSignaled(HANDLE object) OVERRIDE {
-    base::AutoLock lock(lock_);
-    bool succeeded = (key_.StartWatching() == ERROR_SUCCESS) &&
-                     watcher_.StartWatching(key_.watch_event(), this);
-    if (!callback_.is_null())
-      callback_.Run(succeeded);
+  explicit RegistryReader(const wchar_t* key) {
+    // Ignoring the result. |key_.Valid()| will catch failures.
+    key_.Open(HKEY_LOCAL_MACHINE, key, KEY_QUERY_VALUE);
   }
 
   bool ReadString(const wchar_t* name,
                   DnsSystemSettings::RegString* out) const {
-    base::AutoLock lock(lock_);
+    DCHECK(CalledOnValidThread());
     out->set = false;
     if (!key_.Valid()) {
       // Assume that if the |key_| is invalid then the key is missing.
       return true;
     }
     LONG result = key_.ReadValue(name, &out->value);
     if (result == ERROR_SUCCESS) {
       out->set = true;
       return true;
     }
     return (result == ERROR_FILE_NOT_FOUND);
   }
 
   bool ReadDword(const wchar_t* name,
                  DnsSystemSettings::RegDword* out) const {
-    base::AutoLock lock(lock_);
+    DCHECK(CalledOnValidThread());
     out->set = false;
     if (!key_.Valid()) {
       // Assume that if the |key_| is invalid then the key is missing.
       return true;
     }
     LONG result = key_.ReadValueDW(name, &out->value);
     if (result == ERROR_SUCCESS) {
       out->set = true;
       return true;
     }
     return (result == ERROR_FILE_NOT_FOUND);
   }
 
  private:
-  void CancelLocked() {
-    lock_.AssertAcquired();
+  base::win::RegKey key_;
+
+  DISALLOW_COPY_AND_ASSIGN(RegistryReader);
+};
+
+
+// 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());
+    Cancel();
+    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;
+    callback_ = callback;
+    return true;
+  }
+
+  bool IsWatching() const {
+    DCHECK(CalledOnValidThread());
+    return !callback_.is_null();
+  }
+
+  void Cancel() {
+    DCHECK(CalledOnValidThread());
     callback_.Reset();
     if (key_.Valid()) {
       watcher_.StopWatching();
       key_.StopWatching();
+      key_.Close();
     }
   }
 
-  mutable base::Lock lock_;
+  virtual void OnObjectSignaled(HANDLE object) OVERRIDE {
+    DCHECK(CalledOnValidThread());
+    bool succeeded = (key_.StartWatching() == ERROR_SUCCESS) &&
+                      watcher_.StartWatching(key_.watch_event(), this);
+    CallbackType callback = callback_;
+    if (!succeeded)
+      Cancel();
+    if (!callback.is_null())
+      callback.Run(succeeded);
+  }
+
+ private:
   CallbackType callback_;
   base::win::RegKey key_;
   base::win::ObjectWatcher watcher_;
 
   DISALLOW_COPY_AND_ASSIGN(RegistryWatcher);
 };
 
+// Returns NULL if failed.
+scoped_ptr_malloc<IP_ADAPTER_ADDRESSES> ReadIpHelper(ULONG flags) {
+  base::ThreadRestrictions::AssertIOAllowed();
+
+  scoped_ptr_malloc<IP_ADAPTER_ADDRESSES> out;
+  ULONG len = 15000;  // As recommended by MSDN for GetAdaptersAddresses.
+  UINT rv = ERROR_BUFFER_OVERFLOW;
+  // Try up to three times.
+  for (unsigned tries = 0;
+        (tries < 3) && (rv == ERROR_BUFFER_OVERFLOW);
+        tries++) {
+    out.reset(
+        reinterpret_cast<PIP_ADAPTER_ADDRESSES>(malloc(len)));
+    rv = GetAdaptersAddresses(AF_UNSPEC, flags, NULL,
+                              out.get(), &len);
+  }
+  if (rv != NO_ERROR)
+    out.reset();
+  return out.Pass();
+}
+
 // Converts a string16 domain name to ASCII, possibly using punycode.
 // Returns true if the conversion succeeds and output is not empty. In case of
 // failure, |domain| might become dirty.
 bool ParseDomainASCII(const string16& widestr, std::string* domain) {
   DCHECK(domain);
   if (widestr.empty())
     return false;
 
   // Check if already ASCII.
   if (IsStringASCII(widestr)) {
@@ skipping 184 matching lines @@
       : service_(service),
         success_(false) {}
 
   bool Watch() {
     DCHECK(loop()->BelongsToCurrentThread());
 
     RegistryWatcher::CallbackType callback =
         base::Bind(&ConfigReader::OnChange, base::Unretained(this));
 
     // The Tcpip key must be present.
-    if (!tcpip_watcher_.Watch(
-        L"SYSTEM\\CurrentControlSet\\Services\\Tcpip\\Parameters",
-        callback))
+    if (!tcpip_watcher_.Watch(kTcpipPath, callback))
       return false;
 
     // Watch for IPv6 nameservers.
-    tcpip6_watcher_.Watch(
-        L"SYSTEM\\CurrentControlSet\\Services\\Tcpip6\\Parameters",
-        callback);
+    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(
-        L"SYSTEM\\CurrentControlSet\\Services\\Dnscache\\Parameters",
-        callback);
-
-    policy_watcher_.Watch(
-        L"SOFTWARE\\Policies\\Microsoft\\Windows NT\\DNSClient",
-        callback);
+    dnscache_watcher_.Watch(kDnscachePath, callback);
+    policy_watcher_.Watch(kPolicyPath, callback);
 
     WorkNow();
     return true;
   }
 
   void Cancel() {
     DCHECK(loop()->BelongsToCurrentThread());
     SerialWorker::Cancel();
     policy_watcher_.Cancel();
     dnscache_watcher_.Cancel();
@@ skipping 11 matching lines @@
     if (!IsCancelled())
       service_->InvalidateConfig();
     // We don't trust a config that we cannot watch in the future.
     // TODO(szym): re-start watcher if that makes sense. http://crbug.com/116139
     if (succeeded)
       WorkNow();
     else
       LOG(ERROR) << "Failed to watch DNS config";
   }
 
-  bool ReadIpHelper(DnsSystemSettings* settings) {
-    base::ThreadRestrictions::AssertIOAllowed();
-
-    ULONG flags = GAA_FLAG_SKIP_ANYCAST |
-                  GAA_FLAG_SKIP_UNICAST |
-                  GAA_FLAG_SKIP_MULTICAST |
-                  GAA_FLAG_SKIP_FRIENDLY_NAME;
-    ULONG len = 15000;  // As recommended by MSDN for GetAdaptersAddresses.
-    UINT rv = ERROR_BUFFER_OVERFLOW;
-    // Try up to three times.
-    for (unsigned tries = 0;
-         (tries < 3) && (rv == ERROR_BUFFER_OVERFLOW);
-         tries++) {
-      settings->addresses.reset(
-          reinterpret_cast<PIP_ADAPTER_ADDRESSES>(malloc(len)));
-      rv = GetAdaptersAddresses(AF_UNSPEC, flags, NULL,
-                                settings->addresses.get(), &len);
-    }
-    return (rv == NO_ERROR);
-  }
-
-  bool ReadDevolutionSetting(const RegistryWatcher& watcher,
+  bool ReadDevolutionSetting(const RegistryReader& reader,
                              DnsSystemSettings::DevolutionSetting& setting) {
-    return watcher.ReadDword(L"UseDomainNameDevolution", &setting.enabled) &&
-           watcher.ReadDword(L"DomainNameDevolutionLevel", &setting.level);
+    return reader.ReadDword(L"UseDomainNameDevolution", &setting.enabled) &&
+           reader.ReadDword(L"DomainNameDevolutionLevel", &setting.level);
   }
 
   virtual void DoWork() OVERRIDE {
     // Should be called on WorkerPool.
     success_ = false;
 
     DnsSystemSettings settings;
     memset(&settings, 0, sizeof(settings));
-    if (!ReadIpHelper(&settings))
+    settings.addresses = ReadIpHelper(GAA_FLAG_SKIP_ANYCAST |
+                                      GAA_FLAG_SKIP_UNICAST |
+                                      GAA_FLAG_SKIP_MULTICAST |
+                                      GAA_FLAG_SKIP_FRIENDLY_NAME);
+    if (!settings.addresses.get())
       return;  // no point reading the rest
 
-    if (!policy_watcher_.ReadString(L"SearchList",
-                                    &settings.policy_search_list))
+    RegistryReader tcpip_reader(kTcpipPath);
+    RegistryReader tcpip6_reader(kTcpip6Path);
+    RegistryReader dnscache_reader(kDnscachePath);
+    RegistryReader policy_reader(kPolicyPath);
+
+    if (!policy_reader.ReadString(L"SearchList",
+                                  &settings.policy_search_list))
       return;
 
-    if (!tcpip_watcher_.ReadString(L"SearchList", &settings.tcpip_search_list))
+    if (!tcpip_reader.ReadString(L"SearchList", &settings.tcpip_search_list))
       return;
 
-    if (!tcpip_watcher_.ReadString(L"Domain", &settings.tcpip_domain))
+    if (!tcpip_reader.ReadString(L"Domain", &settings.tcpip_domain))
       return;
 
-    if (!ReadDevolutionSetting(policy_watcher_, settings.policy_devolution))
+    if (!ReadDevolutionSetting(policy_reader, settings.policy_devolution))
       return;
 
-    if (!ReadDevolutionSetting(dnscache_watcher_,
+    if (!ReadDevolutionSetting(dnscache_reader,
                                settings.dnscache_devolution))
       return;
 
-    if (!ReadDevolutionSetting(tcpip_watcher_, settings.tcpip_devolution))
+    if (!ReadDevolutionSetting(tcpip_reader, settings.tcpip_devolution))
       return;
 
-    if (!policy_watcher_.ReadDword(L"AppendToMultiLabelName",
-                                   &settings.append_to_multi_label_name))
+    if (!policy_reader.ReadDword(L"AppendToMultiLabelName",
+                                 &settings.append_to_multi_label_name))
       return;
 
     success_ = ConvertSettingsToDnsConfig(settings, &dns_config_);
   }
 
   virtual void OnWorkFinished() OVERRIDE {
     DCHECK(loop()->BelongsToCurrentThread());
     DCHECK(!IsCancelled());
     if (success_) {
       service_->OnConfigRead(dns_config_);
     } else {
       LOG(WARNING) << "Failed to read config.";
     }
   }
 
   DnsConfigServiceWin* service_;
   // Written in DoRead(), read in OnReadFinished(). No locking required.
   DnsConfig dns_config_;
   bool success_;
 
   RegistryWatcher tcpip_watcher_;
   RegistryWatcher tcpip6_watcher_;
   RegistryWatcher dnscache_watcher_;
   RegistryWatcher policy_watcher_;
 };
 
+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"));
+}
+
+// An extension for DnsHostsReader which also reads local name from
+// GetComputerNameEx and watches registry for changes to local name.

[mmenke, 2012/03/22 16:35:05]

It also watches the HOSTs file, which DnsHostsReaders don't normally do.

+class DnsConfigServiceWin::HostsReader
+    : public DnsHostsReader,
+      public NetworkChangeNotifier::IPAddressObserver {
+ public:
+  explicit HostsReader(DnsConfigServiceWin* service)
+      : DnsHostsReader(GetHostsPath()), service_(service) {
+  }
+
+  bool Watch() {
+    DCHECK(loop()->BelongsToCurrentThread());
+
+    // In case the reader is restarted, remove it from the observer list.
+    NetworkChangeNotifier::RemoveIPAddressObserver(this);

[mmenke, 2012/03/22 16:35:05]

Shouldn't we just call Cancel()?

Alternatively, here and above, you could use DCHECKs to make sure we were
already cancelled and require Cancel be called before restarting the watch.  The
advantage of that approach is it's a little more robust against Watch() being
called again at unexpected times.  Up to you.

[szym, 2012/03/22 18:42:45]

SerialWorker::Cancel() cannot be undone. Making it undoable without races with
the WorkerPool is tricky.
 

+
+    if (!hosts_watcher_.Watch(path(),
+                              base::Bind(&HostsReader::OnHostsChanged,
+                                         base::Unretained(this)))) {
+      return false;
+    }
+    NetworkChangeNotifier::AddIPAddressObserver(this);
+    WorkNow();
+    return true;
+  }
+
+  void Cancel() {
+    DnsHostsReader::Cancel();
+    hosts_watcher_.Cancel();
+    NetworkChangeNotifier::RemoveIPAddressObserver(this);
+  }
+
+ private:
+  virtual void OnIPAddressChanged() OVERRIDE {
+    DCHECK(loop()->BelongsToCurrentThread());
+    service_->InvalidateHosts();
+    if (!hosts_watcher_.IsWatching())
+      return;
+    WorkNow();
+  }
+
+  void OnHostsChanged(bool succeeded) {
+    DCHECK(loop()->BelongsToCurrentThread());
+    service_->InvalidateHosts();
+    if (succeeded)
+      WorkNow();
+    else
+      LOG(ERROR) << "Failed to watch DNS hosts";
+  }
+
+  virtual void DoWork() OVERRIDE {
+    DnsHostsReader::DoWork();
+
+    if (!success_)
+      return;
+
+    // Default address of local computer name and "localhost" can be overridden
+    // by the HOSTS file, but if it's not there, then we need to fill it in.
+
+    success_ = false;
+
+    WCHAR buffer[MAX_PATH];
+    DWORD size = MAX_PATH;
+    std::string localname;
+    if (!GetComputerNameExW(ComputerNameDnsHostname, buffer, &size) ||
+        !ParseDomainASCII(buffer, &localname)) {
+      LOG(ERROR) << "Failed to read local computer name";
+      return;
+    }
+    // TODO(szym): normalize |localname| to lowercase. http://crbug.com/118995
+
+    scoped_ptr_malloc<IP_ADAPTER_ADDRESSES> addresses =
+        ReadIpHelper(GAA_FLAG_SKIP_ANYCAST |
+                     GAA_FLAG_SKIP_DNS_SERVER |
+                     GAA_FLAG_SKIP_MULTICAST |
+                     GAA_FLAG_SKIP_FRIENDLY_NAME);
+    if (!addresses.get())
+      return;
+
+    // The order of adapters is the network binding order, so stick to the
+    // first good adapter for each family.
+    IPAddressNumber& local_ipv4 =
+        dns_hosts_[DnsHostsKey(localname, ADDRESS_FAMILY_IPV4)];
+    IPAddressNumber& local_ipv6 =
+        dns_hosts_[DnsHostsKey(localname, ADDRESS_FAMILY_IPV6)];

[mmenke, 2012/03/22 16:35:05]

This isn't safe.  If we insert an entry into dns_hosts_ for local_ipv6, it could
invalidate |local_ipv4|.

[mmenke, 2012/03/22 16:35:05]

Also, suggest you use pointers here instead of references.  Makes things
clearer.  You may even just want to store them on the stack instead, and at the
end, only modify dns_hosts_ if they aren't empty.

[szym, 2012/03/22 18:42:45]

The references (like iterators) in std::map are not affected by other
insertions. http://kera.name/articles/2011/06/iterator-invalidation-rules/

That said, I don't think this optimization (avoiding looking into the map more
than once) is worth the complexity.

[mmenke, 2012/03/22 19:36:08]

I had not realized that  Very handy, though I agree that it's not worth the
complexity.

+    for (const IP_ADAPTER_ADDRESSES* adapter = addresses.get();
+         adapter != NULL && (local_ipv4.empty() || local_ipv6.empty());
+         adapter = adapter->Next) {
+      if (adapter->OperStatus != IfOperStatusUp)
+        continue;
+      if (adapter->IfType == IF_TYPE_SOFTWARE_LOOPBACK)
+        continue;
+
+      for (const IP_ADAPTER_UNICAST_ADDRESS* address =
+             adapter->FirstUnicastAddress;

[mmenke, 2012/03/22 16:35:05]

nit:  4-space indent

+           address != NULL;
+           address = address->Next) {
+        IPEndPoint ipe;
+        if (ipe.FromSockAddr(address->Address.lpSockaddr,
+                             address->Address.iSockaddrLength)) {

[mmenke, 2012/03/22 16:35:05]

nit:  Think this would be a little easier to read if you aborted first, then had
the rest of the logic.  Means you don't have to nest quite so deep.

if (!blah) {
  return;
}

if (ipv4)
...

+          if (local_ipv4.empty() && (ipe.GetFamily() == AF_INET)) {
+            local_ipv4 = ipe.address();
+          } else if (local_ipv6.empty() && (ipe.GetFamily() == AF_INET6)) {
+            local_ipv6 = ipe.address();
+          }
+        } else {
+          return;
+        }
+      }
+    }
+
+    if (local_ipv4.empty())
+      dns_hosts_.erase(DnsHostsKey(localname, ADDRESS_FAMILY_IPV4));
+
+    if (local_ipv6.empty())
+      dns_hosts_.erase(DnsHostsKey(localname, ADDRESS_FAMILY_IPV6));
+
+    IPAddressNumber& loop_ipv4 =
+        dns_hosts_[DnsHostsKey("localhost", ADDRESS_FAMILY_IPV4)];
+    IPAddressNumber& loop_ipv6 =
+        dns_hosts_[DnsHostsKey("localhost", ADDRESS_FAMILY_IPV6)];

[mmenke, 2012/03/22 16:35:05]

Again, suggest you use pointers.  Also think "loopback" would be a little
clearer, though it does make linebreaking a little uglier.

[szym, 2012/03/22 18:42:45]

I re-discovered that std::map::insert will not overwrite an existing value.
Which is exactly what I tried to accomplish here.

+    if (loop_ipv4.empty()) {
+      const unsigned char kIPv4Localhost[] = { 127, 0, 0, 1 };
+      loop_ipv4 = IPAddressNumber(kIPv4Localhost,
+                                  kIPv4Localhost + arraysize(kIPv4Localhost));
+    }
+
+    if (loop_ipv6.empty()) {
+      const unsigned char kIPv6Localhost[] = { 0, 0, 0, 0, 0, 0, 0, 0,
+                                               0, 0, 0, 0, 0, 0, 0, 1 };
+      loop_ipv6 = IPAddressNumber(kIPv6Localhost,
+                                  kIPv6Localhost + arraysize(kIPv6Localhost));
+    }
+
+    success_ = true;
+  }
+
+  virtual void OnWorkFinished() OVERRIDE {
+    DCHECK(loop()->BelongsToCurrentThread());
+    if (!success_ || !hosts_watcher_.IsWatching())
+      return;
+    service_->OnHostsRead(dns_hosts_);
+  }
+
+  DnsConfigServiceWin* service_;
+  FilePathWatcherWrapper hosts_watcher_;
+
+  DISALLOW_COPY_AND_ASSIGN(HostsReader);
+};
+
+
 DnsConfigServiceWin::DnsConfigServiceWin()
     : config_reader_(new ConfigReader(this)),
-      hosts_watcher_(new FilePathWatcherWrapper()) {}
+      hosts_reader_(new HostsReader(this)) {}
 
 DnsConfigServiceWin::~DnsConfigServiceWin() {
   DCHECK(CalledOnValidThread());
   config_reader_->Cancel();
-  if (hosts_reader_)
-    hosts_reader_->Cancel();
+  hosts_reader_->Cancel();
 }
 
 void DnsConfigServiceWin::Watch(const CallbackType& callback) {
   DCHECK(CalledOnValidThread());
   DCHECK(!callback.is_null());
   set_callback(callback);
 
   // This is done only once per lifetime so open the keys and file watcher
   // handles on this thread.
   // TODO(szym): Should/can this be avoided? http://crbug.com/114223
   base::ThreadRestrictions::ScopedAllowIO allow_io;
 
   if (!config_reader_->Watch()) {
     LOG(ERROR) << "Failed to start watching DNS config";
     InvalidateConfig();
   }
 
-  TCHAR buffer[MAX_PATH];
-  UINT rc = GetSystemDirectory(buffer, MAX_PATH);
-  DCHECK(0 < rc && rc < MAX_PATH);
-  FilePath hosts_path = FilePath(buffer).
-      Append(FILE_PATH_LITERAL("drivers\\etc\\hosts"));
-  hosts_reader_ = new DnsHostsReader(
-      hosts_path,
-      base::Bind(&DnsConfigServiceWin::OnHostsRead, base::Unretained(this)));
-  if (hosts_watcher_->Watch(hosts_path,
-                            base::Bind(&DnsConfigServiceWin::OnHostsChanged,
-                                       base::Unretained(this)))) {
-    OnHostsChanged(true);
-  } else {
-    OnHostsChanged(false);
+  if (!hosts_reader_->Watch()) {
+    LOG(ERROR) << "Failed to start watching HOSTS";
+    InvalidateHosts();
   }
 }
 
-void DnsConfigServiceWin::OnHostsChanged(bool succeeded) {
-  InvalidateHosts();
-  if (succeeded)
-    hosts_reader_->WorkNow();
-  else
-    LOG(ERROR) << "Failed to watch DNS hosts";
-}
-
 }  // namespace internal
 
 // static
 scoped_ptr<DnsConfigService> DnsConfigService::CreateSystemService() {
   return scoped_ptr<DnsConfigService>(new internal::DnsConfigServiceWin());
 }
 
 }  // namespace net