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

net/dns/dns_config_service_win.cc

Index: net/dns/dns_config_service_win.cc
diff --git a/net/dns/dns_config_service_win.cc b/net/dns/dns_config_service_win.cc
index 047b92dd03f8d3ecc5a5e77583655ffabeed7881..edd32dbc6d5618401f28010e4b7d986c919257e9 100644
--- a/net/dns/dns_config_service_win.cc
+++ b/net/dns/dns_config_service_win.cc
@@ -16,6 +16,7 @@
 #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"
@@ -23,6 +24,7 @@
 #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"
@@ -35,43 +37,27 @@ 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.
@@ -87,7 +73,7 @@ class RegistryWatcher : public base::win::ObjectWatcher::Delegate {
 
   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.
@@ -102,16 +88,60 @@ class RegistryWatcher : public base::win::ObjectWatcher::Delegate {
   }
 
  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_;
@@ -119,6 +149,27 @@ class RegistryWatcher : public base::win::ObjectWatcher::Delegate {
   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.
@@ -323,15 +374,11 @@ class DnsConfigServiceWin::ConfigReader : public SerialWorker {
         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
@@ -339,13 +386,8 @@ class DnsConfigServiceWin::ConfigReader : public SerialWorker {
     // 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;
@@ -377,31 +419,10 @@ class DnsConfigServiceWin::ConfigReader : public SerialWorker {
       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 {
@@ -410,31 +431,40 @@ class DnsConfigServiceWin::ConfigReader : public SerialWorker {
 
     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_);
@@ -461,15 +491,172 @@ class DnsConfigServiceWin::ConfigReader : public SerialWorker {
   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) {
@@ -487,31 +674,12 @@ void DnsConfigServiceWin::Watch(const CallbackType& callback) {
     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