[Spice-devel] [PATCH spice-gtk 1/3] MainChannel: move task free from finalize to dispose

[Fabiano Fidêncio]

On Thu, Oct 22, 2015 at 10:16 AM, Pavel Grunt <pgrunt at redhat.com> wrote:
> On Thu, 2015-10-22 at 10:06 +0200, Fabiano Fidêncio wrote:
>> On Thu, Oct 22, 2015 at 9:13 AM, Victor Toso <lists at victortoso.com> wrote:
>> > Hi,
>> >
>> > On Wed, Oct 21, 2015 at 03:52:49PM -0500, Jonathon Jongsma wrote:
>> > > In order to avoid reference cycles, you're supposed to release
>> > > references in dispose, especially to those objects that can hold
>> > > references to yourself. This probably wasn't causing any leaks, since
>> > > the file transfer tasks generally are not alive when the main channel is
>> > > destroyed, but it's more proper.
>> > > ---
>> > >  src/channel-main.c | 7 +++++--
>> > >  1 file changed, 5 insertions(+), 2 deletions(-)
>> > >
>> > > diff --git a/src/channel-main.c b/src/channel-main.c
>> > > index 3a8c1dd..0a73e92 100644
>> > > --- a/src/channel-main.c
>> > > +++ b/src/channel-main.c
>> > > @@ -405,6 +405,11 @@ static void spice_main_channel_dispose(GObject *obj)
>> > >          c->migrate_delayed_id = 0;
>> > >      }
>> > >
>> > > +    if (c->file_xfer_tasks) {
>> > > +        g_hash_table_unref(c->file_xfer_tasks);
>> > > +        c->file_xfer_tasks = NULL;
>> > > +    }
>> > > +
>> >
>> > g_clear_object(&c->file_xfer_tasks);
>>
>> No, please, it's wrong.
>> For GHashTable you must use g_clear_pointer() instead of
>> g_clear_object() and g_clear_pointer() is part of GLib since 2.34,
>> while we depend on 2.28.
>>
> We are using g_clear_pointer() on a few places, for older glib it is defined in
> the glib-compat.h

That's true. We do use g_clear_pointer() in other places and would be
really nice if we can have it all over the place then.

For this patch, in particular, I don't care if it's using
g_clear_pointer() or not, but would be nice to have a new patch using
g_clear_pointer()/g_clear_object() wherever we can do this.

Best Regards,
--
Fabiano Fidêncio