[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;
+  static ConnectionType RecomputeCurrentConnectionType();

[pauljensen, 2017/05/23 17:57:40]

can we leave this virtual since NCNWin outlives dns thread? then tests can
overload it, and we can get rid of calculate_connection_type_

[jkarlin, 2017/05/23 18:02:15]

NCNWin outlives dns thread, but there is no guarantee that the
NetworkChangeNotifierWin will still be alive by the time the reply task is run.

E.g.,

1. On main thread NCN PostTaskAndReply's to dns thread
2. dns thread does its thing and posts a task on the main thread
3. main thread runs a task to delete the NCN
4. main thread runs the reply task, but the NCN is gone

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

can it post back with a weakptr?

[pauljensen, 2017/05/23 18:06:47]

I think this is very similar to the example here:
https://chromium.googlesource.com/chromium/src/+/master/docs/threading_and_ta...

[jkarlin, 2017/05/26 15:09:55]

Back to the original.

 
   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_;
 
   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_