-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Make sure we clean up old QItemSelectionModels #7950
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
22ee4f2
to
b77cbbb
Compare
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks much simpler. Is there any risk to holding a reference to a deleted selection model, e.g. if something tried to call a method on it? Are we just relying on the fact that if _active
is False
nothing will be called?
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. |
I had a little poke around (just modifying the existing test to do stuff before, during and after delete). TL;DR: is that I don't think this PR changes the chances of a crash due to a completion command being run with no completion model. So I would like to treat that question as out of scope of this PR :) In general, and in this case, if you try to call a method on a deleted PyQt object (eg here if we call |
This starts happening reliably with the "Using session completion" end to end test after the previous change where I made it so we don't set the completion widget model to none, 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 manually go through the scenario a few times. 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.
5677ebe
to
00daeff
Compare
Got it -- I think part of my confusion was I assumed |
@@ -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() | |||
|
|||
self.setModel(model) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just clarifying ... if we call oldModel.deleteLater; self.setModel(None)
, you are saying the deleteLater
never takes effect?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The model does still get deleted in that case but I want to avoid the self.setModel(None)
call because of the whole leaking selection model thing.
Don't call
QTreeView.setModel(None)
as it cases a leak ofQItemSelectionModel
s. 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
destroyed
signal seems to be an alternate way of resetting the state of theQTreeView
and deleting the selection models and it does have the problem of creating newQItemSelectionModel
s 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