Replace NetworkChangeNotifierLinux::Thread with SequencedTaskRunner

Dedicated thread wastes resources and causes races in tests where
it can outlive thread pools used by DnsConfigServicePosix's
SerialWorkers.

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

net/android/network_change_notifier_android.cc

@@ -67,9 +67,12 @@
 #include "base/macros.h"
 #include "base/message_loop/message_loop.h"
 #include "base/metrics/histogram_macros.h"
+#include "base/sequenced_task_runner.h"
+#include "base/task/post_task.h"
+#include "base/task/task_traits.h"
 #include "base/threading/thread.h"
 #include "net/base/address_tracker_linux.h"
-#include "net/dns/dns_config_service.h"
+#include "net/dns/dns_config_service_posix.h"
 
 namespace net {
 
@@ -86,41 +89,32 @@ enum NetId {
 
 // Thread on which we can run DnsConfigService, which requires a TYPE_IO
 // message loop to monitor /system/etc/hosts.
-class NetworkChangeNotifierAndroid::DnsConfigServiceThread
-    : public base::Thread {
+class NetworkChangeNotifierAndroid::BlockingThreadObjects {
  public:
-  DnsConfigServiceThread()
-      : base::Thread("DnsConfigService"),
-        address_tracker_(base::DoNothing(),
+  BlockingThreadObjects()
+      : address_tracker_(base::DoNothing(),
                          base::DoNothing(),
                          // We're only interested in tunnel interface changes.
                          base::Bind(NotifyNetworkChangeNotifierObservers),
                          std::unordered_set<std::string>()) {}
 
-  ~DnsConfigServiceThread() override {
-    Stop();
-  }
-
-  void Init() override {
+  void Init() {
     address_tracker_.Init();
-    dns_config_service_ = DnsConfigService::CreateSystemService();
-    dns_config_service_->WatchConfig(
+    dns_config_service_.WatchConfig(
         base::Bind(&NetworkChangeNotifier::SetDnsConfig));
   }
 
-  void CleanUp() override { dns_config_service_.reset(); }
-
   static void NotifyNetworkChangeNotifierObservers() {
     NetworkChangeNotifier::NotifyObserversOfIPAddressChange();
     NetworkChangeNotifier::NotifyObserversOfConnectionTypeChange();
   }
 
  private:
-  std::unique_ptr<DnsConfigService> dns_config_service_;
+  internal::DnsConfigServicePosix dns_config_service_;
   // Used to detect tunnel state changes.
   internal::AddressTrackerLinux address_tracker_;
 
-  DISALLOW_COPY_AND_ASSIGN(DnsConfigServiceThread);
+  DISALLOW_COPY_AND_ASSIGN(BlockingThreadObjects);
 };
 
 NetworkChangeNotifierAndroid::~NetworkChangeNotifierAndroid() {
@@ -175,7 +169,7 @@ NetworkChangeNotifierAndroid::GetCurrentDefaultNetwork() const {
 }
 
 void NetworkChangeNotifierAndroid::OnConnectionTypeChanged() {
-  DnsConfigServiceThread::NotifyNetworkChangeNotifierObservers();
+  BlockingThreadObjects::NotifyNetworkChangeNotifierObservers();
 }
 
 void NetworkChangeNotifierAndroid::OnMaxBandwidthChanged(
@@ -211,13 +205,25 @@ NetworkChangeNotifierAndroid::NetworkChangeNotifierAndroid(
     NetworkChangeNotifierDelegateAndroid* delegate)
     : NetworkChangeNotifier(NetworkChangeCalculatorParamsAndroid()),
       delegate_(delegate),
-      dns_config_service_thread_(new DnsConfigServiceThread()),
+      blocking_thread_runner_(
+          base::CreateSequencedTaskRunnerWithTraits({base::MayBlock()})),
+      blocking_thread_objects_(
+          new BlockingThreadObjects(),
+          // Ensure |blocking_thread_objects_| lives on
+          // |blocking_thread_runner_| to prevent races where
+          // NetworkChangeNotifierAndroid outlives
+          // ScopedTaskEnvironment. https://crbug.com/938126
+          base::OnTaskRunnerDeleter(blocking_thread_runner_)),
       force_network_handles_supported_for_testing_(false) {
   CHECK_EQ(NetId::INVALID, NetworkChangeNotifier::kInvalidNetworkHandle)
       << "kInvalidNetworkHandle doesn't match NetId::INVALID";
   delegate_->AddObserver(this);
-  dns_config_service_thread_->StartWithOptions(
-      base::Thread::Options(base::MessageLoop::TYPE_IO, 0));
+  blocking_thread_runner_->PostTask(
+      FROM_HERE,
+      base::BindOnce(&BlockingThreadObjects::Init,
+                     // The Unretained pointer is safe here because it's
+                     // posted before the deleter can post.
+                     base::Unretained(blocking_thread_objects_.get())));
 }
 
 // static

net/android/network_change_notifier_android.h

@@ -10,10 +10,16 @@
 #include "base/android/jni_android.h"
 #include "base/compiler_specific.h"
 #include "base/macros.h"
+#include "base/memory/scoped_refptr.h"
 #include "net/android/network_change_notifier_delegate_android.h"
 #include "net/base/net_export.h"
 #include "net/base/network_change_notifier.h"
 
+namespace base {
+class SequencedTaskRunner;
+struct OnTaskRunnerDeleter;
+}  // namespace base
+
 namespace net {
 
 class NetworkChangeNotifierAndroidTest;
@@ -81,7 +87,7 @@ class NET_EXPORT_PRIVATE NetworkChangeNotifierAndroid
   friend class NetworkChangeNotifierAndroidTest;
   friend class NetworkChangeNotifierFactoryAndroid;
 
-  class DnsConfigServiceThread;
+  class BlockingThreadObjects;
 
   // Enable NetworkHandles support for tests.
   void ForceNetworkHandlesSupportedForTesting();
@@ -90,7 +96,14 @@ class NET_EXPORT_PRIVATE NetworkChangeNotifierAndroid
       NetworkChangeNotifierDelegateAndroid* delegate);
 
   NetworkChangeNotifierDelegateAndroid* const delegate_;
-  std::unique_ptr<DnsConfigServiceThread> dns_config_service_thread_;
+  // |blocking_thread_objects_| will live on this runner.
+  scoped_refptr<base::SequencedTaskRunner> blocking_thread_runner_;
+  // A collection of objects that must live on blocking sequences. These objects
+  // listen for notifications and relay the notifications to the registered
+  // observers without posting back to the thread the object was created on.
+  // Also used for DnsConfigService which also must live on blocking sequences.
+  std::unique_ptr<BlockingThreadObjects, base::OnTaskRunnerDeleter>
+      blocking_thread_objects_;
   bool force_network_handles_supported_for_testing_;
 
   DISALLOW_COPY_AND_ASSIGN(NetworkChangeNotifierAndroid);

net/base/address_tracker_linux.cc

@@ -118,7 +118,6 @@ AddressTrackerLinux::AddressTrackerLinux()
       address_callback_(base::DoNothing()),
       link_callback_(base::DoNothing()),
       tunnel_callback_(base::DoNothing()),
-      watcher_(FROM_HERE),
       ignored_interfaces_(),
       connection_type_initialized_(false),
       connection_type_initialized_cv_(&connection_type_lock_),
@@ -135,7 +134,6 @@ AddressTrackerLinux::AddressTrackerLinux(
       address_callback_(address_callback),
       link_callback_(link_callback),
       tunnel_callback_(tunnel_callback),
-      watcher_(FROM_HERE),
       ignored_interfaces_(ignored_interfaces),
       connection_type_initialized_(false),
       connection_type_initialized_cv_(&connection_type_lock_),
@@ -146,9 +144,7 @@ AddressTrackerLinux::AddressTrackerLinux(
   DCHECK(!link_callback.is_null());
 }
 
-AddressTrackerLinux::~AddressTrackerLinux() {
-  watcher_.StopWatchingFileDescriptor();
-}
+AddressTrackerLinux::~AddressTrackerLinux() = default;
 
 void AddressTrackerLinux::Init() {
   netlink_fd_.reset(socket(AF_NETLINK, SOCK_RAW, NETLINK_ROUTE));
@@ -230,19 +226,15 @@ void AddressTrackerLinux::Init() {
   }
 
   if (tracking_) {
-    rv = base::MessageLoopCurrentForIO::Get()->WatchFileDescriptor(
-        netlink_fd_.get(), true, base::MessagePumpForIO::WATCH_READ, &watcher_,
-        this);
-    if (rv < 0) {
-      PLOG(ERROR) << "Could not watch NETLINK socket";
-      AbortAndForceOnline();
-      return;
-    }
+    watcher_ = base::FileDescriptorWatcher::WatchReadable(
+        netlink_fd_.get(),
+        base::BindRepeating(&AddressTrackerLinux::OnFileCanReadWithoutBlocking,
+                            base::Unretained(this)));
   }
 }
 
 void AddressTrackerLinux::AbortAndForceOnline() {
-  watcher_.StopWatchingFileDescriptor();
+  watcher_.reset();
   netlink_fd_.reset();
   AddressTrackerAutoLock lock(*this, connection_type_lock_);
   current_connection_type_ = NetworkChangeNotifier::CONNECTION_UNKNOWN;
@@ -424,8 +416,7 @@ void AddressTrackerLinux::HandleMessage(char* buffer,
   }
 }
 
-void AddressTrackerLinux::OnFileCanReadWithoutBlocking(int fd) {
-  DCHECK_EQ(netlink_fd_.get(), fd);
+void AddressTrackerLinux::OnFileCanReadWithoutBlocking() {
   bool address_changed;
   bool link_changed;
   bool tunnel_changed;
@@ -438,8 +429,6 @@ void AddressTrackerLinux::OnFileCanReadWithoutBlocking(int fd) {
     tunnel_callback_.Run();
 }
 
-void AddressTrackerLinux::OnFileCanWriteWithoutBlocking(int /* fd */) {}
-
 bool AddressTrackerLinux::IsTunnelInterface(int interface_index) const {
   char buf[IFNAMSIZ] = {0};
   return IsTunnelInterfaceName(get_interface_name_(interface_index, buf));

net/base/address_tracker_linux.h

@@ -17,9 +17,9 @@
 
 #include "base/callback.h"
 #include "base/compiler_specific.h"
+#include "base/files/file_descriptor_watcher_posix.h"
 #include "base/files/scoped_file.h"
 #include "base/macros.h"
-#include "base/message_loop/message_pump_for_io.h"
 #include "base/synchronization/condition_variable.h"
 #include "base/synchronization/lock.h"
 #include "base/threading/thread_checker.h"
@@ -32,8 +32,7 @@ namespace internal {
 
 // Keeps track of network interface addresses using rtnetlink. Used by
 // NetworkChangeNotifier to provide signals to registered IPAddressObservers.
-class NET_EXPORT_PRIVATE AddressTrackerLinux
-    : public base::MessagePumpForIO::FdWatcher {
+class NET_EXPORT_PRIVATE AddressTrackerLinux {
  public:
   typedef std::map<IPAddress, struct ifaddrmsg> AddressMap;
 
@@ -58,7 +57,7 @@ class NET_EXPORT_PRIVATE AddressTrackerLinux
       const base::Closure& link_callback,
       const base::Closure& tunnel_callback,
       const std::unordered_set<std::string>& ignored_interfaces);
-  ~AddressTrackerLinux() override;
+  virtual ~AddressTrackerLinux();
 
   // In tracking mode, it starts watching the system configuration for
   // changes. The current thread must have a MessageLoopForIO. In
@@ -129,9 +128,8 @@ class NET_EXPORT_PRIVATE AddressTrackerLinux
   // Call when some part of initialization failed; forces online and unblocks.
   void AbortAndForceOnline();
 
-  // MessagePumpForIO::FdWatcher:
-  void OnFileCanReadWithoutBlocking(int fd) override;
-  void OnFileCanWriteWithoutBlocking(int /* fd */) override;
+  // Called by |watcher_| when |netlink_fd_| can be read without blocking.
+  void OnFileCanReadWithoutBlocking();
 
   // Does |interface_index| refer to a tunnel interface?
   bool IsTunnelInterface(int interface_index) const;
@@ -157,7 +155,7 @@ class NET_EXPORT_PRIVATE AddressTrackerLinux
 
   // Note that |watcher_| must be inactive when |netlink_fd_| is closed.
   base::ScopedFD netlink_fd_;
-  base::MessagePumpForIO::FdWatchController watcher_;
+  std::unique_ptr<base::FileDescriptorWatcher::Controller> watcher_;
 
   mutable base::Lock address_map_lock_;
   AddressMap address_map_;

net/base/network_change_notifier_linux.cc

@@ -9,83 +9,73 @@
 #include "base/compiler_specific.h"
 #include "base/macros.h"
 #include "base/message_loop/message_loop.h"
+#include "base/sequenced_task_runner.h"
+#include "base/task/post_task.h"
+#include "base/task/task_traits.h"
 #include "base/threading/thread.h"
 #include "net/base/address_tracker_linux.h"
-#include "net/dns/dns_config_service.h"
+#include "net/dns/dns_config_service_posix.h"
 
 namespace net {
 
-class NetworkChangeNotifierLinux::Thread : public base::Thread {
+// A collection of objects that live on blocking threads.
+class NetworkChangeNotifierLinux::BlockingThreadObjects {
  public:
-  explicit Thread(const std::unordered_set<std::string>& ignored_interfaces);
-  ~Thread() override;
+  explicit BlockingThreadObjects(
+      const std::unordered_set<std::string>& ignored_interfaces);
 
   // Plumbing for NetworkChangeNotifier::GetCurrentConnectionType.
   // Safe to call from any thread.
   NetworkChangeNotifier::ConnectionType GetCurrentConnectionType() {
-    return address_tracker_->GetCurrentConnectionType();
+    return address_tracker_.GetCurrentConnectionType();
   }
 
   const internal::AddressTrackerLinux* address_tracker() const {
-    return address_tracker_.get();
+    return &address_tracker_;
   }
 
- protected:
-  // base::Thread
-  void Init() override;
-  void CleanUp() override;
+  // Begin watching for DNS and netlink changes.
+  void Init();
 
  private:
   void OnIPAddressChanged();
   void OnLinkChanged();
-  std::unique_ptr<DnsConfigService> dns_config_service_;
+  internal::DnsConfigServicePosix dns_config_service_;
   // Used to detect online/offline state and IP address changes.
-  std::unique_ptr<internal::AddressTrackerLinux> address_tracker_;
+  internal::AddressTrackerLinux address_tracker_;
   NetworkChangeNotifier::ConnectionType last_type_;
 
-  DISALLOW_COPY_AND_ASSIGN(Thread);
+  DISALLOW_COPY_AND_ASSIGN(BlockingThreadObjects);
 };
 
-NetworkChangeNotifierLinux::Thread::Thread(
+NetworkChangeNotifierLinux::BlockingThreadObjects::BlockingThreadObjects(
     const std::unordered_set<std::string>& ignored_interfaces)
-    : base::Thread("NetworkChangeNotifier"),
-      address_tracker_(new internal::AddressTrackerLinux(
-          base::Bind(&NetworkChangeNotifierLinux::Thread::OnIPAddressChanged,
-                     base::Unretained(this)),
-          base::Bind(&NetworkChangeNotifierLinux::Thread::OnLinkChanged,
-                     base::Unretained(this)),
+    : address_tracker_(
+          base::BindRepeating(&NetworkChangeNotifierLinux::
+                                  BlockingThreadObjects::OnIPAddressChanged,
+                              base::Unretained(this)),
+          base::BindRepeating(
+              &NetworkChangeNotifierLinux::BlockingThreadObjects::OnLinkChanged,
+              base::Unretained(this)),
           base::DoNothing(),
-          ignored_interfaces)),
+          ignored_interfaces),
       last_type_(NetworkChangeNotifier::CONNECTION_NONE) {}
 
-NetworkChangeNotifierLinux::Thread::~Thread() {
-  DCHECK(!Thread::IsRunning());
-}
-
-void NetworkChangeNotifierLinux::Thread::Init() {
-  address_tracker_->Init();
+void NetworkChangeNotifierLinux::BlockingThreadObjects::Init() {
+  address_tracker_.Init();
   last_type_ = GetCurrentConnectionType();
-  dns_config_service_ = DnsConfigService::CreateSystemService();
-  dns_config_service_->WatchConfig(
+  dns_config_service_.WatchConfig(
       base::Bind(&NetworkChangeNotifier::SetDnsConfig));
 }
 
-void NetworkChangeNotifierLinux::Thread::CleanUp() {
-  // Delete AddressTrackerLinux before MessageLoop gets deleted as
-  // AddressTrackerLinux's FileDescriptorWatcher holds a pointer to the
-  // MessageLoop.
-  address_tracker_.reset();
-  dns_config_service_.reset();
-}
-
-void NetworkChangeNotifierLinux::Thread::OnIPAddressChanged() {
+void NetworkChangeNotifierLinux::BlockingThreadObjects::OnIPAddressChanged() {
   NetworkChangeNotifier::NotifyObserversOfIPAddressChange();
   // When the IP address of a network interface is added/deleted, the
   // connection type may have changed.
   OnLinkChanged();
 }
 
-void NetworkChangeNotifierLinux::Thread::OnLinkChanged() {
+void NetworkChangeNotifierLinux::BlockingThreadObjects::OnLinkChanged() {
   if (last_type_ != GetCurrentConnectionType()) {
     NetworkChangeNotifier::NotifyObserversOfConnectionTypeChange();
     last_type_ = GetCurrentConnectionType();
@@ -100,19 +90,24 @@ void NetworkChangeNotifierLinux::Thread::OnLinkChanged() {
 NetworkChangeNotifierLinux::NetworkChangeNotifierLinux(
     const std::unordered_set<std::string>& ignored_interfaces)
     : NetworkChangeNotifier(NetworkChangeCalculatorParamsLinux()),
-      notifier_thread_(new Thread(ignored_interfaces)) {
-  // We create this notifier thread because the notification implementation
-  // needs a MessageLoopForIO, and there's no guarantee that
-  // MessageLoopCurrent::Get() meets that criterion.
-  base::Thread::Options thread_options(base::MessageLoop::TYPE_IO, 0);
-  notifier_thread_->StartWithOptions(thread_options);
+      blocking_thread_runner_(
+          base::CreateSequencedTaskRunnerWithTraits({base::MayBlock()})),
+      blocking_thread_objects_(
+          new BlockingThreadObjects(ignored_interfaces),
+          // Ensure |blocking_thread_objects_| lives on
+          // |blocking_thread_runner_| to prevent races where
+          // NetworkChangeNotifierLinux outlives
+          // ScopedTaskEnvironment. https://crbug.com/938126
+          base::OnTaskRunnerDeleter(blocking_thread_runner_)) {
+  blocking_thread_runner_->PostTask(
+      FROM_HERE,
+      base::BindOnce(&NetworkChangeNotifierLinux::BlockingThreadObjects::Init,
+                     // The Unretained pointer is safe here because it's
+                     // posted before the deleter can post.
+                     base::Unretained(blocking_thread_objects_.get())));
 }
 
-NetworkChangeNotifierLinux::~NetworkChangeNotifierLinux() {
-  // Stopping from here allows us to sanity- check that the notifier
-  // thread shut down properly.
-  notifier_thread_->Stop();
-}
+NetworkChangeNotifierLinux::~NetworkChangeNotifierLinux() = default;
 
 // static
 NetworkChangeNotifier::NetworkChangeCalculatorParams
@@ -130,12 +125,12 @@ NetworkChangeNotifierLinux::NetworkChangeCalculatorParamsLinux() {
 
 NetworkChangeNotifier::ConnectionType
 NetworkChangeNotifierLinux::GetCurrentConnectionType() const {
-  return notifier_thread_->GetCurrentConnectionType();
+  return blocking_thread_objects_->GetCurrentConnectionType();
 }
 
 const internal::AddressTrackerLinux*
 NetworkChangeNotifierLinux::GetAddressTrackerInternal() const {
-  return notifier_thread_->address_tracker();
+  return blocking_thread_objects_->address_tracker();
 }
 
 }  // namespace net

net/base/network_change_notifier_linux.h

@@ -10,9 +10,15 @@
 
 #include "base/compiler_specific.h"
 #include "base/macros.h"
+#include "base/memory/scoped_refptr.h"
 #include "net/base/net_export.h"
 #include "net/base/network_change_notifier.h"
 
+namespace base {
+class SequencedTaskRunner;
+struct OnTaskRunnerDeleter;
+}  // namespace base
+
 namespace net {
 
 class NET_EXPORT_PRIVATE NetworkChangeNotifierLinux
@@ -28,7 +34,7 @@ class NET_EXPORT_PRIVATE NetworkChangeNotifierLinux
       const std::unordered_set<std::string>& ignored_interfaces);
 
  private:
-  class Thread;
+  class BlockingThreadObjects;
 
   ~NetworkChangeNotifierLinux() override;
   static NetworkChangeCalculatorParams NetworkChangeCalculatorParamsLinux();
@@ -39,11 +45,14 @@ class NET_EXPORT_PRIVATE NetworkChangeNotifierLinux
   const internal::AddressTrackerLinux* GetAddressTrackerInternal()
       const override;
 
-  // The thread used to listen for notifications.  This relays the notification
-  // to the registered observers without posting back to the thread the object
-  // was created on.
-  // Also used for DnsConfigService which requires TYPE_IO message loop.
-  std::unique_ptr<Thread> notifier_thread_;
+  // |blocking_thread_objects_| will live on this runner.
+  scoped_refptr<base::SequencedTaskRunner> blocking_thread_runner_;
+  // A collection of objects that must live on blocking sequences. These objects
+  // listen for notifications and relay the notifications to the registered
+  // observers without posting back to the thread the object was created on.
+  // Also used for DnsConfigService which also must live on blocking sequences.
+  std::unique_ptr<BlockingThreadObjects, base::OnTaskRunnerDeleter>
+      blocking_thread_objects_;
 
   DISALLOW_COPY_AND_ASSIGN(NetworkChangeNotifierLinux);
 };

net/dns/dns_config_service_posix.cc

@@ -18,7 +18,7 @@
 #include "base/metrics/histogram_macros.h"
 #include "base/single_thread_task_runner.h"
 #include "base/threading/scoped_blocking_call.h"
-#include "base/threading/thread_task_runner_handle.h"
+#include "base/threading/sequenced_task_runner_handle.h"
 #include "base/time/time.h"
 #include "build/build_config.h"
 #include "net/base/ip_address.h"
@@ -273,7 +273,7 @@ class DnsConfigServicePosix::Watcher {
   void OnConfigChanged(bool succeeded) {
     // Ignore transient flutter of resolv.conf by delaying the signal a bit.
     const base::TimeDelta kDelay = base::TimeDelta::FromMilliseconds(50);
-    base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
+    base::SequencedTaskRunnerHandle::Get()->PostDelayedTask(
         FROM_HERE,
         base::BindOnce(&Watcher::OnConfigChangedDelayed,
                        weak_factory_.GetWeakPtr(), succeeded),