From 1e50913640a4634cf17da6be37c6a67fc166b975 Mon Sep 17 00:00:00 2001 From: szymonmazanik <25953768+szymonmazanik@users.noreply.github.com> Date: Wed, 23 Sep 2026 14:13:22 +0200 Subject: [PATCH] fix: preserve shortcut and window hook callback lifetimes Clear shortcut callbacks after successful unregistration, outside the manager mutex. Retained handles can no longer invoke stale callbacks. Keep a local copy while invoking shortcuts and desktop window hooks so self-replacement or removal does not destroy executing captures. Add CTest coverage for self-replacement, hook removal, and retained handles. --- src/platform/linux/window_manager_linux.cpp | 12 +- src/platform/macos/window_manager_macos.mm | 12 +- .../windows/window_manager_windows.cpp | 12 +- src/shortcut.cpp | 6 +- src/shortcut_manager.cpp | 5 + tests/CMakeLists.txt | 5 + tests/callback_lifetime_test.cpp | 104 ++++++++++++++++++ 7 files changed, 142 insertions(+), 14 deletions(-) create mode 100644 tests/callback_lifetime_test.cpp diff --git a/src/platform/linux/window_manager_linux.cpp b/src/platform/linux/window_manager_linux.cpp index 3e31c16..facf9be 100644 --- a/src/platform/linux/window_manager_linux.cpp +++ b/src/platform/linux/window_manager_linux.cpp @@ -901,14 +901,18 @@ bool WindowManager::HasWillHideHook() const { } void WindowManager::HandleWillShow(WindowId id) { - if (pimpl_->will_show_hook_) { - (*pimpl_->will_show_hook_)(id); + // The hook may replace or clear itself. Keep this invocation alive. + auto hook = pimpl_->will_show_hook_; + if (hook) { + (*hook)(id); } } void WindowManager::HandleWillHide(WindowId id) { - if (pimpl_->will_hide_hook_) { - (*pimpl_->will_hide_hook_)(id); + // The hook may replace or clear itself. Keep this invocation alive. + auto hook = pimpl_->will_hide_hook_; + if (hook) { + (*hook)(id); } } diff --git a/src/platform/macos/window_manager_macos.mm b/src/platform/macos/window_manager_macos.mm index 90b946e..5d0d041 100644 --- a/src/platform/macos/window_manager_macos.mm +++ b/src/platform/macos/window_manager_macos.mm @@ -526,14 +526,18 @@ static WindowId ResolveWindowId(NSWindow* ns_window) { } void WindowManager::HandleWillShow(WindowId id) { - if (pimpl_->will_show_hook_) { - (*pimpl_->will_show_hook_)(id); + // The hook may replace or clear itself. Keep this invocation alive. + auto hook = pimpl_->will_show_hook_; + if (hook) { + (*hook)(id); } } void WindowManager::HandleWillHide(WindowId id) { - if (pimpl_->will_hide_hook_) { - (*pimpl_->will_hide_hook_)(id); + // The hook may replace or clear itself. Keep this invocation alive. + auto hook = pimpl_->will_hide_hook_; + if (hook) { + (*hook)(id); } } diff --git a/src/platform/windows/window_manager_windows.cpp b/src/platform/windows/window_manager_windows.cpp index 4c01da3..bf88643 100644 --- a/src/platform/windows/window_manager_windows.cpp +++ b/src/platform/windows/window_manager_windows.cpp @@ -892,14 +892,18 @@ bool WindowManager::HasWillHideHook() const { } void WindowManager::HandleWillShow(WindowId id) { - if (pimpl_->will_show_hook_) { - (*pimpl_->will_show_hook_)(id); + // The hook may replace or clear itself. Keep this invocation alive. + auto hook = pimpl_->will_show_hook_; + if (hook) { + (*hook)(id); } } void WindowManager::HandleWillHide(WindowId id) { - if (pimpl_->will_hide_hook_) { - (*pimpl_->will_hide_hook_)(id); + // The hook may replace or clear itself. Keep this invocation alive. + auto hook = pimpl_->will_hide_hook_; + if (hook) { + (*hook)(id); } } diff --git a/src/shortcut.cpp b/src/shortcut.cpp index 885b36d..93a2585 100644 --- a/src/shortcut.cpp +++ b/src/shortcut.cpp @@ -49,8 +49,10 @@ bool Shortcut::IsEnabled() const { } void Shortcut::Invoke() { - if (enabled_ && callback_) { - callback_(); + // A callback may replace or clear itself while it is executing. + auto callback = callback_; + if (enabled_ && callback) { + callback(); } } diff --git a/src/shortcut_manager.cpp b/src/shortcut_manager.cpp index 0f67d45..528ed79 100644 --- a/src/shortcut_manager.cpp +++ b/src/shortcut_manager.cpp @@ -89,6 +89,11 @@ bool ShortcutManager::Unregister(ShortcutId id) { shortcuts_by_id_.erase(it); shortcuts_by_accelerator_.erase(accelerator); + // Destroying captured objects may re-enter the manager. Do it outside the lock. + lock.unlock(); + // Retained handles must not invoke a callback after its registration ends. + shortcut->SetCallback(nullptr); + // Emit event EmitAsync(id, accelerator); diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 4add9a1..bbc9ee8 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -39,6 +39,11 @@ add_executable(shortcut_accelerator_test shortcut_accelerator_test.cpp) target_link_libraries(shortcut_accelerator_test PRIVATE nativeapi) add_test(NAME shortcut_accelerator_test COMMAND shortcut_accelerator_test) +add_executable(callback_lifetime_test callback_lifetime_test.cpp) +target_link_libraries(callback_lifetime_test PRIVATE nativeapi) +add_test(NAME callback_lifetime_test COMMAND callback_lifetime_test) +set_tests_properties(callback_lifetime_test PROPERTIES TIMEOUT 20) + # The registry side of launch-at-login: the Run value, the StartupApproved flag that # Task Manager toggles, and a non-ASCII identifier. Writes only under HKCU, under a # test-only identifier, and removes what it writes. diff --git a/tests/callback_lifetime_test.cpp b/tests/callback_lifetime_test.cpp new file mode 100644 index 0000000..094a570 --- /dev/null +++ b/tests/callback_lifetime_test.cpp @@ -0,0 +1,104 @@ +// Callbacks must keep their captured objects alive while replacing or clearing +// themselves. Unregistering a shortcut must release its callback even if the +// caller still owns the shortcut. +// +// The registration case needs a desktop session and skips if the platform cannot +// register the accelerator. The self-replacement cases run without registration. + +#include +#include +#include + +#include "../src/shortcut.h" +#include "../src/shortcut_manager.h" +#include "../src/window_manager.h" + +namespace { + +int g_failures = 0; + +void Check(bool condition, const char* message) { + if (!condition) { + std::cerr << "FAIL: " << message << std::endl; + ++g_failures; + } +} + +void TestShortcutSelfReplacement() { + nativeapi::Shortcut shortcut(1, "Ctrl+A", nullptr); + auto captured = std::make_shared(42); + std::weak_ptr weak = captured; + int replacements = 0; + shortcut.SetCallback([&, captured] { + shortcut.SetCallback([&] { ++replacements; }); + Check(!weak.expired(), "active shortcut callback was destroyed during replacement"); + }); + captured.reset(); + shortcut.Invoke(); + Check(weak.expired(), "replaced shortcut callback was retained"); + shortcut.Invoke(); + Check(replacements == 1, "replacement callback was not installed"); + shortcut.SetCallback(nullptr); +} + +void TestHookSelfRemoval() { + auto& manager = nativeapi::WindowManager::GetInstance(); + for (bool show : {true, false}) { + auto captured = std::make_shared(42); + std::weak_ptr weak = captured; + auto callback = [&, captured](nativeapi::WindowId) { + if (show) { + manager.SetWillShowHook(std::nullopt); + } else { + manager.SetWillHideHook(std::nullopt); + } + Check(!weak.expired(), "active window hook was destroyed during removal"); + }; + if (show) { + manager.SetWillShowHook(std::move(callback)); + } else { + manager.SetWillHideHook(std::move(callback)); + } + captured.reset(); + if (show) { + manager.HandleWillShow(0); + } else { + manager.HandleWillHide(0); + } + Check(weak.expired(), "removed window hook was retained"); + } +} + +void TestUnregisterClearsRetainedShortcut() { + auto& manager = nativeapi::ShortcutManager::GetInstance(); + int calls = 0; + auto captured = std::make_shared(42); + std::weak_ptr weak = captured; + auto shortcut = manager.Register("Ctrl+Alt+Shift+F12", [&, captured] { ++calls; }); + captured.reset(); + if (!shortcut) { + std::cout << "SKIP: platform could not register a global shortcut" << std::endl; + return; + } + shortcut->Invoke(); + Check(calls == 1, "registered shortcut callback did not run"); + Check(manager.Unregister(shortcut->GetId()), "shortcut unregister failed"); + shortcut->Invoke(); + Check(calls == 1, "retained shortcut invoked after unregister"); + Check(weak.expired(), "unregistered shortcut retained its captured state"); +} +} // namespace + +int main() { + std::cout << "callback_lifetime_test" << std::endl; + + TestShortcutSelfReplacement(); + TestHookSelfRemoval(); + TestUnregisterClearsRetainedShortcut(); + if (g_failures > 0) { + std::cerr << g_failures << " check(s) failed" << std::endl; + return EXIT_FAILURE; + } + std::cout << "All checks passed." << std::endl; + return EXIT_SUCCESS; +}