Create a notifier class for system DNS config changes

Essentially a thread-safe notifier wrapping around DnsConfigService,
intended (but not yet used) for NetworkChangeNotifier and
HostResolverManager(s) to listen to a shared object for system DNS
config changes. In subsequent CLs, this will allow us to divorce
HostResolverManager from listening to NetworkChangeNotifier and be able
to apply Chrome-internal config changes on top of the system-only
changes currently triggerring change notifications.

In non-test, will generally be expected to be broadly shared and leaked
on shutdown.  Sequence-safe destruction is still implemented (using an
internal Core class with an OnTaskRunnerDeleter) for any special cases
and for tests.

Note that some of the internal workings of the new class work similarly
to base::ObserverListThreadSafe, but it was too difficult to make that
work with the requirement that we always be able to get an initial
config once ready, whether read before or after adding observers.

Bug: 971411
Change-Id: Iac79dba9f74018ae6b9559c27df60174bf4edc93
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1654881Reviewed-by: Matt Menke <mmenke@chromium.org>
Commit-Queue: Eric Orth <ericorth@chromium.org>
Cr-Commit-Position: refs/heads/master@{#672087}

net/dns/BUILD.gn

@@ -73,6 +73,8 @@ source_set("dns") {
       "record_rdata.cc",
       "serial_worker.cc",
       "serial_worker.h",
+      "system_dns_config_change_notifier.cc",
+      "system_dns_config_change_notifier.h",
     ]
 
     if (is_fuchsia) {
@@ -386,6 +388,7 @@ source_set("tests") {
     "record_parsed_unittest.cc",
     "record_rdata_unittest.cc",
     "serial_worker_unittest.cc",
+    "system_dns_config_change_notifier_unittest.cc",
   ]
 
   if (is_posix) {
@@ -420,10 +423,12 @@ source_set("test_support") {
   sources = [
     "dns_test_util.cc",
     "mock_host_resolver.cc",
+    "test_dns_config_service.cc",
   ]
   public = [
     "dns_test_util.h",
     "mock_host_resolver.h",
+    "test_dns_config_service.h",
   ]
 
   if (enable_mdns) {

net/dns/dns_config.cc

@@ -12,8 +12,15 @@ namespace net {
 
 // Default values are taken from glibc resolv.h except timeout which is set to
 // |kDnsDefaultTimeoutMs|.
-DnsConfig::DnsConfig()
-    : unhandled_options(false),
+DnsConfig::DnsConfig() : DnsConfig(std::vector<IPEndPoint>()) {}
+
+DnsConfig::DnsConfig(const DnsConfig& other) = default;
+
+DnsConfig::DnsConfig(DnsConfig&& other) = default;
+
+DnsConfig::DnsConfig(std::vector<IPEndPoint> nameservers)
+    : nameservers(std::move(nameservers)),
+      unhandled_options(false),
       append_to_multi_label_name(true),
       randomize_ports(false),
       ndots(1),
@@ -23,10 +30,6 @@ DnsConfig::DnsConfig()
       use_local_ipv6(false),
       secure_dns_mode(SecureDnsMode::OFF) {}
 
-DnsConfig::DnsConfig(const DnsConfig& other) = default;
-
-DnsConfig::DnsConfig(DnsConfig&& other) = default;
-
 DnsConfig::~DnsConfig() = default;
 
 DnsConfig& DnsConfig::operator=(const DnsConfig& other) = default;
@@ -37,6 +40,10 @@ bool DnsConfig::Equals(const DnsConfig& d) const {
   return EqualsIgnoreHosts(d) && (hosts == d.hosts);
 }
 
+bool DnsConfig::operator==(const DnsConfig& d) const {
+  return Equals(d);
+}
+
 bool DnsConfig::EqualsIgnoreHosts(const DnsConfig& d) const {
   return (nameservers == d.nameservers) && (search == d.search) &&
          (unhandled_options == d.unhandled_options) &&

net/dns/dns_config.h

@@ -28,12 +28,14 @@ struct NET_EXPORT DnsConfig {
   DnsConfig();
   DnsConfig(const DnsConfig& other);
   DnsConfig(DnsConfig&& other);
+  explicit DnsConfig(std::vector<IPEndPoint> nameservers);
   ~DnsConfig();
 
   DnsConfig& operator=(const DnsConfig& other);
   DnsConfig& operator=(DnsConfig&& other);
 
   bool Equals(const DnsConfig& d) const;
+  bool operator==(const DnsConfig& d) const;
 
   bool EqualsIgnoreHosts(const DnsConfig& d) const;
 

net/dns/dns_config_service.cc

@@ -16,7 +16,9 @@ DnsConfigService::DnsConfigService()
       have_config_(false),
       have_hosts_(false),
       need_update_(false),
-      last_sent_empty_(true) {}
+      last_sent_empty_(true) {
+  DETACH_FROM_SEQUENCE(sequence_checker_);
+}
 
 DnsConfigService::~DnsConfigService() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);

net/dns/dns_config_service_unittest.cc

@@ -15,6 +15,7 @@
 #include "base/test/test_timeouts.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "net/dns/public/dns_protocol.h"
+#include "net/dns/test_dns_config_service.h"
 #include "net/test/test_with_scoped_task_environment.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -31,33 +32,6 @@ class DnsConfigServiceTest : public TestWithScopedTaskEnvironment {
   }
 
  protected:
-  class TestDnsConfigService : public DnsConfigService {
-   public:
-    void ReadNow() override {}
-    bool StartWatching() override { return true; }
-
-    // Expose the protected methods to this test suite.
-    void InvalidateConfig() {
-      DnsConfigService::InvalidateConfig();
-    }
-
-    void InvalidateHosts() {
-      DnsConfigService::InvalidateHosts();
-    }
-
-    void OnConfigRead(const DnsConfig& config) {
-      DnsConfigService::OnConfigRead(config);
-    }
-
-    void OnHostsRead(const DnsHosts& hosts) {
-      DnsConfigService::OnHostsRead(hosts);
-    }
-
-    void set_watch_failed(bool value) {
-      DnsConfigService::set_watch_failed(value);
-    }
-  };
-
   void WaitForConfig(base::TimeDelta timeout) {
     base::RunLoop run_loop;
     base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(

net/dns/system_dns_config_change_notifier.cc

+// Copyright 2019 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/system_dns_config_change_notifier.h"
+
+#include <map>
+#include <utility>
+
+#include "base/bind.h"
+#include "base/location.h"
+#include "base/logging.h"
+#include "base/memory/weak_ptr.h"
+#include "base/sequence_checker.h"
+#include "base/sequenced_task_runner.h"
+#include "base/synchronization/lock.h"
+#include "base/task/post_task.h"
+#include "base/task/task_traits.h"
+#include "base/threading/sequenced_task_runner_handle.h"
+#include "net/dns/dns_config_service.h"
+
+namespace net {
+
+namespace {
+
+// Internal information and handling for a registered Observer. Handles
+// posting to and DCHECKing the correct sequence for the Observer.
+class WrappedObserver {
+ public:
+  explicit WrappedObserver(SystemDnsConfigChangeNotifier::Observer* observer)
+      : task_runner_(base::SequencedTaskRunnerHandle::Get()),
+        observer_(observer),
+        weak_ptr_factory_(this) {}
+
+  ~WrappedObserver() { DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); }
+
+  void OnNotifyThreadsafe(base::Optional<DnsConfig> config) {
+    task_runner_->PostTask(
+        FROM_HERE,
+        base::BindOnce(&WrappedObserver::OnNotify,
+                       weak_ptr_factory_.GetWeakPtr(), std::move(config)));
+  }
+
+  void OnNotify(base::Optional<DnsConfig> config) {
+    DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+    DCHECK(!config || config.value().IsValid());
+
+    observer_->OnSystemDnsConfigChanged(std::move(config));
+  }
+
+ private:
+  scoped_refptr<base::SequencedTaskRunner> task_runner_;
+  SystemDnsConfigChangeNotifier::Observer* const observer_;
+
+  SEQUENCE_CHECKER(sequence_checker_);
+  base::WeakPtrFactory<WrappedObserver> weak_ptr_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(WrappedObserver);
+};
+
+}  // namespace
+
+// Internal core to be destroyed via base::OnTaskRunnerDeleter to ensure
+// sequence safety.
+class SystemDnsConfigChangeNotifier::Core {
+ public:
+  Core(scoped_refptr<base::SequencedTaskRunner> task_runner,
+       std::unique_ptr<DnsConfigService> dns_config_service)
+      : task_runner_(std::move(task_runner)),
+        dns_config_service_(std::move(dns_config_service)),
+        weak_ptr_factory_(this) {
+    DCHECK(task_runner_);
+    DCHECK(dns_config_service_);
+
+    DETACH_FROM_SEQUENCE(sequence_checker_);
+
+    task_runner_->PostTask(
+        FROM_HERE,
+        base::BindOnce(&Core::StartWatching, weak_ptr_factory_.GetWeakPtr()));
+  }
+
+  ~Core() {
+    DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+    DCHECK(wrapped_observers_.empty());
+  }
+
+  void AddObserver(Observer* observer) {
+    // Create wrapped observer outside locking in case construction requires
+    // complex side effects.
+    auto wrapped_observer = std::make_unique<WrappedObserver>(observer);
+
+    {
+      base::AutoLock lock(lock_);
+
+      if (config_) {
+        // Even though this is the same sequence as the observer, use the
+        // threadsafe OnNotify to post the notification for both lock and
+        // reentrancy safety.
+        wrapped_observer->OnNotifyThreadsafe(config_);
+      }
+
+      DCHECK_EQ(0u, wrapped_observers_.count(observer));
+      wrapped_observers_.emplace(observer, std::move(wrapped_observer));
+    }
+  }
+
+  void RemoveObserver(Observer* observer) {
+    // Destroy wrapped observer outside locking in case destruction requires
+    // complex side effects.
+    std::unique_ptr<WrappedObserver> removed_wrapped_observer;
+
+    {
+      base::AutoLock lock(lock_);
+      auto it = wrapped_observers_.find(observer);
+      DCHECK(it != wrapped_observers_.end());
+      removed_wrapped_observer = std::move(it->second);
+      wrapped_observers_.erase(it);
+    }
+  }
+
+ private:
+  void StartWatching() {
+    DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
+    dns_config_service_->WatchConfig(base::BindRepeating(
+        &Core::OnConfigChanged, weak_ptr_factory_.GetWeakPtr()));
+  }
+
+  void OnConfigChanged(const DnsConfig& config) {
+    DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+    base::AutoLock lock(lock_);
+
+    // |config_| is |base::nullopt| if most recent config was invalid (or no
+    // valid config has yet been read), so convert |config| to a similar form
+    // before comparing for change.
+    base::Optional<DnsConfig> new_config;
+    if (config.IsValid())
+      new_config = config;
+
+    if (config_ == new_config)
+      return;
+
+    config_ = std::move(new_config);
+
+    for (auto& wrapped_observer : wrapped_observers_) {
+      wrapped_observer.second->OnNotifyThreadsafe(config_);
+    }
+  }
+
+  // Fields that may be accessed from any sequence. Must protect access using
+  // |lock_|.
+  mutable base::Lock lock_;
+  // Only stores valid configs. |base::nullopt| if most recent config was
+  // invalid (or no valid config has yet been read).
+  base::Optional<DnsConfig> config_;
+  std::map<Observer*, std::unique_ptr<WrappedObserver>> wrapped_observers_;
+
+  // Fields valid only on |task_runner_|.
+  scoped_refptr<base::SequencedTaskRunner> task_runner_;
+  SEQUENCE_CHECKER(sequence_checker_);
+  std::unique_ptr<DnsConfigService> dns_config_service_;
+  base::WeakPtrFactory<Core> weak_ptr_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(Core);
+};
+
+SystemDnsConfigChangeNotifier::SystemDnsConfigChangeNotifier()
+    : SystemDnsConfigChangeNotifier(
+          base::CreateSequencedTaskRunnerWithTraits({base::MayBlock()}),
+          DnsConfigService::CreateSystemService()) {}
+
+SystemDnsConfigChangeNotifier::SystemDnsConfigChangeNotifier(
+    scoped_refptr<base::SequencedTaskRunner> task_runner,
+    std::unique_ptr<DnsConfigService> dns_config_service)
+    : core_(new Core(task_runner, std::move(dns_config_service)),
+            base::OnTaskRunnerDeleter(task_runner)) {}
+
+SystemDnsConfigChangeNotifier::~SystemDnsConfigChangeNotifier() = default;
+
+void SystemDnsConfigChangeNotifier::AddObserver(Observer* observer) {
+  core_->AddObserver(observer);
+}
+
+void SystemDnsConfigChangeNotifier::RemoveObserver(Observer* observer) {
+  core_->RemoveObserver(observer);
+}
+
+}  // namespace net

net/dns/system_dns_config_change_notifier.h

+// Copyright 2019 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.
+
+#ifndef NET_DNS_SYSTEM_DNS_CONFIG_CHANGE_NOTIFIER_H_
+#define NET_DNS_SYSTEM_DNS_CONFIG_CHANGE_NOTIFIER_H_
+
+#include <memory>
+
+#include "base/macros.h"
+#include "base/memory/scoped_refptr.h"
+#include "base/optional.h"
+#include "base/sequenced_task_runner.h"
+#include "net/base/net_export.h"
+#include "net/dns/dns_config.h"
+
+namespace net {
+
+class DnsConfigService;
+
+// Notifier that can be subscribed to to listen for changes to system DNS
+// configuration. Expected to only be used internally to HostResolverManager and
+// NetworkChangeNotifier. Other classes are expected to subscribe to
+// NetworkChangeNotifier::AddDNSObserver() to subscribe to listen to both system
+// config changes and configuration applied on top by Chrome.
+//
+// This class is thread and sequence safe except that RemoveObserver() must be
+// called on the same sequence as the matched AddObserver() call.
+//
+// TODO(crbug.com/971411): Use this class in HostResolverManager and
+// NetworkChangeNotifier.
+class NET_EXPORT_PRIVATE SystemDnsConfigChangeNotifier {
+ public:
+  class Observer {
+   public:
+    // Called on loading new config, including the initial read once the first
+    // valid config has been read. If a config read encounters errors or an
+    // invalid config is read, will be invoked with |base::nullopt|. Only
+    // invoked when |config| changes.
+    virtual void OnSystemDnsConfigChanged(base::Optional<DnsConfig> config) = 0;
+  };
+
+  SystemDnsConfigChangeNotifier();
+  // Alternate constructor allowing specifying the underlying DnsConfigService.
+  // |dns_config_service| will only be interacted with and destroyed using
+  // |task_runner|. As required by DnsConfigService, blocking I/O may be
+  // performed on |task_runner|, so it must support blocking (i.e.
+  // base::MayBlock).
+  SystemDnsConfigChangeNotifier(
+      scoped_refptr<base::SequencedTaskRunner> task_runner,
+      std::unique_ptr<DnsConfigService> dns_config_service);
+  ~SystemDnsConfigChangeNotifier();
+
+  // An added Observer will receive notifications on the sequence where
+  // AddObserver() was called. If the config has been successfully read before
+  // calling this method, a notification will be sent for that current config
+  // before any other notifications.
+  void AddObserver(Observer* observer);
+
+  // In order to ensure notifications immediately stop on calling
+  // RemoveObserver(), must be called on the same sequence where the associated
+  // AddObserver() was called.
+  void RemoveObserver(Observer* observer);
+
+ private:
+  class Core;
+
+  std::unique_ptr<Core, base::OnTaskRunnerDeleter> core_;
+
+  DISALLOW_COPY_AND_ASSIGN(SystemDnsConfigChangeNotifier);
+};
+
+}  // namespace net
+
+#endif  // NET_DNS_SYSTEM_DNS_CONFIG_CHANGE_NOTIFIER_H_

net/dns/test_dns_config_service.cc

+// Copyright 2019 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/test_dns_config_service.h"
+
+namespace net {
+
+bool TestDnsConfigService::StartWatching() {
+  return true;
+}
+
+}  // namespace net

net/dns/test_dns_config_service.h

+// Copyright 2019 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.h"
+
+#ifndef NET_DNS_TEST_DNS_CONFIG_SERVICE_H_
+#define NET_DNS_TEST_DNS_CONFIG_SERVICE_H_
+
+namespace net {
+
+// Simple test implementation of DnsConfigService that will trigger
+// notifications only on explicitly calling On...() methods.
+class TestDnsConfigService : public DnsConfigService {
+ public:
+  void ReadNow() override {}
+  bool StartWatching() override;
+
+  // Expose the protected methods to this test suite.
+  void InvalidateConfig() { DnsConfigService::InvalidateConfig(); }
+
+  void InvalidateHosts() { DnsConfigService::InvalidateHosts(); }
+
+  void OnConfigRead(const DnsConfig& config) {
+    DnsConfigService::OnConfigRead(config);
+  }
+
+  void OnHostsRead(const DnsHosts& hosts) {
+    DnsConfigService::OnHostsRead(hosts);
+  }
+
+  void set_watch_failed(bool value) {
+    DnsConfigService::set_watch_failed(value);
+  }
+};
+
+}  // namespace net
+
+#endif  // NET_DNS_TEST_DNS_CONFIG_SERVICE_H_