GNOME Bugzilla – Bug 658650
reference cycle in {ContactList,Individual}Store
Last modified: 2018-05-22 15:03:57 UTC
The fix for bug #657086 introduced a reference cycle: - The store keeps hash tables containing GtkTreeRowReference - The GtkTreeRowReference keeps a reference on the store An easy way to experience this cycle: - Join a XMPP muc - Open and close the "Invite participant" dialog - The EmpathyContactListStore is never freed I'm not sure of the right way to fix this. Xavier, Alban: any thoughts? Depending on the way we break this cycle, we may want to remove the fix for bug #658644
Adding Kris in CC since he knows much better Gtk trees. So the problem is we have our GtkTreeStore subclass which contains contacts. Each contact can be in multiple groups so could have multiple rows. When a contact change we need a quick way to lookup all GtkTreeIter where that contact is to update info. We speedup that lookup by having a hashtable of contact->GtkTreeRowReference inside our store but then the store keeps the rowref and the rowref keeps the store => ref cycle => impossible to free the store. Maybe we are doing something totally wrong? Kris: Suggestions?
I guess this is the expected behavior ... A GtkTreeView will release its reference on a GtkTreeModel when it is done with it. In your case, the store will only be finalized once you have first released all of the row references inside store. As far as I know it is not really common to have row references to the same model inside the model. I guess you would typically keep a pointer to the internal data structure instead of a row reference. Or if the iterators are persistent, you could keep the iterator (however, then you do not get the behavior that the iter is automatically reset when the row is removed, like with GtkTreeRowReference).
Iters are not persistent when adding/removing rows above the row we want to ref, right? GtkTreeRowReference does that by updating the path each time rows are added/removed above it. We cannot access GtkTreeStore's internal neither. So I guess what we have left is implementing our own GtkTreeModel, or our own GtkTreeRowReference that does not take ref on the store. Shouldn't be too difficult to keep paths up-to-date when contacts are inserted/removed in the store.
(In reply to comment #3) > Iters are not persistent when adding/removing rows above the row we want to > ref, right? If the model sets the ITERS_PERSIST flag, the iterators should be persistent, also on adds/removes. TreeStore uses GNode and sets user_data to an allocated GNode. For adds/removes above the row to refer to, a new GNode is created and the links are updated. So the GNode pointer stored in user_data for current rows should not be touched and is thus persistent. (In reply to comment #3) > So I guess what we have left is implementing our own GtkTreeModel, or our own > GtkTreeRowReference that does not take ref on the store. Shouldn't be too > difficult to keep paths up-to-date when contacts are inserted/removed in the > store. Note that for the GtkTreeRowReference implementation, we had to employ a custom signal marshaller (if I remember correctly) to keep the path updates consistent. So it is not as trivial as it may seem (at least for the general case).
Ok, so GtkTreeStore actually have persistent iters, so we can just keep GtkTreeIter in our cache and be done. Much easier and even faster since we don't have to keep GtkTreeRowReference that keeps updating internal path on each model modification and convert path->iter for each access. Thanks Kris for your answer :)
Created attachment 196470 [details] [review] individual-store: store GtkTreeIter rather than GtkTreeRowReference in the cache GtkTreeRowReference keeps a ref on the store introducing a ref cycle.
Created attachment 196471 [details] [review] contact-list-store: store GtkTreeIter rather than GtkTreeRowReference in the cache GtkTreeRowReference keeps a ref on the store introducing a ref cycle.
looks good, I reviewed only the one for individual though. Please make sure contact updates are still working fine even after some other contacts got deleted above, to be sure.
Created attachment 196544 [details] [review] Remove unused variables The two patches from Guillaume are good to me. Here is an additional patch to clean unused variables.
Attachment 196470 [details] pushed as 7561e27 - individual-store: store GtkTreeIter rather than GtkTreeRowReference in the cache Attachment 196471 [details] pushed as e5eefe6 - contact-list-store: store GtkTreeIter rather than GtkTreeRowReference in the cache
Created attachment 196979 [details] [review] EmpathyIndividualView-658650.patch I tested with GDB-breakpoints in empathy-chat on empathy_individual_view_init() and individual_view_dispose() and they are called successfully in the "Invite participant" dialog. But I get the following segfault sometimes when closing the invite dialog (after trying about 5 times successively). It seems the view is already disposed but signal callbacks are still called. I tried the attached patch but it still crash. (empathy-chat:3543): GLib-GObject-WARNING **: invalid unclassed pointer in cast to `EmpathyIndividualView' Program received signal SIGSEGV, Segmentation fault. individual_view_filter_visible_func (model=0x986b510, iter=0xbfffe000, user_data=0x86fc898) at empathy-individual-view.c:1788 1788 if (priv->custom_filter != NULL) (gdb) bt
+ Trace 228491
$1 = (EmpathyIndividualViewPriv *) 0xaaaaaaaa (gdb)
*** Bug 659780 has been marked as a duplicate of this bug. ***
*** Bug 660209 has been marked as a duplicate of this bug. ***
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/empathy/issues/429.