Make sure we clean up old QItemSelectionModels #7950
Conversation
This file contains 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
rcorre
reviewed
Oct 7, 2023
There was a problem hiding this comment.
Thank you for the excellent explanation! As it appears to be a small leak and may be a bug in QT, I'd lean towards making a QT bug report or contacting QT devs before we attempt a hacky fix of our own.
That being said, it's been years since I looked at this code :)
To avoid a leak when calling `QTreeView.setModel(None)`, this commit switches to relying on the `model.destroyed` signal to make sure related state is cleaned up. Upstream bug: https://bugreports.qt.io/browse/QTBUG-49966 When you call `setModel(None)` on a QTreeView it causes a small memory leak of `QItemSelectionModel` objects created by the QTreeView's child QHeaderView object. `QAbstractItemView` will create a new `QItemSelectionModel` whenever a model is set, even if that model is `None`. When the new model is non-null the new selection model will be set to be deleted when the new model is, but when the new model is None the new selection model will be linked to the static `QAbstractItemModelPrivate::staticEmptyModel()`. Since that empty model lives forever, so do the related section models, unless callers take care to clean them up themselves. Both `QTreeView` and it's child `QHeaderView` implement `QAbstractItemView` and have this behaviour. For the `QTreeView` we were making sure to delete the old selection model ourselves (as of fe1215c). But for the one created by the `QHeaderView` we can't get a reference it because `QTreeView.setModel()` would call `QHeaderView.setModel()` and then `QHeaderView.setSelectionModel()` right away to assign it's own selection model to the child, leaving no references to the selection model created by `QHeaderView.setModel()`. I was previously using `header.findChildren(QItemSelectionModel)` to clean up old orphaned selection models, but this approach is a lot simpler! To observe this for yourself you can plonk something like this in `set_model()` before the early return and switch between the old and new implementation and see how it changes behaviour. header = self.header() header_children = header.findChildren(QItemSelectionModel) our_children = self.findChildren(QItemSelectionModel) print(f"{len(our_children)=} {len(header_children)=}") You can also observer the selection models accumulating in Gammaray (https://github.com/KDAB/GammaRay/) if you just open and close the selection a lot and then filter the object view by "model". The relevant code is in `QTreeView` and `QAbstractItemView`'s `setModel()`, `setSlectionModel()` and `modelDestroyed()`. Bot mostly in the `setModels()` where you can see the relevant signals being connected and disconnected. https://code.qt.io/cgit/qt/qtbase.git/tree/src/widgets/itemviews/qtreeview.cpp#n179 Fixes: #7947
toofar
force-pushed
the
fix/7947_clean_up_selection_models
branch
from
22ee4f2
to
b77cbbb
Compare
toofar
commented
Jun 3, 2024
| @@ -364,15 +364,13 @@ def set_model(self, model): | |||
| old_model = self.model() | |||
| if old_model is not None and model is not old_model: | |||
| old_model.deleteLater() | |||
| self._selection_model().deleteLater() | |||
There was a problem hiding this comment.
The selection model should be cleared up when the model is: https://code.qt.io/cgit/qt/qtbase.git/tree/src/widgets/itemviews/qabstractitemview.cpp?h=6.7.1#n760
This starts happening reliable with the "Using session completion" after
the previous change where I made it so we don't set the completion
widget model to non, just call `deleteLater()` on the old one. The
relevant part of the test is:
And I run :session-save hello
# This loads a completion model with one entry
When I run :cmd-set-text -s :session-load
# This one selects the entry, which clears the model because the
# session completion only applies to the first word after the
# command, which we've just selected.
And I run :completion-item-focus next
# This does nothing, since we have no completion model loaded,
# not sure why it is in the test, presumably making sure no
# exception is thrown.
And I run :completion-item-focus next
The log message is reliable in the test but it's a bit flaky to
reproduce manually. I've reproduced it via `for f in 1 2;do qute-send -i
/tmp/qutebrowser-basedir-npzdpupy/runtime/ipc-8075cf54f63b8e1bc05db34f41292c38
":completion-item-focus next" ;done` if I manualy go through the
scenario a few time. Possibly it would reproduce easier if I pin it to
one process using `taskset`.
I think the reason for this is that the model is marked for deletion in
the next event loop, then a signal goes out, then the selection model is
marked for deletion on the next event loop. And possibly this only
happens between the model and the selection model being deleted?
We want to make sure that the selection model gets deleted when clearing the model, since we are switching from doing that directly to having it happen indirectly based off of signals we don't manage. Hopefully this doesn't end up to be flaky. I think we are depending on this happening in two different Qt even loop runs (`model.deleteLater()` in the first one then `selmod.deleteLater()` gets called from the `model.deleted` signal). I tried using `qtbot.wait_signals()` with a list but it segfaulted in some cleanup method. Perhaps is doesn't handle the deleted signal well? Alternately we could maybe just wait for the selmodel one and check `sip.isdelted(model)` to check the model is gone too.
rcorre
reviewed
Jun 3, 2024
Comment on lines
+76
to
+77
| # model (eg when no completion function is available for the current text | ||
| # pattern. |
There was a problem hiding this comment.
Suggested change
| # model (eg when no completion function is available for the current text | |
| # pattern. | |
| # model (e.g. when no completion function is available for the current text | |
| # pattern). |
Hmm, is there any specific places you are worried about. I guess I didn't think about it much. I'll try to test this out and see what the behaviour is. |
|
I wasn't thinking of any area in particular. It was just a general sense that holding onto an reference after freeing it is how you get use-after-free bugs. I'm not sure if that's actually a problem in PyQt though. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Don't call
QTreeView.setModel(None)as it cases a leak ofQItemSelectionModels. Normally the selection models get cleaned up when the model they are linked to is destroyed. But when you set a null model the selection models get linked to a static empty model that lives forever.Just deleting the model and letting the connections to its
destroyedsignal seems to be an alternate way of resetting the state of theQTreeViewand deleting the selection models and it does have the problem of creating newQItemSelectionModels with a long lifetime.I spotted this leak with KDAB's Gammary. Maybe we'll spot more that way later (it gets really slow with many objects though).
I'm not sure how to add a test to ensure these are getting cleaned up since it's being done async. I think the fact that all the existing tests are still passing is a pretty strong signal!
Previously I had a comment in about "do we even get called twice with the same model"? I don't think we do in the normal flow of things because we always close and re-open the completion. But who knows what kind of shenanigins you can get up to with IPC.
Fixes: #7947