[NetworkChangeNotifier] Run Windows connection type computation on other thread

net/base/network_change_notifier_win.h

 // 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.
 
 #ifndef NET_BASE_NETWORK_CHANGE_NOTIFIER_WIN_H_
 #define NET_BASE_NETWORK_CHANGE_NOTIFIER_WIN_H_
 
 #include <windows.h>
 
 #include <memory>
 
+#include "base/callback.h"
 #include "base/compiler_specific.h"
 #include "base/macros.h"
+#include "base/memory/ref_counted.h"
 #include "base/memory/weak_ptr.h"
 #include "base/threading/non_thread_safe.h"
 #include "base/timer/timer.h"
 #include "base/win/object_watcher.h"
 #include "net/base/net_export.h"
 #include "net/base/network_change_notifier.h"
 
+namespace base {
+class SingleThreadTaskRunner;
+}  // namespace base
+
 namespace net {
 
 // NetworkChangeNotifierWin inherits from NonThreadSafe, as all its internal
 // notification code must be called on the thread it is created and destroyed
 // on.  All the NetworkChangeNotifier methods it implements are threadsafe.
 class NET_EXPORT_PRIVATE NetworkChangeNotifierWin
     : public NetworkChangeNotifier,
       public base::win::ObjectWatcher::Delegate,
       NON_EXPORTED_BASE(public base::NonThreadSafe) {
  public:
@@ skipping 12 matching lines @@
   ~NetworkChangeNotifierWin() override;
 
   // For unit tests only.
   bool is_watching() { return is_watching_; }
   void set_is_watching(bool is_watching) { is_watching_ = is_watching; }
   int sequential_failures() { return sequential_failures_; }
 
  private:
   class DnsConfigServiceThread;
   friend class NetworkChangeNotifierWinTest;
+  friend class TestNetworkChangeNotifierWin;
 
   // NetworkChangeNotifier methods:
   ConnectionType GetCurrentConnectionType() const override;
 
   // ObjectWatcher::Delegate methods:
   // Must only be called on the thread |this| was created on.
   void OnObjectSignaled(HANDLE object) override;
 
-  // Does the actual work to determine the current connection type.
-  // It is not thread safe, see crbug.com/324913.
-  virtual ConnectionType RecomputeCurrentConnectionType() const;
-
   void SetCurrentConnectionType(ConnectionType connection_type);
 
   // Notifies IP address change observers of a change immediately, and notifies
   // network state change observers on a delay.  Must only be called on the
   // thread |this| was created on.
-  void NotifyObservers();
+  void NotifyObservers(ConnectionType connection_type);
 
   // Forwards connection type notifications to parent class.
   void NotifyParentOfConnectionTypeChange();
+  void NotifyParentOfConnectionTypeChangeImpl(ConnectionType connection_type);
 
   // Tries to start listening for a single subsequent address change.  Returns
   // false on failure.  The caller is responsible for updating |is_watching_|.
   // Virtual for unit tests.  Must only be called on the thread |this| was
   // created on.
   virtual bool WatchForAddressChangeInternal();
 
   static NetworkChangeCalculatorParams NetworkChangeCalculatorParamsWin();
 
   // All member variables may only be accessed on the thread |this| was created
   // on.
 
   // False when not currently watching for network change events.  This only
   // happens on initialization and when WatchForAddressChangeInternal fails and
   // there is a pending task to try again.  Needed for safe cleanup.
   bool is_watching_;
 
   base::win::ObjectWatcher addr_watcher_;
   OVERLAPPED addr_overlapped_;
 
   base::OneShotTimer timer_;
 
   // Number of times WatchForAddressChange has failed in a row.
   int sequential_failures_;
 
   // Thread on which we can run DnsConfigService.
   std::unique_ptr<DnsConfigServiceThread> dns_config_service_thread_;
+  scoped_refptr<base::SingleThreadTaskRunner> dns_task_runner_;

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

Hmm, can we avoid dns_task_runner_ and calculate_connection_type_ by instead
moving the DnsConfigServiceThread decl into this file, then having the test
reset dns_config_service_thread_ to be some overloaded impl in tests?

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

We could get rid of dns_task_runner_ that way I suppose, but injecting task
runners is pretty standard procedure and in this case seems pretty reasonable.

We could get rid of calculate_connection_type_ if we made it a virtual member
function of DnsConfigServiceThread and made DnsConfigServiceThread ref counted.
Would that be preferable? I'm not so sure as it changes the meaning of the
class.

[pauljensen, 2017/05/23 14:55:50]

Why would DnsConfigServiceThread need to be ref counted?  It's owned by NCNWin.

[jkarlin, 2017/05/23 15:11:06]

Ah, you're right. All tasks on the dns thread will finish running (thanks to the
call to Stop()) before the dns thread is destroyed, so it's safe. 

Can be done, but I don't see a strong reason to prefer that over what's in the
CL.

 
   mutable base::Lock last_computed_connection_type_lock_;
   ConnectionType last_computed_connection_type_;
 
   // Result of IsOffline() when NotifyObserversOfConnectionTypeChange()
   // was last called.
   bool last_announced_offline_;
   // Number of times polled to check if still offline.
   int offline_polls_;
 
+  // The callback run to calculate connection type.
+  base::Callback<ConnectionType()> calculate_connection_type_;
+
   // Used for calling WatchForAddressChange again on failure.
   base::WeakPtrFactory<NetworkChangeNotifierWin> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(NetworkChangeNotifierWin);
 };
 
 }  // namespace net
 
 #endif  // NET_BASE_NETWORK_CHANGE_NOTIFIER_WIN_H_