[Linux] do not destroy unowned GdkKeymap instance #34351
Conversation
`gdk_keymap_get_for_display()` does not transfer ownership to the caller. Destroying the keymap object would cause problems further down the road when `GtkWindow` gets to clean up and tries to destroy the same keymap.
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
dkwingsmt
left a comment
dkwingsmt
left a comment
There was a problem hiding this comment.
Makes sense, thanks for finding this!
There was a problem hiding this comment.
The keymap isn't actually used outside of fl_view_constructed so in the current code there's no need to have a reference at all. However, the keymap_keys_changed_cb is never disconnected, thus this could be called after the FlView is destroyed as long as the keymap exists.
What the solution probably should be is to hold a refrence to the keymap, keep the clear object and remove the signal handler in fl_view_remove and fl_view_dispose (currently nothing is done in dispose).
|
Sorry, I forgot to disconnect the callback. So the current change should be OK, but we need to disconnect the callback on remove or dispose, is it right? |
robert-ancell
left a comment
robert-ancell
left a comment
There was a problem hiding this comment.
Actually now I re-read it keymap is used inside lookup_key, so the change makes sense as it currently is.
|
@dkwingsmt yes, please propose a PR that disconnects the callback in those two cases. |
3 participants
gdk_keymap_get_for_display()does not transfer ownership to the caller. Destroying the keymap object would cause problems down the road whenGtkWindowgets to clean up and tries to disconnect from the same keymap thatFlViewalready destroyed.Since #32665, when closing any Flutter application that uses
HdyWindowinstead of a plainGtkWindowthat has a slightly different object tree structure:Where the callstack looks like this:
Taking a reference could be another option, but generally, native GTK widgets do not take a reference to the keymap probably because the keymap is managed per display and thus, supposed to outlive any widget and window instance. Therefore, the proposal is to simply remove the
g_clear_object()call.Pre-launch Checklist
///).