mirror of
https://github.com/LorenEteval/Furious.git
synced 2026-10-07 14:28:15 +03:00
Fix reentrant Qt lifetimes
Signed-off-by: Loren Eteval <loren.eteval@proton.me>
This commit is contained in:
@@ -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;
|
||||
|
||||
+10
-1
@@ -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."""
|
||||
|
||||
@@ -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."""
|
||||
|
||||
+11
-2
@@ -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."""
|
||||
|
||||
@@ -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."""
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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()
|
||||
|
||||
Reference in New Issue
Block a user