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

net/base/network_change_notifier_mac.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/base/network_change_notifier_mac.h"
 
 #include <netinet/in.h>
 #include <resolv.h>
 
 #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"
 
 namespace net {
 
 static bool CalculateReachability(SCNetworkConnectionFlags flags) {
   bool reachable = flags & kSCNetworkFlagsReachable;
   bool connection_required = flags & kSCNetworkFlagsConnectionRequired;
   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 {
  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);
 };
 
 NetworkChangeNotifierMac::NetworkChangeNotifierMac()
     : NetworkChangeNotifier(NetworkChangeCalculatorParamsMac()),
       connection_type_(CONNECTION_UNKNOWN),
       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_));
   dns_config_service_thread_->StartWithOptions(
       base::Thread::Options(base::MessageLoop::TYPE_IO, 0));
 }
 
 NetworkChangeNotifierMac::~NetworkChangeNotifierMac() {
   // Delete the ConfigWatcher to join the notifier thread, ensuring that
   // StartReachabilityNotifications() has an opportunity to run to completion.
@@ skipping 15 matching lines @@
   // 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(500);
   params.ip_address_online_delay_ = base::TimeDelta::FromMilliseconds(500);
   params.connection_type_offline_delay_ =
       base::TimeDelta::FromMilliseconds(1000);
   params.connection_type_online_delay_ = base::TimeDelta::FromMilliseconds(500);
   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;
   base::AutoLock lock(connection_type_lock_);
   // Make sure the initial connection type is set before returning.
   while (!connection_type_initialized_) {
     initial_connection_type_cv_.Wait();
   }
   return connection_type_;
 }
@@ skipping 126 matching lines @@
 #endif  // defined(OS_IOS)
 }
 
 // static
 void NetworkChangeNotifierMac::ReachabilityCallback(
     SCNetworkReachabilityRef target,
     SCNetworkConnectionFlags flags,
     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);
   }
 
 #if defined(OS_IOS)
   // On iOS, the SCDynamicStore API does not exist, and we use the reachability
   // API to detect IP address changes instead.
   NotifyObserversOfIPAddressChange();
 #endif  // defined(OS_IOS)
 }
 
 }  // namespace net