Reland replace NetworkChangeNotifierWin::DnsConfigServiceThread with SequencedTaskRunner

Dedicated thread wastes resources and causes races in tests where
it can outlive thread pools.

Bug: 938126
Change-Id: I335d719d72c6291c03ef33480a81b7cda719b877
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1609883
Auto-Submit: Paul Jensen <pauljensen@chromium.org>
Commit-Queue: Eric Orth <ericorth@chromium.org>
Reviewed-by: Eric Orth <ericorth@chromium.org>
Cr-Commit-Position: refs/heads/master@{#659155}

net/base/network_change_notifier_win.cc

@@ -12,7 +12,10 @@
 #include "base/logging.h"
 #include "base/macros.h"
 #include "base/metrics/histogram_macros.h"
+#include "base/sequenced_task_runner.h"
 #include "base/single_thread_task_runner.h"
+#include "base/task/post_task.h"
+#include "base/task/task_traits.h"
 #include "base/task_runner_util.h"
 #include "base/threading/thread.h"
 #include "base/threading/thread_task_runner_handle.h"
@@ -30,32 +33,18 @@ const int kWatchForAddressChangeRetryIntervalMs = 500;
 
 }  // namespace
 
-// Thread on which we can run DnsConfigService, which requires AssertIOAllowed
-// to open registry keys and to handle FilePathWatcher updates.
-class NetworkChangeNotifierWin::DnsConfigServiceThread : public base::Thread {
- public:
-  DnsConfigServiceThread() : base::Thread("DnsConfigService") {}
-
-  ~DnsConfigServiceThread() override { Stop(); }
-
-  void Init() override {
-    service_ = DnsConfigService::CreateSystemService();
-    service_->WatchConfig(base::Bind(&NetworkChangeNotifier::SetDnsConfig));
-  }
-
-  void CleanUp() override { service_.reset(); }
-
- private:
-  std::unique_ptr<DnsConfigService> service_;
-
-  DISALLOW_COPY_AND_ASSIGN(DnsConfigServiceThread);
-};
-
 NetworkChangeNotifierWin::NetworkChangeNotifierWin()
     : NetworkChangeNotifier(NetworkChangeCalculatorParamsWin()),
       is_watching_(false),
       sequential_failures_(0),
-      dns_config_service_thread_(new DnsConfigServiceThread()),
+      dns_config_service_runner_(
+          base::CreateSequencedTaskRunnerWithTraits({base::MayBlock()})),
+      dns_config_service_(
+          DnsConfigService::CreateSystemService().release(),
+          // Ensure DnsConfigService lives on |dns_config_service_runner_|
+          // to prevent races where NetworkChangeNotifierWin outlives
+          // ScopedTaskEnvironment. https://crbug.com/938126
+          base::OnTaskRunnerDeleter(dns_config_service_runner_)),
       last_computed_connection_type_(RecomputeCurrentConnectionType()),
       last_announced_offline_(last_computed_connection_type_ ==
                               CONNECTION_NONE),
@@ -65,7 +54,7 @@ NetworkChangeNotifierWin::NetworkChangeNotifierWin()
 }
 
 NetworkChangeNotifierWin::~NetworkChangeNotifierWin() {
-  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   if (is_watching_) {
     CancelIPChangeNotify(&addr_overlapped_);
     addr_watcher_.StopWatching();
@@ -204,15 +193,15 @@ NetworkChangeNotifierWin::RecomputeCurrentConnectionType() const {
                           : NetworkChangeNotifier::CONNECTION_NONE;
 }
 
-void NetworkChangeNotifierWin::RecomputeCurrentConnectionTypeOnDnsThread(
-    base::Callback<void(ConnectionType)> reply_callback) const {
+void NetworkChangeNotifierWin::RecomputeCurrentConnectionTypeOnDnsSequence(
+    base::OnceCallback<void(ConnectionType)> reply_callback) const {
   // Unretained is safe in this call because this object owns the thread and the
   // thread is stopped in this object's destructor.
   base::PostTaskAndReplyWithResult(
-      dns_config_service_thread_->task_runner().get(), FROM_HERE,
-      base::Bind(&NetworkChangeNotifierWin::RecomputeCurrentConnectionType,
-                 base::Unretained(this)),
-      reply_callback);
+      dns_config_service_runner_.get(), FROM_HERE,
+      base::BindOnce(&NetworkChangeNotifierWin::RecomputeCurrentConnectionType,
+                     base::Unretained(this)),
+      std::move(reply_callback));
 }
 
 NetworkChangeNotifier::ConnectionType
@@ -228,19 +217,19 @@ void NetworkChangeNotifierWin::SetCurrentConnectionType(
 }
 
 void NetworkChangeNotifierWin::OnObjectSignaled(HANDLE object) {
-  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   DCHECK(is_watching_);
   is_watching_ = false;
 
   // Start watching for the next address change.
   WatchForAddressChange();
 
-  RecomputeCurrentConnectionTypeOnDnsThread(base::Bind(
+  RecomputeCurrentConnectionTypeOnDnsSequence(base::BindOnce(
       &NetworkChangeNotifierWin::NotifyObservers, weak_factory_.GetWeakPtr()));
 }
 
 void NetworkChangeNotifierWin::NotifyObservers(ConnectionType connection_type) {
-  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   SetCurrentConnectionType(connection_type);
   NotifyObserversOfIPAddressChange();
 
@@ -257,7 +246,7 @@ void NetworkChangeNotifierWin::NotifyObservers(ConnectionType connection_type) {
 }
 
 void NetworkChangeNotifierWin::WatchForAddressChange() {
-  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   DCHECK(!is_watching_);
 
   // NotifyAddrChange occasionally fails with ERROR_OPEN_FAILED for unknown
@@ -287,7 +276,7 @@ void NetworkChangeNotifierWin::WatchForAddressChange() {
   // network change event, since network changes were not being observed in
   // that interval.
   if (sequential_failures_ > 0) {
-    RecomputeCurrentConnectionTypeOnDnsThread(
+    RecomputeCurrentConnectionTypeOnDnsSequence(
         base::Bind(&NetworkChangeNotifierWin::NotifyObservers,
                    weak_factory_.GetWeakPtr()));
   }
@@ -302,12 +291,13 @@ void NetworkChangeNotifierWin::WatchForAddressChange() {
 }
 
 bool NetworkChangeNotifierWin::WatchForAddressChangeInternal() {
-  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
 
-  if (!dns_config_service_thread_->IsRunning()) {
-    dns_config_service_thread_->StartWithOptions(
-      base::Thread::Options(base::MessageLoop::TYPE_IO, 0));
-  }
+  dns_config_service_runner_->PostTask(
+      FROM_HERE, base::BindOnce(&DnsConfigService::WatchConfig,
+                                base::Unretained(dns_config_service_.get()),
+                                base::BindRepeating(
+                                    &NetworkChangeNotifier::SetDnsConfig)));
 
   ResetEventIfSignaled(addr_overlapped_.hEvent);
   HANDLE handle = nullptr;
@@ -320,7 +310,7 @@ bool NetworkChangeNotifierWin::WatchForAddressChangeInternal() {
 }
 
 void NetworkChangeNotifierWin::NotifyParentOfConnectionTypeChange() {
-  RecomputeCurrentConnectionTypeOnDnsThread(base::Bind(
+  RecomputeCurrentConnectionTypeOnDnsSequence(base::BindOnce(
       &NetworkChangeNotifierWin::NotifyParentOfConnectionTypeChangeImpl,
       weak_factory_.GetWeakPtr()));
 }

net/base/network_change_notifier_win.h

@@ -12,17 +12,25 @@
 #include "base/callback.h"
 #include "base/compiler_specific.h"
 #include "base/macros.h"
+#include "base/memory/scoped_refptr.h"
 #include "base/memory/weak_ptr.h"
-#include "base/threading/thread_checker.h"
+#include "base/sequence_checker.h"
 #include "base/timer/timer.h"
 #include "base/win/object_watcher.h"
 #include "net/base/net_export.h"
 #include "net/base/network_change_notifier.h"
 
+namespace base {
+class SequencedTaskRunner;
+struct OnTaskRunnerDeleter;
+}  // namespace base
+
 namespace net {
 
-// NetworkChangeNotifierWin uses a ThreadChecker, as all its internal
-// notification code must be called on the thread it is created and destroyed
+class DnsConfigService;
+
+// NetworkChangeNotifierWin uses a SequenceChecker, as all its internal
+// notification code must be called on the sequence it is created and destroyed
 // on.  All the NetworkChangeNotifier methods it implements are threadsafe.
 class NET_EXPORT_PRIVATE NetworkChangeNotifierWin
     : public NetworkChangeNotifier,
@@ -32,8 +40,8 @@ class NET_EXPORT_PRIVATE NetworkChangeNotifierWin
 
   // Begins listening for a single subsequent address change.  If it fails to
   // start watching, it retries on a timer.  Must be called only once, on the
-  // thread |this| was created on.  This cannot be called in the constructor, as
-  // WatchForAddressChangeInternal is mocked out in unit tests.
+  // sequence |this| was created on.  This cannot be called in the constructor,
+  // as WatchForAddressChangeInternal is mocked out in unit tests.
   // TODO(mmenke): Consider making this function a part of the
   //               NetworkChangeNotifier interface, so other subclasses can be
   //               unit tested in similar fashion, as needed.
@@ -48,30 +56,29 @@ class NET_EXPORT_PRIVATE NetworkChangeNotifierWin
   int sequential_failures() { return sequential_failures_; }
 
  private:
-  class DnsConfigServiceThread;
   friend class NetworkChangeNotifierWinTest;
 
   // NetworkChangeNotifier methods:
   ConnectionType GetCurrentConnectionType() const override;
 
   // ObjectWatcher::Delegate methods:
-  // Must only be called on the thread |this| was created on.
+  // Must only be called on the sequence |this| was created on.
   void OnObjectSignaled(HANDLE object) override;
 
   // Does the actual work to determine the current connection type.
   // It is not thread safe, see crbug.com/324913.
   virtual ConnectionType RecomputeCurrentConnectionType() const;
 
-  // Calls RecomputeCurrentConnectionTypeImpl on the DNS thread and runs
-  // |reply_callback| with the type on the calling thread.
-  virtual void RecomputeCurrentConnectionTypeOnDnsThread(
-      base::Callback<void(ConnectionType)> reply_callback) const;
+  // Calls RecomputeCurrentConnectionTypeImpl on the DNS sequence and runs
+  // |reply_callback| with the type on the calling sequence.
+  virtual void RecomputeCurrentConnectionTypeOnDnsSequence(
+      base::OnceCallback<void(ConnectionType)> reply_callback) const;
 
   void SetCurrentConnectionType(ConnectionType connection_type);
 
   // Notifies IP address change observers of a change immediately, and notifies
   // network state change observers on a delay.  Must only be called on the
-  // thread |this| was created on.
+  // sequence |this| was created on.
   void NotifyObservers(ConnectionType connection_type);
 
   // Forwards connection type notifications to parent class.
@@ -80,14 +87,14 @@ class NET_EXPORT_PRIVATE NetworkChangeNotifierWin
 
   // Tries to start listening for a single subsequent address change.  Returns
   // false on failure.  The caller is responsible for updating |is_watching_|.
-  // Virtual for unit tests.  Must only be called on the thread |this| was
+  // Virtual for unit tests.  Must only be called on the sequence |this| was
   // created on.
   virtual bool WatchForAddressChangeInternal();
 
   static NetworkChangeCalculatorParams NetworkChangeCalculatorParamsWin();
 
-  // All member variables may only be accessed on the thread |this| was created
-  // on.
+  // All member variables may only be accessed on the sequence |this| was
+  // created on.
 
   // False when not currently watching for network change events.  This only
   // happens on initialization and when WatchForAddressChangeInternal fails and
@@ -102,8 +109,11 @@ class NET_EXPORT_PRIVATE NetworkChangeNotifierWin
   // Number of times WatchForAddressChange has failed in a row.
   int sequential_failures_;
 
-  // Thread on which we can run DnsConfigService.
-  std::unique_ptr<DnsConfigServiceThread> dns_config_service_thread_;
+  // |dns_config_service_| will live on this runner.
+  scoped_refptr<base::SequencedTaskRunner> dns_config_service_runner_;
+  // DnsConfigService that lives on |dns_config_service_runner_|.
+  std::unique_ptr<DnsConfigService, base::OnTaskRunnerDeleter>
+      dns_config_service_;
 
   mutable base::Lock last_computed_connection_type_lock_;
   ConnectionType last_computed_connection_type_;
@@ -114,7 +124,7 @@ class NET_EXPORT_PRIVATE NetworkChangeNotifierWin
   // Number of times polled to check if still offline.
   int offline_polls_;
 
-  THREAD_CHECKER(thread_checker_);
+  SEQUENCE_CHECKER(sequence_checker_);
 
   // Used for calling WatchForAddressChange again on failure.
   base::WeakPtrFactory<NetworkChangeNotifierWin> weak_factory_;

net/base/network_change_notifier_win_unittest.cc

@@ -43,10 +43,10 @@ class TestNetworkChangeNotifierWin : public NetworkChangeNotifierWin {
   }
 
   // From NetworkChangeNotifierWin.
-  void RecomputeCurrentConnectionTypeOnDnsThread(
-      base::Callback<void(ConnectionType)> reply_callback) const override {
+  void RecomputeCurrentConnectionTypeOnDnsSequence(
+      base::OnceCallback<void(ConnectionType)> reply_callback) const override {
     base::ThreadTaskRunnerHandle::Get()->PostTask(
-        FROM_HERE, base::BindOnce(reply_callback,
+        FROM_HERE, base::BindOnce(std::move(reply_callback),
                                   NetworkChangeNotifier::CONNECTION_UNKNOWN));
   }
 

net/dns/dns_config_service_win.cc

@@ -18,14 +18,14 @@
 #include "base/macros.h"
 #include "base/memory/free_deleter.h"
 #include "base/metrics/histogram_macros.h"
+#include "base/sequence_checker.h"
 #include "base/single_thread_task_runner.h"
 #include "base/strings/string_split.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/synchronization/lock.h"
 #include "base/threading/scoped_blocking_call.h"
-#include "base/threading/thread_checker.h"
-#include "base/threading/thread_task_runner_handle.h"
+#include "base/threading/sequenced_task_runner_handle.h"
 #include "base/time/time.h"
 #include "base/win/registry.h"
 #include "base/win/scoped_handle.h"
@@ -76,11 +76,11 @@ class RegistryReader {
     key_.Open(HKEY_LOCAL_MACHINE, key, KEY_QUERY_VALUE);
   }
 
-  ~RegistryReader() { DCHECK_CALLED_ON_VALID_THREAD(thread_checker_); }
+  ~RegistryReader() { DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); }
 
   bool ReadString(const base::char16* name,
                   DnsSystemSettings::RegString* out) const {
-    DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+    DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
     out->set = false;
     if (!key_.Valid()) {
       // Assume that if the |key_| is invalid then the key is missing.
@@ -96,7 +96,7 @@ class RegistryReader {
 
   bool ReadDword(const base::char16* name,
                  DnsSystemSettings::RegDword* out) const {
-    DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+    DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
     out->set = false;
     if (!key_.Valid()) {
       // Assume that if the |key_| is invalid then the key is missing.
@@ -113,7 +113,7 @@ class RegistryReader {
  private:
   base::win::RegKey key_;
 
-  THREAD_CHECKER(thread_checker_);
+  SEQUENCE_CHECKER(sequence_checker_);
 
   DISALLOW_COPY_AND_ASSIGN(RegistryReader);
 };
@@ -275,10 +275,10 @@ class RegistryWatcher {
   typedef base::Callback<void(bool succeeded)> CallbackType;
   RegistryWatcher() {}
 
-  ~RegistryWatcher() { DCHECK_CALLED_ON_VALID_THREAD(thread_checker_); }
+  ~RegistryWatcher() { DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); }
 
   bool Watch(const base::char16* key, const CallbackType& callback) {
-    DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+    DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
     DCHECK(!callback.is_null());
     DCHECK(callback_.is_null());
     callback_ = callback;
@@ -290,7 +290,7 @@ class RegistryWatcher {
   }
 
   void OnObjectSignaled() {
-    DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+    DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
     DCHECK(!callback_.is_null());
     if (key_.StartWatching(base::Bind(&RegistryWatcher::OnObjectSignaled,
                                       base::Unretained(this)))) {
@@ -305,7 +305,7 @@ class RegistryWatcher {
   CallbackType callback_;
   base::win::RegKey key_;
 
-  THREAD_CHECKER(thread_checker_);
+  SEQUENCE_CHECKER(sequence_checker_);
 
   DISALLOW_COPY_AND_ASSIGN(RegistryWatcher);
 };
@@ -564,7 +564,7 @@ ConfigParseWinResult ConvertSettingsToDnsConfig(
   return result;
 }
 
-// Watches registry and HOSTS file for changes. Must live on a thread which
+// Watches registry and HOSTS file for changes. Must live on a sequence which
 // allows IO.
 class DnsConfigServiceWin::Watcher
     : public NetworkChangeNotifier::IPAddressObserver {
@@ -671,7 +671,7 @@ class DnsConfigServiceWin::ConfigReader : public SerialWorker {
     } else {
       LOG(WARNING) << "Failed to read DnsConfig.";
       // Try again in a while in case DnsConfigWatcher missed the signal.
-      base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
+      base::SequencedTaskRunnerHandle::Get()->PostDelayedTask(
           FROM_HERE, base::BindOnce(&ConfigReader::WorkNow, this),
           base::TimeDelta::FromSeconds(kRetryIntervalSeconds));
     }
@@ -729,13 +729,16 @@ class DnsConfigServiceWin::HostsReader : public SerialWorker {
   DISALLOW_COPY_AND_ASSIGN(HostsReader);
 };
 
-DnsConfigServiceWin::DnsConfigServiceWin()
-    : config_reader_(new ConfigReader(this)),
-      hosts_reader_(new HostsReader(this)) {}
+DnsConfigServiceWin::DnsConfigServiceWin() {
+  // Allow constructing on one sequence and living on another.
+  DETACH_FROM_SEQUENCE(sequence_checker_);
+}
 
 DnsConfigServiceWin::~DnsConfigServiceWin() {
-  config_reader_->Cancel();
-  hosts_reader_->Cancel();
+  if (config_reader_)
+    config_reader_->Cancel();
+  if (hosts_reader_)
+    hosts_reader_->Cancel();
 }
 
 void DnsConfigServiceWin::ReadNow() {
@@ -744,6 +747,10 @@ void DnsConfigServiceWin::ReadNow() {
 }
 
 bool DnsConfigServiceWin::StartWatching() {
+  if (!config_reader_)
+    config_reader_ = base::MakeRefCounted<ConfigReader>(this);
+  if (!hosts_reader_)
+    hosts_reader_ = base::MakeRefCounted<HostsReader>(this);
   // TODO(szym): re-start watcher if that makes sense. http://crbug.com/116139
   watcher_.reset(new Watcher(this));
   UMA_HISTOGRAM_ENUMERATION("AsyncDNS.WatchStatus", DNS_CONFIG_WATCH_STARTED,

net/dns/dns_config_service_win.h

@@ -124,6 +124,11 @@ ConfigParseWinResult NET_EXPORT_PRIVATE ConvertSettingsToDnsConfig(
     const DnsSystemSettings& settings,
     DnsConfig* dns_config);
 
+// Service for reading and watching Windows system DNS settings. This object is
+// not thread-safe and methods may perform blocking I/O so methods must be
+// called on a sequence that allows blocking (i.e. base::MayBlock). It may be
+// constructed on a different sequence than which it's later called on.
+// WatchConfig() must be called prior to ReadConfig().
 // Use DnsConfigService::CreateSystemService to use it outside of tests.
 class NET_EXPORT_PRIVATE DnsConfigServiceWin : public DnsConfigService {
  public: