Merged
Conversation
the existing implementation was to workaround the previous (reverted) QMenu leak implementation. the new QMenu leak workaround implementation does not need it.
Contributor
Author
|
The following script better explains the leak and the workarounds. from PySide6 import QtWidgets
import shiboken6 as shiboken
import sys
def dump(obj):
d = dict(
refcnt=sys.getrefcount(obj),
creator='Python' if shiboken.createdByPython(obj) else 'C++',
owner='Python' if shiboken.ownedByPython(obj) else 'C++',
valid=shiboken.isValid(obj)
)
print(d)
app = QtWidgets.QApplication([])
def case1():
# no problem when a menu creates its own submenu
topmenu = QtWidgets.QMenu()
submenu = topmenu.addMenu("submenu")
dump(submenu)
del topmenu
dump(submenu)
# topmenu was the C++ parent
assert not shiboken.isValid(submenu)
def case2():
# PySide{2,6} has a leak here
topmenu = QtWidgets.QMenu()
submenu = QtWidgets.QMenu()
dump(submenu)
# C++ QMenu::addMenu does not take ownership
topmenu.addMenu(submenu)
dump(submenu)
del topmenu
dump(submenu)
def workaround1():
# let C++ handle the life-cycle
topmenu = QtWidgets.QMenu()
submenu = QtWidgets.QMenu(topmenu) # specify topmenu as C++ parent
dump(submenu)
topmenu.addMenu(submenu)
dump(submenu)
del topmenu
dump(submenu)
# topmenu was the C++ parent
assert not shiboken.isValid(submenu)
def workaround2():
# let Python wrapper handle the life-cycle
topmenu = QtWidgets.QMenu()
submenu = QtWidgets.QMenu()
dump(submenu)
topmenu.addMenu(submenu)
submenu.setParent(None) # workaround
dump(submenu)
del topmenu
dump(submenu)
# topmenu was not the C++ parent
assert shiboken.isValid(submenu)
case1()
print()
case2()
print()
workaround1()
print()
workaround2()
print()Output |
Member
|
@pijyoi that post is brilliant; thanks for making it. the PR LGTM, and issues like these keep making me think that there may be some interest in developing a python qt-bindings code scanner within codeql Anyway, merging! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Partially fixes #2497
There are 3 places that leak
QMenus. This PR fixes 2 of them. The last one is left unfixed as fixing it causes a CI segfault on PySide6 >= 6.3 on Linux platforms. It is believed that the last leak is masking a latent issue.The workaround used has changed from #2502. In #2502, "topmenu" was made the
C++ parentof "submenu".It was found that PySide{2,6} binding makes "topmenu" a
Python-wrapper parentof "submenu", increasing the refcnt of "submenu", even though "submenu" has noC++ parent.The workaround here relies on PySide's
setParent(None)dropping this refcnt. (Note that theC++ parentwas alreadyNoneto begin with)The end-result is that the life-state of "submenu" remains the same before and after
addMenu, as shown in the output of the following script: refcnt and shiboken.dump output remains the same.If upstream fixes the leak, then this
setParent(None)call should have no effect.Output
As mentioned, this PR only fixes 2 of the leaks. When running the test scripts in #2497, the rate of memory growth is reduced and only 1 QMenu is leaked.