From 6a7eba6a98de15df2dbcd625e333aa556366ad2c Mon Sep 17 00:00:00 2001 From: Loren Eteval Date: Tue, 6 Oct 2026 14:34:56 +0800 Subject: [PATCH] Fix reentrant Qt lifetimes Signed-off-by: Loren Eteval --- Furious/Qt/AGENTS.md | 3 + Furious/Qt/QtGui.py | 11 ++- Furious/Qt/QtWidgets.py | 38 ++++++++++ Furious/Qt/Signals.py | 13 +++- Furious/Qt/ThemeTransition.py | 33 ++++++++- tests/test_qt_lifetime.py | 131 +++++++++++++++++++++++++++++++++ tests/test_theme_transition.py | 120 +++++++++++++++++++++++++++++- 7 files changed, 340 insertions(+), 9 deletions(-) diff --git a/Furious/Qt/AGENTS.md b/Furious/Qt/AGENTS.md index f217b06..82347f1 100644 --- a/Furious/Qt/AGENTS.md +++ b/Furious/Qt/AGENTS.md @@ -62,6 +62,9 @@ behavior, and lifetime primitives; pages and services consume them without creat workflow owner. None replaces the strong owner required while asynchronous UI remains active. For independent sender/receiver trees, test both destruction orders: receiver cleanup must disconnect its edge, and sender cleanup must retire receiver-side tracking without keeping a signal wrapper or sender alive. + Replacing a borrowed sender retires the whole registration, including both lifecycle hooks. `connectWeakly()` can + collect these opaque handles in a fresh per-connection list for explicit retirement; its ordinary return remains + the main Qt connection. Borrowers clear references on either native teardown without stealing a shared Qt owner. - `AppQAction.callback` is strong by design, so its owner must not outlive the captured receiver; construction alone does not enforce that requirement. An action also owns a submenu supplied without a QWidget parent and schedules its native deletion when the action dies; diff --git a/Furious/Qt/QtGui.py b/Furious/Qt/QtGui.py index 9a6d926..e0e39f4 100644 --- a/Furious/Qt/QtGui.py +++ b/Furious/Qt/QtGui.py @@ -26,6 +26,8 @@ from Furious.Qt.Signals import connectWeakly from PySide6 import QtCore from PySide6.QtGui import * +from shiboken6 import isValid + import logging import functools @@ -147,6 +149,8 @@ class AppQAction(Mixins.QTranslatable, Mixins.ThemeAware, QAction): # Create reference self._menu = menu + connectWeakly(menu.destroyed, self, '_clearMenu', sender=menu) + if menu.parent() is None: # setMenu() associates a submenu without giving it a Qt parent. # This action owns otherwise unparented menus; explicit widget @@ -203,7 +207,12 @@ class AppQAction(Mixins.QTranslatable, Mixins.ThemeAware, QAction): if callable(self.callback): self.callback() - self.triggeredCallback(paramChecked) + if isValid(self): + self.triggeredCallback(paramChecked) + + def _clearMenu(self, *_args): + """Drop a borrowed submenu wrapper when its native owner destroys it.""" + self._menu = None def addAction(self, action): """Add action.""" diff --git a/Furious/Qt/QtWidgets.py b/Furious/Qt/QtWidgets.py index 275bfde..1de2588 100644 --- a/Furious/Qt/QtWidgets.py +++ b/Furious/Qt/QtWidgets.py @@ -36,6 +36,7 @@ from shiboken6 import isValid from typing import Union +import weakref import functools __all__ = [ @@ -2193,6 +2194,14 @@ class AppQPushButton(Mixins.QTranslatable, Mixins.ThemeAware, QPushButton): self.setToolTip(_(self.toolTip())) +def _releasePopupMenuReference(reference, *_args): + """End borrowing even if compiled callbacks retain the dead button wrapper.""" + button = reference() + + if button is not None: + button._popupMenu = None + + class AppQMenuPushButton(AppQPushButton): """Open a popup menu from a regular Fluent-style push button.""" @@ -2201,6 +2210,11 @@ class AppQMenuPushButton(AppQPushButton): super().__init__(*args, **kwargs) self._popupMenu = None + self._popupMenuConnections = [] + + self.destroyed.connect( + functools.partial(_releasePopupMenuReference, weakref.ref(self)) + ) self.setPopupMenu(popupMenu) connectWeakly(self.clicked, self, 'showPopupMenu') @@ -2214,8 +2228,32 @@ class AppQMenuPushButton(AppQPushButton): if menu is not None and not isinstance(menu, QMenu): raise TypeError('popupMenu must be a QMenu or None') + if menu is not None and not isValid(menu): + raise ValueError('popupMenu must be a valid QMenu') + + if menu is self._popupMenu: + return + + # Retire dispatch and both endpoint hooks before replacing a borrowed menu. + for connection in self._popupMenuConnections: + QtCore.QObject.disconnect(connection) + + self._popupMenuConnections.clear() self._popupMenu = menu + if menu is not None: + connectWeakly( + menu.destroyed, + self, + '_clearPopupMenu', + sender=menu, + connectionHandles=self._popupMenuConnections, + ) + + def _clearPopupMenu(self, *_args): + """Release the borrowed menu and its lifecycle registrations.""" + self.setPopupMenu(None) + @QtCore.Slot() def showPopupMenu(self): """Open the configured menu immediately below the button.""" diff --git a/Furious/Qt/Signals.py b/Furious/Qt/Signals.py index fbd46e3..9d3b77b 100644 --- a/Furious/Qt/Signals.py +++ b/Furious/Qt/Signals.py @@ -123,8 +123,14 @@ def connectWeakly( sender=None, forwardSender: bool = False, connectionType=None, + connectionHandles=None, ): - """Connect without strongly owning a transient receiver or sender.""" + """Connect weakly, optionally collecting handles for explicit retirement.""" + if connectionHandles is not None and ( + not isinstance(connectionHandles, list) or connectionHandles + ): + raise ValueError('connectionHandles must be an empty list') + # A plain dispatcher is intentional. Nuitka's PySide6 compatibility layer # process-globally protects compiled bound methods passed directly to connect(). invoke = _weakMethodInvoker( @@ -140,13 +146,16 @@ def connectWeakly( else signal.connect(invoke, connectionType) ) + if connectionHandles is not None: + connectionHandles.append(connection) + if ( isinstance(receiver, QtCore.QObject) and isinstance(sender, QtCore.QObject) and not _ownsQObject(receiver, sender) ): - connections = [connection] + connections = [connection] if connectionHandles is None else connectionHandles def disconnect(*_args): """Release dispatch and both cleanup hooks when either endpoint dies.""" diff --git a/Furious/Qt/ThemeTransition.py b/Furious/Qt/ThemeTransition.py index f5f5bda..cf8d19d 100644 --- a/Furious/Qt/ThemeTransition.py +++ b/Furious/Qt/ThemeTransition.py @@ -28,9 +28,17 @@ from shiboken6 import isValid from collections.abc import Callable, Iterable +import functools + __all__ = ['ThemeTransition'] +def _clearTransitionState(animations, animationsByWindow, *_args): + """Release plain bookkeeping even when the coordinator wrapper is retained.""" + animations.clear() + animationsByWindow.clear() + + class _ThemeSnapshotOverlay(QWidget): """Paint one old-theme window snapshot without intercepting input.""" @@ -97,6 +105,12 @@ class ThemeTransition(QtCore.QObject): self._animations = {} self._animationsByWindow = {} + self.destroyed.connect( + functools.partial( + _clearTransitionState, self._animations, self._animationsByWindow + ) + ) + def isRunning(self) -> bool: """Return whether any window snapshot is currently fading.""" return bool(self._animations) @@ -163,12 +177,21 @@ class ThemeTransition(QtCore.QObject): if animate and self._styleAllowsAnimations(): captures = self._captureWindows() + if not isValid(self): + return + # Capturing first preserves the user's current composite appearance when a # rapid second switch interrupts an in-progress transition. self.stop() + if not isValid(self): + return + applyTheme() + if not isValid(self): + return + for window, snapshot in captures: if not self._canCapture(window): continue @@ -218,8 +241,12 @@ class ThemeTransition(QtCore.QObject): self.transitionStarted.emit() + if not isValid(self): + return + for animation in tuple(self._animations): - animation.start() + if isValid(animation): + animation.start() def _releaseDestroyedOverlays(self): """Native target destruction stops animations without emitting finished.""" @@ -271,7 +298,9 @@ class ThemeTransition(QtCore.QObject): ): self._releaseAnimation(self._animationsByWindow.get(watched)) - return super().eventFilter(watched, event) + # Completion listeners can destroy this filter or the watched window. + # Consume the event only if continuing native delivery would be unsafe. + return not isValid(watched) def stop(self): """Stop and dispose every active transition without leaving an overlay.""" diff --git a/tests/test_qt_lifetime.py b/tests/test_qt_lifetime.py index d1dcbd8..7e3e8d7 100644 --- a/tests/test_qt_lifetime.py +++ b/tests/test_qt_lifetime.py @@ -55,6 +55,7 @@ from Furious.Qt import ( AppQDialog, AppQMainWindow, AppQMenu, + AppQMenuPushButton, AppQMessageBox, AppQSwitch, AppQSeparator, @@ -159,6 +160,136 @@ class DelayedReceiver(QtCore.QObject): class QtLifetimeTest(unittest.TestCase): """Stress direct destruction evidence without relying on process RSS alone.""" + def testActionCallbackCanDestroyItsOwnerBeforeTheVirtualHook(self): + """Native deletion during a callback cancels the remaining activation hook.""" + application() + + class NativeAction(AppQAction): + def triggeredCallback(self, checked): + calls.append('hook') + self.setChecked(checked) + + for destroyOwner in (False, True): + for _ in range(30): + calls = [] + owner = QtCore.QObject() + action = NativeAction('Reentrant', parent=owner) + reference = weakref.ref(action) + + def activate(): + calls.append('callback') + if destroyOwner: + deleteQObject(owner) + + action.callback = activate + + with mock.patch('sys.excepthook') as exceptionHook: + action.trigger() + processQtEvents() + exceptionHook.assert_not_called() + + self.assertEqual( + calls, ['callback'] if destroyOwner else ['callback', 'hook'] + ) + + if isValid(owner): + deleteQObject(owner) + + self.assertFalse(isValid(action)) + del action + self.assertIsNone(reference()) + + def testBorrowedMenuDestructionClearsActionAndButtonReferences(self): + """Destroying a menu's owner must leave independent borrowers usable.""" + application() + + for _ in range(30): + owner = QWidget() + menu = AppQMenu(parent=owner) + reference = weakref.ref(menu) + action = AppQAction('Borrower', menu=menu) + button = AppQMenuPushButton('Popup', popupMenu=menu) + + deleteQObject(owner) + + self.assertIsNone(action._menu) + self.assertIsNone(button.popupMenu()) + self.assertFalse(button._popupMenuConnections) + self.assertFalse(isValid(menu)) + + del menu + self.assertIsNone(reference()) + + child = AppQAction('Child', parent=action) + action.addAction(child) + action.removeAction(child) + button.showPopupMenu() + deleteQObject(action) + deleteQObject(button) + + def testPopupMenuReplacementRetiresBothEndpointHooks(self): + """Repeated replacement keeps live menus and borrower tracking bounded.""" + application() + owner = QWidget() + menus = [AppQMenu(parent=owner), AppQMenu(parent=owner)] + button = AppQMenuPushButton('Popup') + destroyedSignal = QtCore.SIGNAL('destroyed(QObject*)') + baselines = [menu.receivers(destroyedSignal) for menu in menus] + buttonBaseline = button.receivers(destroyedSignal) + + for index in range(100): + active = index % 2 + button.setPopupMenu(menus[active]) + + self.assertEqual(len(button._popupMenuConnections), 3) + self.assertEqual(button.receivers(destroyedSignal), buttonBaseline + 1) + + for menuIndex, menu in enumerate(menus): + self.assertEqual( + menu.receivers(destroyedSignal), + baselines[menuIndex] + (2 if menuIndex == active else 0), + ) + + deleteQObject(menus[0]) + self.assertIs(button.popupMenu(), menus[1]) + + with self.assertRaises(ValueError): + button.setPopupMenu(menus[0]) + + self.assertIs(button.popupMenu(), menus[1]) + deleteQObject(button) + self.assertTrue(isValid(menus[1])) + self.assertEqual(menus[1].receivers(destroyedSignal), baselines[1]) + deleteQObject(owner) + + def testDestroyedPopupButtonReleasesItsMenuBorrow(self): + """A retained dead borrower cannot own an otherwise unreferenced menu.""" + application() + + for keepExternalOwner in (False, True): + for _ in range(30): + menu = AppQMenu() + reference = weakref.ref(menu) + destroyed = [] + menu.destroyed.connect(lambda *_args: destroyed.append(True)) + button = AppQMenuPushButton('Popup', popupMenu=menu) + + if not keepExternalOwner: + del menu + + deleteQObject(button) + + self.assertIsNone(button.popupMenu()) + + if keepExternalOwner: + self.assertTrue(isValid(menu)) + self.assertFalse(destroyed) + deleteQObject(menu) + del menu + + self.assertEqual(destroyed, [True]) + self.assertIsNone(reference()) + def testSingletonSocketsReleaseTheirCallbackAndNativeOwner(self): """Repeated IPC registration must release completed socket wrappers.""" result = runSingletonIPCProbe(30) diff --git a/tests/test_theme_transition.py b/tests/test_theme_transition.py index f496884..8ec3cbc 100644 --- a/tests/test_theme_transition.py +++ b/tests/test_theme_transition.py @@ -19,22 +19,134 @@ from __future__ import annotations -import unittest +from Furious.Qt import ThemeTransition from PySide6 import QtCore from PySide6.QtTest import QSignalSpy from PySide6.QtWidgets import QWidget -from shiboken6 import isValid +from shiboken6 import isValid, delete as deleteQObject -from Furious.Qt import ThemeTransition +from tests.support import ( + application, + assertChildSucceeded, + processQtEvents, + runPythonChild, + waitFor, +) -from tests.support import application, processQtEvents, waitFor +from unittest import mock + +import unittest +import weakref class ThemeTransitionTest(unittest.TestCase): """Exercise cross-fades through the real Qt event loop.""" + def testCompletionCanDestroyTheWatchedWindowDuringResizeDelivery(self): + """Consume the active event when completion deletes its native target.""" + # Unsafe native event delivery can crash rather than raise a Python error. + # Keep that failure inside an exact, bounded offscreen child process. + result = runPythonChild( + ''' +from tests.support import application, processQtEvents +from Furious.Qt import ThemeTransition +from PySide6.QtTest import QSignalSpy +from PySide6.QtWidgets import QWidget +from shiboken6 import isValid, delete as deleteQObject +from unittest import mock +import weakref + +application() + +for _ in range(30): + window = QWidget() + window.resize(160, 100) + window.show() + processQtEvents() + transition = ThemeTransition( + duration=100000, + windowProvider=lambda: (window,), + animationsEnabled=lambda: True, + ) + transition.apply(lambda: None) + animation = next(iter(transition._animations)) + overlay = transition._animations[animation][1] + references = [weakref.ref(item) for item in (window, animation, overlay)] + destroyed = QSignalSpy(window.destroyed) + finished = QSignalSpy(transition.transitionFinished) + transition.transitionFinished.connect(lambda: deleteQObject(window)) + + with mock.patch('sys.excepthook') as exceptionHook: + window.resize(170, 110) + processQtEvents() + exceptionHook.assert_not_called() + + assert destroyed.count() == 1 + assert finished.count() == 1 + assert not isValid(window) + assert not isValid(animation) + assert not isValid(overlay) + assert isValid(transition) + assert not transition._animations + assert not transition._animationsByWindow + + del destroyed, finished, window, animation, overlay + assert all(reference() is None for reference in references) + deleteQObject(transition) + del transition + processQtEvents() +''', + timeout=30, + ) + assertChildSucceeded(self, result, 'window deletion during resize delivery') + + def testReentrantCoordinatorDestructionClearsStateAndCancelsContinuation(self): + """Callbacks may delete the coordinator before animation acquisition/start.""" + application() + + for boundary in ('theme', 'started', 'finished'): + for _ in range(20): + window = QWidget() + window.resize(160, 100) + window.show() + processQtEvents() + transition = ThemeTransition( + duration=100000, + windowProvider=lambda: (window,), + animationsEnabled=lambda: True, + ) + reference = weakref.ref(transition) + + with mock.patch('sys.excepthook') as exceptionHook: + if boundary == 'theme': + transition.apply(lambda: deleteQObject(transition)) + elif boundary == 'started': + transition.transitionStarted.connect( + lambda: deleteQObject(transition) + ) + transition.apply(lambda: None) + else: + transition.apply(lambda: None) + transition.transitionFinished.connect( + lambda: deleteQObject(transition) + ) + window.resize(170, 110) + + processQtEvents() + exceptionHook.assert_not_called() + + self.assertFalse(isValid(transition)) + self.assertFalse(transition._animations) + self.assertFalse(transition._animationsByWindow) + self.assertFalse(self.overlays(window)) + + del transition + self.assertIsNone(reference()) + deleteQObject(window) + processQtEvents() + def setUp(self): """Create per-test windows while retaining one process-wide application.""" application()