[NetworkChangeNotifier] Return more specific network connection types (ethernet/wifi) on OSX

net/base/network_change_notifier_mac.cc

Index: net/base/network_change_notifier_mac.cc
diff --git a/net/base/network_change_notifier_mac.cc b/net/base/network_change_notifier_mac.cc
index 0082153613a1ef79c2c85c9c8081759fb20336d1..8037146a6beabd2777407045e4d29ddfbc48d88e 100644
--- a/net/base/network_change_notifier_mac.cc
+++ b/net/base/network_change_notifier_mac.cc
@@ -9,6 +9,7 @@
 
 #include "base/macros.h"
 #include "base/message_loop/message_loop.h"
+#include "base/task_runner_util.h"
 #include "base/threading/thread.h"
 #include "base/threading/thread_restrictions.h"
 #include "net/dns/dns_config_service.h"
@@ -21,24 +22,6 @@ static bool CalculateReachability(SCNetworkConnectionFlags flags) {
   return reachable && !connection_required;
 }
 
-NetworkChangeNotifier::ConnectionType CalculateConnectionType(
-    SCNetworkConnectionFlags flags) {
-  bool reachable = CalculateReachability(flags);
-  if (reachable) {
-#if defined(OS_IOS)
-    return (flags & kSCNetworkReachabilityFlagsIsWWAN) ?
-        NetworkChangeNotifier::CONNECTION_3G :
-        NetworkChangeNotifier::CONNECTION_WIFI;
-#else
-    // TODO(droger): Get something more detailed than CONNECTION_UNKNOWN.
-    // http://crbug.com/112937
-    return NetworkChangeNotifier::CONNECTION_UNKNOWN;
-#endif  // defined(OS_IOS)
-  } else {
-    return NetworkChangeNotifier::CONNECTION_NONE;
-  }
-}
-
 // Thread on which we can run DnsConfigService, which requires a TYPE_IO
 // message loop.
 class NetworkChangeNotifierMac::DnsConfigServiceThread : public base::Thread {
@@ -66,7 +49,8 @@ NetworkChangeNotifierMac::NetworkChangeNotifierMac()
       connection_type_initialized_(false),
       initial_connection_type_cv_(&connection_type_lock_),
       forwarder_(this),
-      dns_config_service_thread_(new DnsConfigServiceThread()) {
+      dns_config_service_thread_(new DnsConfigServiceThread()),
+      weak_ptr_factory_(this) {
   // Must be initialized after the rest of this object, as it may call back into
   // SetInitialConnectionType().
   config_watcher_.reset(new NetworkConfigWatcherMac(&forwarder_));
@@ -102,6 +86,28 @@ NetworkChangeNotifierMac::NetworkChangeCalculatorParamsMac() {
   return params;
 }
 
+// static
+NetworkChangeNotifier::ConnectionType
+NetworkChangeNotifierMac::CalculateConnectionType(
+    SCNetworkConnectionFlags flags) {
+  // Called on notifier thread during forwarder initialization, otherwise on
+  // the dns thread.
+  bool reachable = CalculateReachability(flags);
+  if (reachable) {
+#if defined(OS_IOS)
+    return (flags & kSCNetworkReachabilityFlagsIsWWAN)
+               ? NetworkChangeNotifier::CONNECTION_3G
+               : NetworkChangeNotifier::CONNECTION_WIFI;
+#else
+    // TODO(droger): Get something more detailed than CONNECTION_UNKNOWN.
+    // http://crbug.com/112937

[pauljensen, 2017/05/23 15:10:30]

I think we can remove this comment now.  Can you also add 112937 to the "BUG="
line in the commit description.

[jkarlin, 2017/05/27 01:06:19]

Done.

+    return ConnectionTypeFromInterfaces();
+#endif  // defined(OS_IOS)
+  } else {
+    return CONNECTION_NONE;
+  }
+}
+
 NetworkChangeNotifier::ConnectionType
 NetworkChangeNotifierMac::GetCurrentConnectionType() const {
   base::ThreadRestrictions::ScopedAllowWait allow_wait;
@@ -248,21 +254,30 @@ void NetworkChangeNotifierMac::ReachabilityCallback(
     void* notifier) {
   NetworkChangeNotifierMac* notifier_mac =
       static_cast<NetworkChangeNotifierMac*>(notifier);
-

[pauljensen, 2017/05/23 15:10:30]

whitespace change?

[jkarlin, 2017/05/27 01:06:19]

Done.

   DCHECK_EQ(notifier_mac->run_loop_.get(), CFRunLoopGetCurrent());
 
-  ConnectionType new_type = CalculateConnectionType(flags);
+  base::PostTaskAndReplyWithResult(
+      notifier_mac->dns_config_service_thread_->message_loop()
+          ->task_runner()

[pauljensen, 2017/05/23 15:18:15]

hmm so I think this may not be the thread we want to use.  NCNMac connection
types are calculated on the NetworkConfigWatcherMac::notifier_thread_ thread,
which I think might race the task posted here which could be bad.

[jkarlin, 2017/05/27 01:06:19]

After F2F, moved to notifier thread and enabled blocking operations on the
notifier thread. 

In terms of concerns of this patch on startup cost, I ran it on my 3 year old
macbook and it ran ConnectionTypeFromInterfaces() on startup in < 1ms in each of
three runs.

run 1: 980 microseconds
run 2: 446 microseconds
run 3: 497 microseconds

+          .get(),
+      FROM_HERE,
+      base::Bind(&NetworkChangeNotifierMac::CalculateConnectionType, flags),
+      base::Bind(&NetworkChangeNotifierMac::ReachabilityCallbackImpl,
+                 notifier_mac->weak_ptr_factory_.GetWeakPtr()));
+}
+
+void NetworkChangeNotifierMac::ReachabilityCallbackImpl(
+    ConnectionType new_type) {
   ConnectionType old_type;
   {
-    base::AutoLock lock(notifier_mac->connection_type_lock_);
-    old_type = notifier_mac->connection_type_;
-    notifier_mac->connection_type_ = new_type;
+    base::AutoLock lock(connection_type_lock_);
+    old_type = connection_type_;
+    connection_type_ = new_type;
   }
   if (old_type != new_type) {
     NotifyObserversOfConnectionTypeChange();
-    double max_bandwidth_mbps =
-        NetworkChangeNotifier::GetMaxBandwidthForConnectionSubtype(
-            new_type == CONNECTION_NONE ? SUBTYPE_NONE : SUBTYPE_UNKNOWN);
+    double max_bandwidth_mbps = GetMaxBandwidthForConnectionSubtype(
+        new_type == CONNECTION_NONE ? SUBTYPE_NONE : SUBTYPE_UNKNOWN);
     NotifyObserversOfMaxBandwidthChange(max_bandwidth_mbps, new_type);
   }