diff --git a/patches/chromium/.patches b/patches/chromium/.patches index 3ed4cfb338..5b63d32414 100644 --- a/patches/chromium/.patches +++ b/patches/chromium/.patches @@ -137,3 +137,4 @@ fix_gate_webstoreprivate_on_a_delegated_availability_check.patch build_drop_the_unused_chrome_browser_win_dep_from_color_mixers.patch fix_plumb_the_requesting_frame_through_pointer_lock_requests.patch feat_let_the_host_of_a_remote_nswindow_set_more_of_its_behavior.patch +device_make_devicemonitormessagewindow_a_nodestructor.patch diff --git a/patches/chromium/device_make_devicemonitormessagewindow_a_nodestructor.patch b/patches/chromium/device_make_devicemonitormessagewindow_a_nodestructor.patch new file mode 100644 index 0000000000..ad98d3e953 --- /dev/null +++ b/patches/chromium/device_make_devicemonitormessagewindow_a_nodestructor.patch @@ -0,0 +1,291 @@ +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 +From: John Kleinschmidt +Date: Thu, 22 Oct 2026 10:32:26 -0400 +Subject: [device] Make DeviceMonitorMessageWindow a NoDestructor + +DeviceMonitorMessageWindow, which owns the DeviceMonitorWin instances, +was deleted by an AtExitManager callback. Any observer still alive at +that point is left holding a dangling pointer to its monitor. + +This happens to SerialDeviceEnumeratorWin::UiThreadHelper at browser +shutdown. DeviceService lives in the UI thread's SequenceLocalStorage, +so it is destroyed by BrowserTaskExecutor::Shutdown(), which runs after +BrowserMainLoop has shut down the ThreadPool. DeviceService holds the +SerialPortManagerImpl in a SequenceBound on a ThreadPool sequence, so +the task that would delete it (and the enumerator and helper it owns) is +dropped. The helper's ScopedObservation is still live when AtExitManager +deletes the monitor, which fails CheckDanglingRawPtrBufferEmpty() at +exit when dangling pointer checks are enabled. + +The AtExit registration dates from when DeviceMonitorMessageWindow owned +its own message window and needed to be destroyed in order with an +AtExit-managed HidService. Neither is true anymore: it listens on +gfx::SingletonHwnd, which is itself a NoDestructor, and HidService is +owned by DeviceService. Make it a base::NoDestructor so the monitors +outlive all of their observers. + +Bug: 572741653 +Change-Id: I85aa5586edab522b6fde5c77a5e4515b848a886b + +Upstreamed at https://chromium-review.googlesource.com/c/chromium/src/+/8561777. +This patch can be removed once that CL lands. + +diff --git a/device/BUILD.gn b/device/BUILD.gn +index 2cdc91e243d010941a6878b26cb164125fc25dcb..741410776eb26868aef2b05d9a3aa042c02b5b0f 100644 +--- a/device/BUILD.gn ++++ b/device/BUILD.gn +@@ -104,6 +104,7 @@ test("device_unittests") { + + if (is_win) { + sources += [ ++ "base/device_monitor_win_unittest.cc", + "bluetooth/bluetooth_adapter_win_unittest.cc", + "bluetooth/bluetooth_device_win_unittest.cc", + "bluetooth/bluetooth_service_record_win_unittest.cc", +diff --git a/device/base/device_monitor_win.cc b/device/base/device_monitor_win.cc +index 72073904d7805ffe60f6077736a548ff83cde78e..ff1858cb5476f116f0595d8cbb850431e2c418ce 100644 +--- a/device/base/device_monitor_win.cc ++++ b/device/base/device_monitor_win.cc +@@ -13,13 +13,13 @@ + #include + #include + +-#include "base/at_exit.h" + #include "base/callback_list.h" + #include "base/compiler_specific.h" + #include "base/containers/span.h" + #include "base/functional/bind.h" +-#include "base/functional/callback_helpers.h" + #include "base/logging.h" ++#include "base/no_destructor.h" ++#include "base/sequence_checker.h" + #include "base/strings/string_util.h" + #include "base/strings/sys_string_conversions.h" + #include "base/task/current_thread.h" +@@ -27,12 +27,8 @@ + + namespace device { + +-class DeviceMonitorMessageWindow; +- + namespace { + +-DeviceMonitorMessageWindow* g_message_window; +- + // Provides basic comparability for GUIDs so that they can be used as keys to an + // STL map. + struct CompareGUID { +@@ -49,19 +45,19 @@ struct CompareGUID { + // for each unique GUID it sees. + class DeviceMonitorMessageWindow { + public: ++ // Returns nullptr if initialization fails. Initialization is retried on the ++ // next call, since it fails when called from a thread without a UI message ++ // pump. + static DeviceMonitorMessageWindow* GetInstance() { +- if (!g_message_window) { +- g_message_window = new DeviceMonitorMessageWindow(); +- if (g_message_window->Init()) { +- base::AtExitManager::RegisterTask( +- base::BindOnce(&base::DeletePointer, +- base::Unretained(g_message_window))); +- } else { +- delete g_message_window; +- g_message_window = nullptr; +- } ++ // Never destroyed, like the gfx::SingletonHwnd it listens on, so that ++ // observers that outlive the AtExitManager are not left with dangling ++ // pointers to the DeviceMonitorWin instances it owns. ++ static base::NoDestructor instance; ++ DCHECK_CALLED_ON_VALID_SEQUENCE(instance->sequence_checker_); ++ if (!instance->initialized_) { ++ instance->initialized_ = instance->Init(); + } +- return g_message_window; ++ return instance->initialized_ ? instance.get() : nullptr; + } + + DeviceMonitorMessageWindow(const DeviceMonitorMessageWindow&) = delete; +@@ -69,6 +65,7 @@ class DeviceMonitorMessageWindow { + delete; + + DeviceMonitorWin* GetForDeviceInterface(const GUID& device_interface) { ++ DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); + std::unique_ptr& device_monitor = + device_monitors_[device_interface]; + if (!device_monitor) { +@@ -77,21 +74,20 @@ class DeviceMonitorMessageWindow { + return device_monitor.get(); + } + +- DeviceMonitorWin* GetForAllInterfaces() { return &all_device_monitor_; } ++ DeviceMonitorWin* GetForAllInterfaces() { ++ DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); ++ return &all_device_monitor_; ++ } + + private: +- friend void base::DeletePointer( +- DeviceMonitorMessageWindow* message_window); ++ friend class base::NoDestructor; + +- DeviceMonitorMessageWindow() {} +- +- ~DeviceMonitorMessageWindow() { +- if (notify_handle_) { +- UnregisterDeviceNotification(notify_handle_); +- } +- } ++ DeviceMonitorMessageWindow() = default; ++ ~DeviceMonitorMessageWindow() = delete; + + bool Init() { ++ DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); ++ + // gfx::SingletonHwnd is a process-wide singleton which only creates its + // window if it is first instantiated on a thread running a UI message + // pump. Bail out before touching it so that a caller on the wrong thread +@@ -107,8 +103,7 @@ class DeviceMonitorMessageWindow { + return false; + } + +- // base::Unretained() is safe because |subscription_| is owned by this +- // object and unregisters the callback when it is destroyed. ++ // base::Unretained() is safe because this object is never destroyed. + subscription_ = + gfx::SingletonHwnd::GetInstance()->RegisterCallback(base::BindRepeating( + &DeviceMonitorMessageWindow::OnWndProc, base::Unretained(this))); +@@ -120,6 +115,7 @@ class DeviceMonitorMessageWindow { + DEVICE_NOTIFY_WINDOW_HANDLE | DEVICE_NOTIFY_ALL_INTERFACE_CLASSES); + if (!notify_handle_) { + PLOG(ERROR) << "Failed to register for device notifications"; ++ subscription_ = {}; + return false; + } + +@@ -127,6 +123,7 @@ class DeviceMonitorMessageWindow { + } + + void OnWndProc(HWND hwnd, UINT message, WPARAM wparam, LPARAM lparam) { ++ DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); + if (message != WM_DEVICECHANGE || + (wparam != DBT_DEVICEARRIVAL && wparam != DBT_DEVICEREMOVECOMPLETE)) { + return; +@@ -168,6 +165,9 @@ class DeviceMonitorMessageWindow { + DeviceMonitorWin all_device_monitor_; + HDEVNOTIFY notify_handle_ = NULL; + base::CallbackListSubscription subscription_; ++ bool initialized_ = false; ++ ++ SEQUENCE_CHECKER(sequence_checker_); + }; + + void DeviceMonitorWin::Observer::OnDeviceAdded( +@@ -202,10 +202,12 @@ DeviceMonitorWin* DeviceMonitorWin::GetForAllInterfaces() { + DeviceMonitorWin::~DeviceMonitorWin() {} + + void DeviceMonitorWin::AddObserver(Observer* observer) { ++ DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); + observer_list_.AddObserver(observer); + } + + void DeviceMonitorWin::RemoveObserver(Observer* observer) { ++ DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); + observer_list_.RemoveObserver(observer); + } + +@@ -213,6 +215,7 @@ DeviceMonitorWin::DeviceMonitorWin() {} + + void DeviceMonitorWin::NotifyDeviceAdded(const GUID& class_guid, + const std::wstring& device_path) { ++ DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); + for (auto& observer : observer_list_) { + observer.OnDeviceAdded(class_guid, device_path); + } +@@ -220,6 +223,7 @@ void DeviceMonitorWin::NotifyDeviceAdded(const GUID& class_guid, + + void DeviceMonitorWin::NotifyDeviceRemoved(const GUID& class_guid, + const std::wstring& device_path) { ++ DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); + for (auto& observer : observer_list_) { + observer.OnDeviceRemoved(class_guid, device_path); + } +diff --git a/device/base/device_monitor_win.h b/device/base/device_monitor_win.h +index 0b1c2a4edd9cccc22cdd1b947b35f8e86adccb65..1ada9645dd940d49b5a19fc2699c79079fb5db82 100644 +--- a/device/base/device_monitor_win.h ++++ b/device/base/device_monitor_win.h +@@ -8,6 +8,7 @@ + #include + + #include "base/observer_list.h" ++#include "base/sequence_checker.h" + #include "base/win/windows_types.h" + #include "device/base/device_base_export.h" + +@@ -48,6 +49,10 @@ class DEVICE_BASE_EXPORT DeviceMonitorWin { + void NotifyDeviceRemoved(const GUID& class_guid, + const std::wstring& device_path); + ++ // `observer_list_` is not thread-safe, so observers must be added and ++ // removed on the sequence that delivers notifications. ++ SEQUENCE_CHECKER(sequence_checker_); ++ + base::ObserverList::Unchecked observer_list_; + }; + +diff --git a/device/base/device_monitor_win_unittest.cc b/device/base/device_monitor_win_unittest.cc +new file mode 100644 +index 0000000000000000000000000000000000000000..fea61c4b0a98d31bcd1174425746076addd91ae5 +--- /dev/null ++++ b/device/base/device_monitor_win_unittest.cc +@@ -0,0 +1,50 @@ ++// 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 "device/base/device_monitor_win.h" ++ ++#include "base/at_exit.h" ++#include "base/scoped_observation.h" ++#include "base/test/task_environment.h" ++#include "testing/gtest/include/gtest/gtest.h" ++ ++namespace device { ++ ++namespace { ++ ++class TestObserver : public DeviceMonitorWin::Observer { ++ public: ++ void Observe(DeviceMonitorWin* monitor) { observation_.Observe(monitor); } ++ void Reset() { observation_.Reset(); } ++ ++ private: ++ base::ScopedObservation ++ observation_{this}; ++}; ++ ++} // namespace ++ ++// Observers can outlive the AtExitManager (e.g. the serial enumerator's UI ++// thread helper, which is leaked at browser shutdown), so the monitors must ++// not be destroyed by it. If they were, `observer` would be left with a ++// dangling pointer to the monitor and would call RemoveObserver() on it after ++// it was freed. ++TEST(DeviceMonitorWinTest, MonitorOutlivesAtExitManager) { ++ base::test::SingleThreadTaskEnvironment task_environment( ++ base::test::SingleThreadTaskEnvironment::MainThreadType::UI); ++ ++ TestObserver observer; ++ DeviceMonitorWin* monitor = nullptr; ++ { ++ base::ShadowingAtExitManager at_exit_manager; ++ monitor = DeviceMonitorWin::GetForAllInterfaces(); ++ ASSERT_TRUE(monitor); ++ observer.Observe(monitor); ++ } ++ ++ EXPECT_EQ(monitor, DeviceMonitorWin::GetForAllInterfaces()); ++ observer.Reset(); ++} ++ ++} // namespace device