From b9cd09174145163e90936140ca3485c1b6ae5a78 Mon Sep 17 00:00:00 2001
From: Keita Suzuki <suzukikeita@chromium.org>
Date: Thu, 02 Jul 2026 23:56:32 -0700
Subject: [PATCH] [wifi] Bind WeakPtr from WiFiServiceMac SSID-change observer

WiFiServiceMac registers an NSNotificationCenter block for CoreWLAN
SSID-change notifications with `queue:nil`, so the block runs on the
posting (CoreWLAN) thread. The block was referencing the `task_runner_`
member, implicitly capturing `this`, and posting
OnWlanObserverNotification bound with base::Unretained. Because the
service is destroyed on the same worker sequence the block posts to, a
notification delivered around teardown can leave a queued task that runs
after the service is gone.

Capture the worker task runner by value and bind a
base::WeakPtr<WiFiServiceMac> instead of base::Unretained so
late-arriving notifications are dropped once the service has been
destroyed.

Adds //components/wifi:unit_tests with a regression test that posts the
SSID-change notification, destroys the service, then drains the run
loop.

TAG=agy
CONV=13ec7a8f-4d73-4c01-b1b7-4d465e984eb3

Bug: 522550059
Change-Id: I60fb685c1ca7785829b0ffc3a31bef08099e8d3e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8018224
Commit-Queue: Keita Suzuki <suzukikeita@chromium.org>
Reviewed-by: Kenichi Ishibashi <bashi@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1656384}
---

diff --git a/components/BUILD.gn b/components/BUILD.gn
index f238dd0c..f4d9aa03 100644
--- a/components/BUILD.gn
+++ b/components/BUILD.gn
@@ -974,6 +974,7 @@
       "//components/enterprise/platform_auth:unit_tests",
       "//components/remote_cocoa/app_shim:unit_tests",
       "//components/remote_cocoa/common:unit_tests",
+      "//components/wifi:unit_tests",
     ]
   }
 
diff --git a/components/wifi/BUILD.gn b/components/wifi/BUILD.gn
index c6c7a5b..4c3dcce 100644
--- a/components/wifi/BUILD.gn
+++ b/components/wifi/BUILD.gn
@@ -68,3 +68,19 @@
     "//components/onc",
   ]
 }
+
+source_set("unit_tests") {
+  testonly = true
+  sources = []
+  deps = [
+    ":wifi",
+    "//base",
+    "//base/test:test_support",
+    "//testing/gtest",
+  ]
+
+  if (is_mac) {
+    sources += [ "wifi_service_mac_unittest.mm" ]
+    frameworks = [ "Foundation.framework" ]
+  }
+}
diff --git a/components/wifi/wifi_service_mac.mm b/components/wifi/wifi_service_mac.mm
index 216f1fd..789c7ec5 100644
--- a/components/wifi/wifi_service_mac.mm
+++ b/components/wifi/wifi_service_mac.mm
@@ -17,6 +17,7 @@
 #include "base/apple/scoped_cftyperef.h"
 #include "base/compiler_specific.h"
 #include "base/functional/bind.h"
+#include "base/memory/weak_ptr.h"
 #include "base/strings/string_view_util.h"
 #include "base/strings/sys_string_conversions.h"
 #import "base/task/sequenced_task_runner.h"
@@ -164,6 +165,8 @@
   std::string connected_network_guid_;
   // Temporary storage of network properties indexed by |network_guid|.
   base::DictValue network_properties_;
+
+  base::WeakPtrFactory<WiFiServiceMac> weak_factory_{this};
 };
 
 WiFiServiceMac::WiFiServiceMac() = default;
@@ -389,12 +392,18 @@
 
   // Subscribe to OS notifications.
   if (!networks_changed_observer_.is_null()) {
+    // CoreWLAN delivers SSID-change notifications on its own dispatch queue,
+    // so the block must not reference `this` directly. Capture the task runner
+    // by value and bind a weak pointer so any task that lands after this
+    // object has been destroyed on the worker sequence is dropped.
+    scoped_refptr<base::SequencedTaskRunner> worker_task_runner = task_runner_;
+    base::WeakPtr<WiFiServiceMac> weak_this = weak_factory_.GetWeakPtr();
     void (^ns_observer)(NSNotification* notification) = ^(
         NSNotification* notification) {
       DVLOG(1) << "Received CoreWLAN notification that the SSID changed";
-      task_runner_->PostTask(
+      worker_task_runner->PostTask(
           FROM_HERE, base::BindOnce(&WiFiServiceMac::OnWlanObserverNotification,
-                                    base::Unretained(this)));
+                                    weak_this));
     };
 
     // A notification with the symbol kCWSSIDDidChangeNotification started being
diff --git a/components/wifi/wifi_service_mac_unittest.mm b/components/wifi/wifi_service_mac_unittest.mm
new file mode 100644
index 0000000..6732bd4f
--- /dev/null
+++ b/components/wifi/wifi_service_mac_unittest.mm
@@ -0,0 +1,62 @@
+// Copyright 2026 The Chromium Authors
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "components/wifi/wifi_service.h"
+
+#import <Foundation/Foundation.h>
+
+#include <memory>
+
+#include "base/functional/callback_helpers.h"
+#include "base/memory/scoped_refptr.h"
+#include "base/run_loop.h"
+#include "base/task/sequenced_task_runner.h"
+#include "base/task/single_thread_task_runner.h"
+#include "base/test/task_environment.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace wifi {
+
+namespace {
+
+// Matches the notification name observed by WiFiServiceMac::SetEventObservers.
+NSNotificationName const kCoreWLANSSIDChangedNotification =
+    @"com.apple.coreWLAN.notification.ssid.legacy";
+
+}  // namespace
+
+using WiFiServiceMacTest = testing::Test;
+
+// CoreWLAN posts SSID-change notifications from a background dispatch queue,
+// so the observer block can run concurrently with the WiFiService being torn
+// down on its worker sequence. Any task the block posts to the worker sequence
+// may therefore run after the service has been destroyed and must not touch
+// it.
+TEST_F(WiFiServiceMacTest, NotificationDeliveredAfterServiceDestroyed) {
+  base::test::SingleThreadTaskEnvironment task_environment;
+
+  std::unique_ptr<WiFiService> wifi_service(WiFiService::Create());
+  wifi_service->Initialize(base::SequencedTaskRunner::GetCurrentDefault());
+  wifi_service->SetEventObservers(
+      base::SingleThreadTaskRunner::GetCurrentDefault(),
+      /*networks_changed_observer=*/base::DoNothing(),
+      /*network_list_changed_observer=*/base::DoNothing());
+
+  // The observer is registered with `queue:nil`, so the block is invoked
+  // synchronously on the posting thread and queues its work on the worker
+  // sequence before destruction below.
+  [NSNotificationCenter.defaultCenter
+      postNotificationName:kCoreWLANSSIDChangedNotification
+                    object:nil];
+
+  wifi_service.reset();
+
+  // Draining the queued work must not touch the destroyed service.
+  base::RunLoop run_loop;
+  base::SingleThreadTaskRunner::GetCurrentDefault()->PostTask(
+      FROM_HERE, run_loop.QuitClosure());
+  run_loop.Run();
+}
+
+}  // namespace wifi

diff --git a/components/wifi/wifi_service_mac_unittest.mm b/components/wifi/wifi_service_mac_unittest.mm
new file mode 100644
index 0000000..6732bd4f
--- /dev/null
+++ b/components/wifi/wifi_service_mac_unittest.mm
@@ -0,0 +1,62 @@
+// Copyright 2026 The Chromium Authors
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "components/wifi/wifi_service.h"
+
+#import <Foundation/Foundation.h>
+
+#include <memory>
+
+#include "base/functional/callback_helpers.h"
+#include "base/memory/scoped_refptr.h"
+#include "base/run_loop.h"
+#include "base/task/sequenced_task_runner.h"
+#include "base/task/single_thread_task_runner.h"
+#include "base/test/task_environment.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace wifi {
+
+namespace {
+
+// Matches the notification name observed by WiFiServiceMac::SetEventObservers.
+NSNotificationName const kCoreWLANSSIDChangedNotification =
+    @"com.apple.coreWLAN.notification.ssid.legacy";
+
+}  // namespace
+
+using WiFiServiceMacTest = testing::Test;
+
+// CoreWLAN posts SSID-change notifications from a background dispatch queue,
+// so the observer block can run concurrently with the WiFiService being torn
+// down on its worker sequence. Any task the block posts to the worker sequence
+// may therefore run after the service has been destroyed and must not touch
+// it.
+TEST_F(WiFiServiceMacTest, NotificationDeliveredAfterServiceDestroyed) {
+  base::test::SingleThreadTaskEnvironment task_environment;
+
+  std::unique_ptr<WiFiService> wifi_service(WiFiService::Create());
+  wifi_service->Initialize(base::SequencedTaskRunner::GetCurrentDefault());
+  wifi_service->SetEventObservers(
+      base::SingleThreadTaskRunner::GetCurrentDefault(),
+      /*networks_changed_observer=*/base::DoNothing(),
+      /*network_list_changed_observer=*/base::DoNothing());
+
+  // The observer is registered with `queue:nil`, so the block is invoked
+  // synchronously on the posting thread and queues its work on the worker
+  // sequence before destruction below.
+  [NSNotificationCenter.defaultCenter
+      postNotificationName:kCoreWLANSSIDChangedNotification
+                    object:nil];
+
+  wifi_service.reset();
+
+  // Draining the queued work must not touch the destroyed service.
+  base::RunLoop run_loop;
+  base::SingleThreadTaskRunner::GetCurrentDefault()->PostTask(
+      FROM_HERE, run_loop.QuitClosure());
+  run_loop.Run();
+}
+
+}  // namespace wifi