Fix race in ~NetworkChangeNotifier overloads

Some ~NetworkChangeNotifier overloads post destruction to other
threads, which races ~NetworkChangeNotifier's clearing of
g_network_change_notifier.  Clear g_network_change_notifier at
the begining of ~NetworkChangeNotifier overloads to prevent this.

Bug: 965660
Change-Id: If6d792ea0f1badd7c816308646104e0f72ca5b4d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1625307Reviewed-by: Wez <wez@chromium.org>
Reviewed-by: Eric Orth <ericorth@chromium.org>
Commit-Queue: Paul Jensen <pauljensen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#662879}

net/android/network_change_notifier_android.cc

@@ -118,6 +118,7 @@ class NetworkChangeNotifierAndroid::BlockingThreadObjects {
 };
 
 NetworkChangeNotifierAndroid::~NetworkChangeNotifierAndroid() {
+  ClearGlobalPointer();
   delegate_->RemoveObserver(this);
 }
 

net/base/network_change_notifier.cc

@@ -114,7 +114,6 @@ class NetworkChangeNotifier::NetworkChangeCalculator
 
   ~NetworkChangeCalculator() override {
     DCHECK(thread_checker_.CalledOnValidThread());
-    DCHECK(g_network_change_notifier);
     RemoveConnectionTypeObserver(this);
     RemoveIPAddressObserver(this);
   }
@@ -174,10 +173,17 @@ class NetworkChangeNotifier::NetworkChangeCalculator
   DISALLOW_COPY_AND_ASSIGN(NetworkChangeCalculator);
 };
 
-NetworkChangeNotifier::~NetworkChangeNotifier() {
-  network_change_calculator_.reset();
+void NetworkChangeNotifier::ClearGlobalPointer() {
+  if (!cleared_global_pointer_) {
+    cleared_global_pointer_ = true;
     DCHECK_EQ(this, g_network_change_notifier);
     g_network_change_notifier = nullptr;
+  }
+}
+
+NetworkChangeNotifier::~NetworkChangeNotifier() {
+  network_change_calculator_.reset();
+  ClearGlobalPointer();
 }
 
 // static
@@ -483,92 +489,122 @@ NetworkChangeNotifier* NetworkChangeNotifier::CreateMock() {
   return new MockNetworkChangeNotifier();
 }
 
+NetworkChangeNotifier::IPAddressObserver::IPAddressObserver() = default;
+NetworkChangeNotifier::IPAddressObserver::~IPAddressObserver() = default;
+
+NetworkChangeNotifier::ConnectionTypeObserver::ConnectionTypeObserver() =
+    default;
+NetworkChangeNotifier::ConnectionTypeObserver::~ConnectionTypeObserver() =
+    default;
+
+NetworkChangeNotifier::DNSObserver::DNSObserver() = default;
+NetworkChangeNotifier::DNSObserver::~DNSObserver() = default;
+
+NetworkChangeNotifier::NetworkChangeObserver::NetworkChangeObserver() = default;
+NetworkChangeNotifier::NetworkChangeObserver::~NetworkChangeObserver() =
+    default;
+
+NetworkChangeNotifier::MaxBandwidthObserver::MaxBandwidthObserver() = default;
+NetworkChangeNotifier::MaxBandwidthObserver::~MaxBandwidthObserver() = default;
+
+NetworkChangeNotifier::NetworkObserver::NetworkObserver() = default;
+NetworkChangeNotifier::NetworkObserver::~NetworkObserver() = default;
+
 void NetworkChangeNotifier::AddIPAddressObserver(IPAddressObserver* observer) {
-  if (g_network_change_notifier)
-    g_network_change_notifier->ip_address_observer_list_->AddObserver(observer);
+  if (g_network_change_notifier) {
+    observer->observer_list_ =
+        g_network_change_notifier->ip_address_observer_list_;
+    observer->observer_list_->AddObserver(observer);
+  }
 }
 
 void NetworkChangeNotifier::AddConnectionTypeObserver(
     ConnectionTypeObserver* observer) {
   if (g_network_change_notifier) {
-    g_network_change_notifier->connection_type_observer_list_->AddObserver(
-        observer);
+    observer->observer_list_ =
+        g_network_change_notifier->connection_type_observer_list_;
+    observer->observer_list_->AddObserver(observer);
   }
 }
 
 void NetworkChangeNotifier::AddDNSObserver(DNSObserver* observer) {
   if (g_network_change_notifier) {
-    g_network_change_notifier->resolver_state_observer_list_->AddObserver(
-        observer);
+    observer->observer_list_ =
+        g_network_change_notifier->resolver_state_observer_list_;
+    observer->observer_list_->AddObserver(observer);
   }
 }
 
 void NetworkChangeNotifier::AddNetworkChangeObserver(
     NetworkChangeObserver* observer) {
   if (g_network_change_notifier) {
-    g_network_change_notifier->network_change_observer_list_->AddObserver(
-        observer);
+    observer->observer_list_ =
+        g_network_change_notifier->network_change_observer_list_;
+    observer->observer_list_->AddObserver(observer);
   }
 }
 
 void NetworkChangeNotifier::AddMaxBandwidthObserver(
     MaxBandwidthObserver* observer) {
   if (g_network_change_notifier) {
-    g_network_change_notifier->max_bandwidth_observer_list_->AddObserver(
-        observer);
+    observer->observer_list_ =
+        g_network_change_notifier->max_bandwidth_observer_list_;
+    observer->observer_list_->AddObserver(observer);
   }
 }
 
 void NetworkChangeNotifier::AddNetworkObserver(NetworkObserver* observer) {
   DCHECK(AreNetworkHandlesSupported());
   if (g_network_change_notifier) {
-    g_network_change_notifier->network_observer_list_->AddObserver(observer);
+    observer->observer_list_ =
+        g_network_change_notifier->network_observer_list_;
+    observer->observer_list_->AddObserver(observer);
   }
 }
 
 void NetworkChangeNotifier::RemoveIPAddressObserver(
     IPAddressObserver* observer) {
-  if (g_network_change_notifier) {
-    g_network_change_notifier->ip_address_observer_list_->RemoveObserver(
-        observer);
+  if (observer->observer_list_) {
+    observer->observer_list_->RemoveObserver(observer);
+    observer->observer_list_.reset();
   }
 }
 
 void NetworkChangeNotifier::RemoveConnectionTypeObserver(
     ConnectionTypeObserver* observer) {
-  if (g_network_change_notifier) {
-    g_network_change_notifier->connection_type_observer_list_->RemoveObserver(
-        observer);
+  if (observer->observer_list_) {
+    observer->observer_list_->RemoveObserver(observer);
+    observer->observer_list_.reset();
   }
 }
 
 void NetworkChangeNotifier::RemoveDNSObserver(DNSObserver* observer) {
-  if (g_network_change_notifier) {
-    g_network_change_notifier->resolver_state_observer_list_->RemoveObserver(
-        observer);
+  if (observer->observer_list_) {
+    observer->observer_list_->RemoveObserver(observer);
+    observer->observer_list_.reset();
   }
 }
 
 void NetworkChangeNotifier::RemoveNetworkChangeObserver(
     NetworkChangeObserver* observer) {
-  if (g_network_change_notifier) {
-    g_network_change_notifier->network_change_observer_list_->RemoveObserver(
-        observer);
+  if (observer->observer_list_) {
+    observer->observer_list_->RemoveObserver(observer);
+    observer->observer_list_.reset();
   }
 }
 
 void NetworkChangeNotifier::RemoveMaxBandwidthObserver(
     MaxBandwidthObserver* observer) {
-  if (g_network_change_notifier) {
-    g_network_change_notifier->max_bandwidth_observer_list_->RemoveObserver(
-        observer);
+  if (observer->observer_list_) {
+    observer->observer_list_->RemoveObserver(observer);
+    observer->observer_list_.reset();
   }
 }
 
 void NetworkChangeNotifier::RemoveNetworkObserver(NetworkObserver* observer) {
-  DCHECK(AreNetworkHandlesSupported());
-  if (g_network_change_notifier) {
-    g_network_change_notifier->network_observer_list_->RemoveObserver(observer);
+  if (observer->observer_list_) {
+    observer->observer_list_->RemoveObserver(observer);
+    observer->observer_list_.reset();
   }
 }
 

net/base/network_change_notifier.h

@@ -109,10 +109,14 @@ class NET_EXPORT NetworkChangeNotifier {
     virtual void OnIPAddressChanged() = 0;
 
    protected:
-    IPAddressObserver() {}
-    virtual ~IPAddressObserver() {}
+    IPAddressObserver();
+    virtual ~IPAddressObserver();
 
    private:
+    friend NetworkChangeNotifier;
+    scoped_refptr<base::ObserverListThreadSafe<IPAddressObserver>>
+        observer_list_;
+
     DISALLOW_COPY_AND_ASSIGN(IPAddressObserver);
   };
 
@@ -126,10 +130,14 @@ class NET_EXPORT NetworkChangeNotifier {
     virtual void OnConnectionTypeChanged(ConnectionType type) = 0;
 
    protected:
-    ConnectionTypeObserver() {}
-    virtual ~ConnectionTypeObserver() {}
+    ConnectionTypeObserver();
+    virtual ~ConnectionTypeObserver();
 
    private:
+    friend NetworkChangeNotifier;
+    scoped_refptr<base::ObserverListThreadSafe<ConnectionTypeObserver>>
+        observer_list_;
+
     DISALLOW_COPY_AND_ASSIGN(ConnectionTypeObserver);
   };
 
@@ -148,10 +156,13 @@ class NET_EXPORT NetworkChangeNotifier {
     virtual void OnInitialDNSConfigRead();
 
    protected:
-    DNSObserver() {}
-    virtual ~DNSObserver() {}
+    DNSObserver();
+    virtual ~DNSObserver();
 
    private:
+    friend NetworkChangeNotifier;
+    scoped_refptr<base::ObserverListThreadSafe<DNSObserver>> observer_list_;
+
     DISALLOW_COPY_AND_ASSIGN(DNSObserver);
   };
 
@@ -185,10 +196,14 @@ class NET_EXPORT NetworkChangeNotifier {
     virtual void OnNetworkChanged(ConnectionType type) = 0;
 
    protected:
-    NetworkChangeObserver() {}
-    virtual ~NetworkChangeObserver() {}
+    NetworkChangeObserver();
+    virtual ~NetworkChangeObserver();
 
    private:
+    friend NetworkChangeNotifier;
+    scoped_refptr<base::ObserverListThreadSafe<NetworkChangeObserver>>
+        observer_list_;
+
     DISALLOW_COPY_AND_ASSIGN(NetworkChangeObserver);
   };
 
@@ -203,10 +218,14 @@ class NET_EXPORT NetworkChangeNotifier {
                                        ConnectionType type) = 0;
 
    protected:
-    MaxBandwidthObserver() {}
-    virtual ~MaxBandwidthObserver() {}
+    MaxBandwidthObserver();
+    virtual ~MaxBandwidthObserver();
 
    private:
+    friend NetworkChangeNotifier;
+    scoped_refptr<base::ObserverListThreadSafe<MaxBandwidthObserver>>
+        observer_list_;
+
     DISALLOW_COPY_AND_ASSIGN(MaxBandwidthObserver);
   };
 
@@ -245,10 +264,13 @@ class NET_EXPORT NetworkChangeNotifier {
     virtual void OnNetworkMadeDefault(NetworkHandle network) = 0;
 
    protected:
-    NetworkObserver() {}
-    virtual ~NetworkObserver() {}
+    NetworkObserver();
+    virtual ~NetworkObserver();
 
    private:
+    friend NetworkChangeNotifier;
+    scoped_refptr<base::ObserverListThreadSafe<NetworkObserver>> observer_list_;
+
     DISALLOW_COPY_AND_ASSIGN(NetworkObserver);
   };
 
@@ -536,6 +558,10 @@ class NET_EXPORT NetworkChangeNotifier {
   // have the same type, return it, otherwise return CONNECTION_UNKNOWN.
   static ConnectionType ConnectionTypeFromInterfaces();
 
+  // Clears the global NetworkChangeNotifier pointer.  This should be called
+  // as early as possible in the destructor to prevent races.
+  void ClearGlobalPointer();
+
  private:
   friend class HostResolverManagerDnsTest;
   friend class NetworkChangeNotifierAndroidTest;
@@ -577,6 +603,9 @@ class NET_EXPORT NetworkChangeNotifier {
   // Set true to disable non-test notifications (to prevent flakes in tests).
   static bool test_notifications_only_;
 
+  // Indicates if this instance cleared g_network_change_notifier_ yet.
+  bool cleared_global_pointer_ = false;
+
   DISALLOW_COPY_AND_ASSIGN(NetworkChangeNotifier);
 };
 

net/base/network_change_notifier_fuchsia.cc

@@ -61,6 +61,7 @@ NetworkChangeNotifierFuchsia::NetworkChangeNotifierFuchsia(
 
 NetworkChangeNotifierFuchsia::~NetworkChangeNotifierFuchsia() {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+  ClearGlobalPointer();
 }
 
 NetworkChangeNotifier::ConnectionType

net/base/network_change_notifier_linux.cc

@@ -107,7 +107,9 @@ NetworkChangeNotifierLinux::NetworkChangeNotifierLinux(
                      base::Unretained(blocking_thread_objects_.get())));
 }
 
-NetworkChangeNotifierLinux::~NetworkChangeNotifierLinux() = default;
+NetworkChangeNotifierLinux::~NetworkChangeNotifierLinux() {
+  ClearGlobalPointer();
+}
 
 // static
 NetworkChangeNotifier::NetworkChangeCalculatorParams

net/base/network_change_notifier_mac.cc

@@ -56,6 +56,7 @@ NetworkChangeNotifierMac::NetworkChangeNotifierMac()
 }
 
 NetworkChangeNotifierMac::~NetworkChangeNotifierMac() {
+  ClearGlobalPointer();
   // Delete the ConfigWatcher to join the notifier thread, ensuring that
   // StartReachabilityNotifications() has an opportunity to run to completion.
   config_watcher_.reset();

net/base/network_change_notifier_posix.cc

@@ -66,7 +66,9 @@ NetworkChangeNotifierPosix::NetworkChangeNotifierPosix(
   OnDNSChanged();
 }
 
-NetworkChangeNotifierPosix::~NetworkChangeNotifierPosix() = default;
+NetworkChangeNotifierPosix::~NetworkChangeNotifierPosix() {
+  ClearGlobalPointer();
+}
 
 void NetworkChangeNotifierPosix::OnDNSChanged() {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);

net/base/network_change_notifier_win.cc

@@ -55,6 +55,7 @@ NetworkChangeNotifierWin::NetworkChangeNotifierWin()
 
 NetworkChangeNotifierWin::~NetworkChangeNotifierWin() {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  ClearGlobalPointer();
   if (is_watching_) {
     CancelIPChangeNotify(&addr_overlapped_);
     addr_watcher_.StopWatching();