Fix potential use-after-free in NetworkChangeNotifierWin shutdown

Make NetworkChangeNotifierWin::RecomputeCurrentConnectionType() static
to avoid referencing NetworkChangeNotifierWin vtable pointer when it
might be dead.  This can happen when RecomputeCurrentConnectionType()
is posted to |blocking_task_runner_| and NetworkChangeNotifierWin is
destroyed.

Bug: 995781
Change-Id: I56805777fb77a5b37829962e41d4d0b0e09eb468
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1789666
Commit-Queue: Eric Orth <ericorth@chromium.org>
Auto-Submit: Paul Jensen <pauljensen@chromium.org>
Reviewed-by: Eric Orth <ericorth@chromium.org>
Cr-Commit-Position: refs/heads/master@{#695799}

net/base/network_change_notifier_win.cc

@@ -119,8 +119,9 @@ NetworkChangeNotifierWin::NetworkChangeCalculatorParamsWin() {
 // experiments I ran... However none of them correctly returned "offline" when
 // executing 'ipconfig /release'.
 //
+// static
 NetworkChangeNotifier::ConnectionType
-NetworkChangeNotifierWin::RecomputeCurrentConnectionType() const {
+NetworkChangeNotifierWin::RecomputeCurrentConnectionType() {
   EnsureWinsockInit();
 
   // The following code was adapted from:
@@ -194,8 +195,7 @@ void NetworkChangeNotifierWin::RecomputeCurrentConnectionTypeOnBlockingSequence(
   // thread is stopped in this object's destructor.
   base::PostTaskAndReplyWithResult(
       blocking_task_runner_.get(), FROM_HERE,
-      base::BindOnce(&NetworkChangeNotifierWin::RecomputeCurrentConnectionType,
-                     base::Unretained(this)),
+      base::BindOnce(&NetworkChangeNotifierWin::RecomputeCurrentConnectionType),
       std::move(reply_callback));
 }
 

net/base/network_change_notifier_win.h

@@ -53,6 +53,7 @@ class NET_EXPORT_PRIVATE NetworkChangeNotifierWin
 
  private:
   friend class NetworkChangeNotifierWinTest;
+  friend class TestNetworkChangeNotifierWin;
 
   // NetworkChangeNotifier methods:
   ConnectionType GetCurrentConnectionType() const override;
@@ -63,7 +64,7 @@ class NET_EXPORT_PRIVATE NetworkChangeNotifierWin
 
   // 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();
 
   // Calls RecomputeCurrentConnectionTypeImpl on the DNS sequence and runs
   // |reply_callback| with the type on the calling sequence.

net/base/network_change_notifier_win_unittest.cc

@@ -24,13 +24,14 @@ using ::testing::StrictMock;
 
 namespace net {
 
-namespace {
-
 // Subclass of NetworkChangeNotifierWin that overrides functions so that no
-// Windows API networking functions are ever called.
+// Windows API networking function results effect tests.
 class TestNetworkChangeNotifierWin : public NetworkChangeNotifierWin {
  public:
-  TestNetworkChangeNotifierWin() {}
+  TestNetworkChangeNotifierWin() {
+    last_computed_connection_type_ = NetworkChangeNotifier::CONNECTION_UNKNOWN;
+    last_announced_offline_ = false;
+  }
 
   ~TestNetworkChangeNotifierWin() override {
     // This is needed so we don't try to stop watching for IP address changes,
@@ -38,12 +39,6 @@ class TestNetworkChangeNotifierWin : public NetworkChangeNotifierWin {
     set_is_watching(false);
   }
 
-  // From NetworkChangeNotifierWin.
-  NetworkChangeNotifier::ConnectionType RecomputeCurrentConnectionType()
-      const override {
-    return NetworkChangeNotifier::CONNECTION_UNKNOWN;
-  }
-
   // From NetworkChangeNotifierWin.
   void RecomputeCurrentConnectionTypeOnBlockingSequence(
       base::OnceCallback<void(ConnectionType)> reply_callback) const override {
@@ -80,8 +75,6 @@ bool ExitMessageLoopAndReturnFalse() {
   return false;
 }
 
-}  // namespace
-
 class NetworkChangeNotifierWinTest : public TestWithTaskEnvironment {
  public:
   // Calls WatchForAddressChange, and simulates a WatchForAddressChangeInternal