Fix race-conditions during NetworkChangeNotifier shutdown by

properly shutting down HistogramWatcher and NetworkChangeCalculator. Observers must be unregistered to avoid use-after-free's when an event is posted to a deleted object. Unregistering observers must take place on the same thread the observer was registered on to have any effect. NetworkChangeNotifier observers must be unregistered prior to calling ~NetworkChangeNotifier() so g_network_change_notifier is non-NULL. BUG=357320 Review URL: https://codereview.chromium.org/213053006 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@260737 0039d316-1c4b-4281-b951-d872f2087c98
hylschwy · Apr 1, 2014 · 7592b41 · 7592b41
1 parent 07e705f
commit 7592b41
Show file tree

Hide file tree

Showing 3 changed files with 50 additions and 3 deletions.
diff --git a/chrome/browser/io_thread.cc b/chrome/browser/io_thread.cc
@@ -744,6 +744,9 @@ void IOThread::CleanUp() {
   // Release objects that the net::URLRequestContext could have been pointing
   // to.
 
+  // Shutdown the HistogramWatcher on the IO thread.
+  net::NetworkChangeNotifier::ShutdownHistogramWatcher();
+
   // This must be reset before the ChromeNetLog is destroyed.
   network_change_observer_.reset();
 

diff --git a/net/base/network_change_notifier.cc b/net/base/network_change_notifier.cc
@@ -6,6 +6,7 @@
 
 #include "base/metrics/histogram.h"
 #include "base/synchronization/lock.h"
+#include "base/threading/thread_checker.h"
 #include "build/build_config.h"
 #include "net/base/net_util.h"
 #include "net/base/network_change_notifier_factory.h"
@@ -66,16 +67,26 @@ class HistogramWatcher
   // because the only other interface, |NotifyDataReceived| is also
   // only called from the network thread.
   void Init() {
+    DCHECK(thread_checker_.CalledOnValidThread());
+    DCHECK(g_network_change_notifier);
     NetworkChangeNotifier::AddConnectionTypeObserver(this);
     NetworkChangeNotifier::AddIPAddressObserver(this);
     NetworkChangeNotifier::AddDNSObserver(this);
     NetworkChangeNotifier::AddNetworkChangeObserver(this);
   }
 
-  virtual ~HistogramWatcher() {}
+  virtual ~HistogramWatcher() {
+    DCHECK(thread_checker_.CalledOnValidThread());
+    DCHECK(g_network_change_notifier);
+    NetworkChangeNotifier::RemoveConnectionTypeObserver(this);
+    NetworkChangeNotifier::RemoveIPAddressObserver(this);
+    NetworkChangeNotifier::RemoveDNSObserver(this);
+    NetworkChangeNotifier::RemoveNetworkChangeObserver(this);
+  }
 
   // NetworkChangeNotifier::IPAddressObserver implementation.
   virtual void OnIPAddressChanged() OVERRIDE {
+    DCHECK(thread_checker_.CalledOnValidThread());
     UMA_HISTOGRAM_MEDIUM_TIMES("NCN.IPAddressChange",
                                SinceLast(&last_ip_address_change_));
     UMA_HISTOGRAM_MEDIUM_TIMES(
@@ -86,6 +97,7 @@ class HistogramWatcher
   // NetworkChangeNotifier::ConnectionTypeObserver implementation.
   virtual void OnConnectionTypeChanged(
       NetworkChangeNotifier::ConnectionType type) OVERRIDE {
+    DCHECK(thread_checker_.CalledOnValidThread());
     base::TimeTicks now = base::TimeTicks::Now();
     int32 kilobytes_read = bytes_read_since_last_connection_change_ / 1000;
     base::TimeDelta state_duration = SinceLast(&last_connection_change_);
@@ -229,13 +241,15 @@ class HistogramWatcher
 
   // NetworkChangeNotifier::DNSObserver implementation.
   virtual void OnDNSChanged() OVERRIDE {
+    DCHECK(thread_checker_.CalledOnValidThread());
     UMA_HISTOGRAM_MEDIUM_TIMES("NCN.DNSConfigChange",
                                SinceLast(&last_dns_change_));
   }
 
   // NetworkChangeNotifier::NetworkChangeObserver implementation.
   virtual void OnNetworkChanged(
       NetworkChangeNotifier::ConnectionType type) OVERRIDE {
+    DCHECK(thread_checker_.CalledOnValidThread());
     if (type != NetworkChangeNotifier::CONNECTION_NONE) {
       UMA_HISTOGRAM_MEDIUM_TIMES("NCN.NetworkOnlineChange",
                                  SinceLast(&last_network_change_));
@@ -248,6 +262,7 @@ class HistogramWatcher
   // Record histogram data whenever we receive a packet. Should only be called
   // from the network thread.
   void NotifyDataReceived(const URLRequest& request, int bytes_read) {
+    DCHECK(thread_checker_.CalledOnValidThread());
     if (IsLocalhost(request.url().host()) ||
         !request.url().SchemeIsHTTPOrHTTPS()) {
       return;
@@ -337,6 +352,8 @@ class HistogramWatcher
   // Erring on the conservative side is hopefully offset by taking the maximum.
   int32 peak_kbps_since_last_connection_change_;
 
+  base::ThreadChecker thread_checker_;
+
   DISALLOW_COPY_AND_ASSIGN(HistogramWatcher);
 };
 
@@ -377,17 +394,22 @@ class NetworkChangeNotifier::NetworkChangeCalculator
         pending_connection_type_(CONNECTION_NONE) {}
 
   void Init() {
+    DCHECK(thread_checker_.CalledOnValidThread());
+    DCHECK(g_network_change_notifier);
     AddConnectionTypeObserver(this);
     AddIPAddressObserver(this);
   }
 
   virtual ~NetworkChangeCalculator() {
+    DCHECK(thread_checker_.CalledOnValidThread());
+    DCHECK(g_network_change_notifier);
     RemoveConnectionTypeObserver(this);
     RemoveIPAddressObserver(this);
   }
 
   // NetworkChangeNotifier::IPAddressObserver implementation.
   virtual void OnIPAddressChanged() OVERRIDE {
+    DCHECK(thread_checker_.CalledOnValidThread());
     base::TimeDelta delay = last_announced_connection_type_ == CONNECTION_NONE
         ? params_.ip_address_offline_delay_ : params_.ip_address_online_delay_;
     // Cancels any previous timer.
@@ -396,6 +418,7 @@ class NetworkChangeNotifier::NetworkChangeCalculator
 
   // NetworkChangeNotifier::ConnectionTypeObserver implementation.
   virtual void OnConnectionTypeChanged(ConnectionType type) OVERRIDE {
+    DCHECK(thread_checker_.CalledOnValidThread());
     pending_connection_type_ = type;
     base::TimeDelta delay = last_announced_connection_type_ == CONNECTION_NONE
         ? params_.connection_type_offline_delay_
@@ -406,6 +429,7 @@ class NetworkChangeNotifier::NetworkChangeCalculator
 
  private:
   void Notify() {
+    DCHECK(thread_checker_.CalledOnValidThread());
     // Don't bother signaling about dead connections.
     if (have_announced_ &&
         (last_announced_connection_type_ == CONNECTION_NONE) &&
@@ -432,9 +456,14 @@ class NetworkChangeNotifier::NetworkChangeCalculator
   ConnectionType pending_connection_type_;
   // Used to delay notifications so duplicates can be combined.
   base::OneShotTimer<NetworkChangeCalculator> timer_;
+
+  base::ThreadChecker thread_checker_;
+
+  DISALLOW_COPY_AND_ASSIGN(NetworkChangeCalculator);
 };
 
 NetworkChangeNotifier::~NetworkChangeNotifier() {
+  network_change_calculator_.reset();
   DCHECK_EQ(this, g_network_change_notifier);
   g_network_change_notifier = NULL;
 }
@@ -518,8 +547,10 @@ const char* NetworkChangeNotifier::ConnectionTypeToString(
 // static
 void NetworkChangeNotifier::NotifyDataReceived(const URLRequest& request,
                                                int bytes_read) {
-  if (!g_network_change_notifier)
+  if (!g_network_change_notifier ||
+      !g_network_change_notifier->histogram_watcher_) {
     return;
+  }
   g_network_change_notifier->histogram_watcher_->NotifyDataReceived(request,
                                                                     bytes_read);
 }
@@ -528,9 +559,17 @@ void NetworkChangeNotifier::NotifyDataReceived(const URLRequest& request,
 void NetworkChangeNotifier::InitHistogramWatcher() {
   if (!g_network_change_notifier)
     return;
+  g_network_change_notifier->histogram_watcher_.reset(new HistogramWatcher());
   g_network_change_notifier->histogram_watcher_->Init();
 }
 
+// static
+void NetworkChangeNotifier::ShutdownHistogramWatcher() {
+  if (!g_network_change_notifier)
+    return;
+  g_network_change_notifier->histogram_watcher_.reset();
+}
+
 #if defined(OS_LINUX)
 // static
 const internal::AddressTrackerLinux*
@@ -644,7 +683,6 @@ NetworkChangeNotifier::NetworkChangeNotifier(
         new ObserverListThreadSafe<NetworkChangeObserver>(
             ObserverListBase<NetworkChangeObserver>::NOTIFY_EXISTING_ONLY)),
       network_state_(new NetworkState()),
-      histogram_watcher_(new HistogramWatcher()),
       network_change_calculator_(new NetworkChangeCalculator(params)) {
   DCHECK(!g_network_change_notifier);
   g_network_change_notifier = this;

diff --git a/net/base/network_change_notifier.h b/net/base/network_change_notifier.h
@@ -224,8 +224,14 @@ class NET_EXPORT NetworkChangeNotifier {
 
   // Register the Observer callbacks for producing histogram data.  This
   // should be called from the network thread to avoid race conditions.
+  // ShutdownHistogramWatcher() must be called prior to NetworkChangeNotifier
+  // destruction.
   static void InitHistogramWatcher();
 
+  // Unregister the Observer callbacks for producing histogram data.  This
+  // should be called from the network thread to avoid race conditions.
+  static void ShutdownHistogramWatcher();
+
   // Allows a second NetworkChangeNotifier to be created for unit testing, so
   // the test suite can create a MockNetworkChangeNotifier, but platform
   // specific NetworkChangeNotifiers can also be created for testing.  To use,