Skip to content

Fix QMenu leak in PlotItem and ViewBox#2502

Merged
j9ac9k merged 3 commits intopyqtgraph:masterfrom
pijyoi:fix-qmenu-leak
Oct 26, 2022
Merged

Fix QMenu leak in PlotItem and ViewBox#2502
j9ac9k merged 3 commits intopyqtgraph:masterfrom
pijyoi:fix-qmenu-leak

Conversation

@pijyoi
Copy link
Copy Markdown
Contributor

@pijyoi pijyoi commented Oct 26, 2022

Fixes #2497

QMenus are instantiated and then added to another menu as a sub-menu using QMenu.addMenu(menu : QMenu).
However, this method specifically does not take ownership of the sub-menu. (https://doc.qt.io/qt-6/qmenu.html#addMenu)
Hence the sub-menus remain parent-less.

On PySide{2,6} bindings, these parent-less sub-menus get leaked, as demonstrated by the scripts in #2497.
Setting a parent to them fixes the leak.

However, these changes cause a failure in test_ref_cycles.py::test_PlotWidget() for PyQt{5,6} bindings.
Specifically, with this change, the objects in w.plotItem.getMenu() already have their underlying C++ object destroyed, resulting in the following error.

>           assert ref() is None
E           AssertionError: assert <PyQt6.QtWidgets.QCheckBox object at 0x000001BC7D7EC310> is None
E            +  where <PyQt6.QtWidgets.QCheckBox object at 0x000001BC7D7EC310> = <[RuntimeError('wrapped C/C++ object of type QCheckBox has been deleted') raised in repr()] weakref object at 0x1bc7d804c20>()

A possible workaround (done in this PR) is to treat such occurrences as a pass.

menu.addMenu(submenu) does not take ownership of "submenu"
self.export and self.exportMethods are not assigned to, so calling
either of those methods would have raised an exception.
@j9ac9k
Copy link
Copy Markdown
Member

j9ac9k commented Oct 26, 2022

Thanks for the PR @pijyoi This LGTM, merging.

@j9ac9k j9ac9k merged commit 131ed27 into pyqtgraph:master Oct 26, 2022
@pijyoi pijyoi deleted the fix-qmenu-leak branch October 26, 2022 18:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

QMenus not destroyed (memory leak)

2 participants