Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix memory leaks in pyVaultNodeOperationCallback. #1004

Merged
merged 2 commits into from
Oct 20, 2021

Conversation

Hoikas
Copy link
Member

@Hoikas Hoikas commented Oct 9, 2021

A long standing issue has been mysterious "key only" memory leaks when the plResManager destructs, as seen in the leak report in plasmadbg.log:

(10/09 07:00:12) 	Leaks in page Personal>psnlMYSTII[000d0022]:
(10/09 07:00:12) 		plSceneObject: (S:0xd0022 F:0 I:526 N:ImgrPhotoPlane02 C:[0,0]) (key only, 10 refs left)
(10/09 07:00:12) 		plLogicModifier: (S:0xd0022 F:0 I:134 N:cRgnImagerActivate_Enter C:[0,0]) (key only, 1 refs left)
(10/09 07:00:12) 		plLogicModifier: (S:0xd0022 F:0 I:135 N:cRgnImagerActivate_Exit C:[0,0]) (key only, 1 refs left)
(10/09 07:00:12) 		plResponderModifier: (S:0xd0022 F:0 I:336 N:RespImager2Button C:[0,0]) (key only, 1 refs left)
(10/09 07:00:12) 		plPythonFileMod: (S:0xd0022 F:0 I:86 N:PythonFileImager02 C:[0,0]) (key only, 13 refs left)
(10/09 07:00:12) 		plDynamicTextMap: (S:0xd0022 F:0 I:1 N:ImagerDynamic02_dynText C:[0,0]) (key only, 1 refs left)

These leaks correspond to the xSimpleImager.py script, which uses vaultOperation callbacks. The vault operation callback C++ code was incorrectly assuming that PyObject_GetAttrString() would return a borrowed reference and not decrementing the reference count. This is fixed by using pyObjectRef to correct and clarify the reference counting.

Unfortunately, Python still reports errors on shutdown, but this is a good step toward correcting them.

`PyObject_GetAttrString` returns a new reference, which was never
released by the previous code. This manifested as mysterious "key only"
leaks in the plKey leak report when the plResManager was destructing. As
a bonus, I have sprinkled in usage of `pyObjectRef` to clarify the
reference semantics.
@Hoikas
Copy link
Member Author

Hoikas commented Oct 17, 2021

I've force-pushed an update that makes the pyVaultNodeOperationCallback constructors explicit to remove potential ambiguity.

@Hoikas
Copy link
Member Author

Hoikas commented Oct 17, 2021

I need to change pyObjectNewRef to safely accept nullptr, don't merge this yet. Done

Sometimes, it is legal to want to acquire a reference to something that
might be null.
@Hoikas Hoikas requested a review from zrax October 19, 2021 22:47
@Hoikas Hoikas merged commit 414bd35 into H-uru:master Oct 20, 2021
@Hoikas Hoikas deleted the python_leaks branch October 20, 2021 00:05
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.

None yet

2 participants