[NetworkChangeNotifier] Run Windows connection type computation on other thread

net/base/network_change_notifier_win.cc

Index: net/base/network_change_notifier_win.cc
diff --git a/net/base/network_change_notifier_win.cc b/net/base/network_change_notifier_win.cc
index 9b2b35ec76bfbc08a1ac6008dbf9f85b80a2618b..26d4e2a764dc8f48878f8d48afc215fc264e2976 100644
--- a/net/base/network_change_notifier_win.cc
+++ b/net/base/network_change_notifier_win.cc
@@ -14,6 +14,7 @@
 #include "base/message_loop/message_loop.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/single_thread_task_runner.h"
+#include "base/task_runner_util.h"
 #include "base/threading/thread.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/time/time.h"
@@ -28,64 +29,6 @@ namespace {
 // Time between NotifyAddrChange retries, on failure.
 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()),
-      last_computed_connection_type_(RecomputeCurrentConnectionType()),
-      last_announced_offline_(last_computed_connection_type_ ==
-                              CONNECTION_NONE),
-      weak_factory_(this) {
-  memset(&addr_overlapped_, 0, sizeof addr_overlapped_);
-  addr_overlapped_.hEvent = WSACreateEvent();
-}
-
-NetworkChangeNotifierWin::~NetworkChangeNotifierWin() {
-  if (is_watching_) {
-    CancelIPChangeNotify(&addr_overlapped_);
-    addr_watcher_.StopWatching();
-  }
-  WSACloseEvent(addr_overlapped_.hEvent);
-}
-
-// static
-NetworkChangeNotifier::NetworkChangeCalculatorParams
-NetworkChangeNotifierWin::NetworkChangeCalculatorParamsWin() {
-  NetworkChangeCalculatorParams params;
-  // Delay values arrived at by simple experimentation and adjusted so as to
-  // produce a single signal when switching between network connections.
-  params.ip_address_offline_delay_ = base::TimeDelta::FromMilliseconds(1500);
-  params.ip_address_online_delay_ = base::TimeDelta::FromMilliseconds(1500);
-  params.connection_type_offline_delay_ =
-      base::TimeDelta::FromMilliseconds(1500);
-  params.connection_type_online_delay_ = base::TimeDelta::FromMilliseconds(500);
-  return params;
-}
-
 // This implementation does not return the actual connection type but merely
 // determines if the user is "online" (in which case it returns
 // CONNECTION_UNKNOWN) or "offline" (and then it returns CONNECTION_NONE).
@@ -134,10 +77,8 @@ NetworkChangeNotifierWin::NetworkChangeCalculatorParamsWin() {
 // experiments I ran... However none of them correctly returned "offline" when
 // executing 'ipconfig /release'.
 //
-NetworkChangeNotifier::ConnectionType
-NetworkChangeNotifierWin::RecomputeCurrentConnectionType() const {
-  DCHECK(CalledOnValidThread());
-
+// It is not thread safe, see crbug.com/324913.
+NetworkChangeNotifier::ConnectionType RecomputeCurrentConnectionType() {
   EnsureWinsockInit();
 
   // The following code was adapted from:
@@ -201,8 +142,69 @@ NetworkChangeNotifierWin::RecomputeCurrentConnectionType() const {
       << "WSALookupServiceEnd() failed with: " << result;
 
   // TODO(droger): Return something more detailed than CONNECTION_UNKNOWN.
-  return found_connection ? ConnectionTypeFromInterfaces()
-                          : NetworkChangeNotifier::CONNECTION_NONE;
+  return found_connection
+             ? NetworkChangeNotifier::ConnectionTypeFromInterfaces()
+             : NetworkChangeNotifier::CONNECTION_NONE;
+}
+
+}  // 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()),
+      last_computed_connection_type_(RecomputeCurrentConnectionType()),
+      last_announced_offline_(last_computed_connection_type_ ==
+                              CONNECTION_NONE),
+      calculate_connection_type_(
+          base::BindRepeating(&RecomputeCurrentConnectionType)),
+      weak_factory_(this) {
+  memset(&addr_overlapped_, 0, sizeof addr_overlapped_);
+  addr_overlapped_.hEvent = WSACreateEvent();
+}
+
+NetworkChangeNotifierWin::~NetworkChangeNotifierWin() {
+  if (is_watching_) {
+    CancelIPChangeNotify(&addr_overlapped_);
+    addr_watcher_.StopWatching();
+  }
+  WSACloseEvent(addr_overlapped_.hEvent);
+}
+
+// static
+NetworkChangeNotifier::NetworkChangeCalculatorParams
+NetworkChangeNotifierWin::NetworkChangeCalculatorParamsWin() {
+  NetworkChangeCalculatorParams params;
+  // Delay values arrived at by simple experimentation and adjusted so as to
+  // produce a single signal when switching between network connections.
+  params.ip_address_offline_delay_ = base::TimeDelta::FromMilliseconds(1500);
+  params.ip_address_online_delay_ = base::TimeDelta::FromMilliseconds(1500);
+  params.connection_type_offline_delay_ =
+      base::TimeDelta::FromMilliseconds(1500);
+  params.connection_type_online_delay_ = base::TimeDelta::FromMilliseconds(500);
+  return params;
 }
 
 NetworkChangeNotifier::ConnectionType
@@ -224,13 +226,17 @@ void NetworkChangeNotifierWin::OnObjectSignaled(HANDLE object) {
 
   // Start watching for the next address change.
   WatchForAddressChange();
+  DCHECK(dns_task_runner_);
 
-  NotifyObservers();
+  base::PostTaskAndReplyWithResult(
+      dns_task_runner_.get(), FROM_HERE, calculate_connection_type_,
+      base::Bind(&NetworkChangeNotifierWin::NotifyObservers,
+                 weak_factory_.GetWeakPtr()));
 }
 
-void NetworkChangeNotifierWin::NotifyObservers() {
+void NetworkChangeNotifierWin::NotifyObservers(ConnectionType connection_type) {
   DCHECK(CalledOnValidThread());
-  SetCurrentConnectionType(RecomputeCurrentConnectionType());
+  SetCurrentConnectionType(connection_type);
   NotifyObserversOfIPAddressChange();
 
   // Calling GetConnectionType() at this very moment is likely to give
@@ -241,6 +247,8 @@ void NetworkChangeNotifierWin::NotifyObservers() {
   // If after one second we determine we are still offline, we will
   // delay again.
   offline_polls_ = 0;
+  // TODO(jkarlin): I'm not sure that calling with 'this' is safe. Can it be
+  // deleted?
   timer_.Start(FROM_HERE, base::TimeDelta::FromSeconds(1), this,
                &NetworkChangeNotifierWin::NotifyParentOfConnectionTypeChange);
 }
@@ -274,8 +282,12 @@ void NetworkChangeNotifierWin::WatchForAddressChange() {
   // Treat the transition from NotifyAddrChange failing to succeeding as a
   // network change event, since network changes were not being observed in
   // that interval.
-  if (sequential_failures_ > 0)
-    NotifyObservers();
+  if (sequential_failures_ > 0) {
+    base::PostTaskAndReplyWithResult(
+        dns_task_runner_.get(), FROM_HERE, calculate_connection_type_,
+        base::Bind(&NetworkChangeNotifierWin::NotifyObservers,
+                   weak_factory_.GetWeakPtr()));
+  }
 
   if (sequential_failures_ < 2000) {
     UMA_HISTOGRAM_COUNTS_10000("Net.NotifyAddrChangeFailures",
@@ -292,6 +304,9 @@ bool NetworkChangeNotifierWin::WatchForAddressChangeInternal() {
   if (!dns_config_service_thread_->IsRunning()) {
     dns_config_service_thread_->StartWithOptions(
       base::Thread::Options(base::MessageLoop::TYPE_IO, 0));
+    if (!dns_task_runner_)
+      dns_task_runner_ =
+          dns_config_service_thread_->message_loop()->task_runner();
   }
 
   ResetEventIfSignaled(addr_overlapped_.hEvent);
@@ -305,7 +320,16 @@ bool NetworkChangeNotifierWin::WatchForAddressChangeInternal() {
 }
 
 void NetworkChangeNotifierWin::NotifyParentOfConnectionTypeChange() {
-  SetCurrentConnectionType(RecomputeCurrentConnectionType());
+  base::PostTaskAndReplyWithResult(
+      dns_task_runner_.get(), FROM_HERE, calculate_connection_type_,
+      base::Bind(
+          &NetworkChangeNotifierWin::NotifyParentOfConnectionTypeChangeImpl,
+          weak_factory_.GetWeakPtr()));
+}
+
+void NetworkChangeNotifierWin::NotifyParentOfConnectionTypeChangeImpl(
+    ConnectionType connection_type) {
+  SetCurrentConnectionType(connection_type);
   bool current_offline = IsOffline();
   offline_polls_++;
   // If we continue to appear offline, delay sending out the notification in
@@ -323,10 +347,10 @@ void NetworkChangeNotifierWin::NotifyParentOfConnectionTypeChange() {
 
   NotifyObserversOfConnectionTypeChange();
   double max_bandwidth_mbps = 0.0;
-  ConnectionType connection_type = CONNECTION_NONE;
+  ConnectionType connection_type2 = CONNECTION_NONE;

[pauljensen, 2017/05/22 18:01:51]

can we give this variable a more descriptive name?

[jkarlin, 2017/05/23 11:44:38]

Done.

   GetCurrentMaxBandwidthAndConnectionType(&max_bandwidth_mbps,
-                                          &connection_type);
-  NotifyObserversOfMaxBandwidthChange(max_bandwidth_mbps, connection_type);
+                                          &connection_type2);
+  NotifyObserversOfMaxBandwidthChange(max_bandwidth_mbps, connection_type2);
 }
 
 }  // namespace net