GNOME Bugzilla – Bug 608848
[ShellGenericContainer] Simplify tracking of skip-paint children
Last modified: 2010-02-03 17:05:21 UTC
The current way we keep track of skip-paint children is more difficult than it needs to be, and can lead to subtle bugs. (For one, the skip-paint state of a child is remembered when it is removed then added back again, which is completely unexpected.) Instead of using weak references to track children, just remove items from the skip-paint list by overriding the remove() virtual function of the ClutterContainer interface. The 'skip_paint' hash table is then destroyed in finalize rather than dispose since it doesn't hold references to memory any more but just passively tracks an attribute of the children that are currently in the container.
Created attachment 152894 [details] [review] [ShellGenericContainer] Simplify tracking of skip-paint children
Comment on attachment 152894 [details] [review] [ShellGenericContainer] Simplify tracking of skip-paint children >+shell_generic_container_real_remove (ClutterContainer *container, "real"? I thought that was generally only used for default implementations (where type_name_method_name() is already in use as the virtual function name, so you have to use type_name_real_method_name() for the impl). >- area->priv->skip_paint = g_hash_table_new_full (NULL, NULL, (GDestroyNotify)g_object_unref, NULL); >+ area->priv->skip_paint = g_hash_table_new (NULL, NULL); Uh? I don't see any other refcounting changes in your patch, so either we're leaking now, or else the code used to unref objects it didn't have a reference on? But I can't see where we'd be getting a ref on the hash table contents, so yeah, it looks like this is correct and we should have been crashing before?
(In reply to comment #2) > (From update of attachment 152894 [details] [review]) > >+shell_generic_container_real_remove (ClutterContainer *container, > > "real"? I thought that was generally only used for default implementations > (where type_name_method_name() is already in use as the virtual function name, > so you have to use type_name_real_method_name() for the impl). Yep, I accidentally copied the real when referencing the ClutterContainer implementation of ClutterGroup (the Clutter sources tend to be a bit fuzzy on the use of 'real'.) Will fix. > >- area->priv->skip_paint = g_hash_table_new_full (NULL, NULL, (GDestroyNotify)g_object_unref, NULL); > >+ area->priv->skip_paint = g_hash_table_new (NULL, NULL); > > Uh? I don't see any other refcounting changes in your patch, so either we're > leaking now, or else the code used to unref objects it didn't have a reference > on? But I can't see where we'd be getting a ref on the hash table contents, so > yeah, it looks like this is correct and we should have been crashing before? That's my interpretation... since we never a) freed a container with skip paint used for its children b) unset the skip paint flag, we were getting lucky and not crashing.
Attachment 152894 [details] pushed as ed36517 - [ShellGenericContainer] Simplify tracking of skip-paint children