Bug 71498

Summary: PackageKit is leaking ~ 10 MiB for each created thread
Product: PackageKit Reporter: Thomas Perl <m>
Component: coreAssignee: Richard Hughes <richard>
Status: RESOLVED FIXED QA Contact:
Severity: normal    
Priority: medium    
Version: unspecified   
Hardware: All   
OS: All   
Whiteboard:
i915 platform: i915 features:
Attachments: Proposed patch -- Call g_thread_unref() on the return value

Description Thomas Perl 2013-11-11 15:09:15 UTC 
In src/pk-backend-job.c, the call to g_thread_new() returns a GThread * object that has a reference count of 1 that needs to be unreferenced, otherwise the GThread * object will never be freed. This leaks ~ 10 MiB per created thread in packagekitd while packagekit is running.

This can be fixed by doing a g_thread_unref() on the returned value - as long as the thread is running, it will keep its own reference, so calling g_thread_unref() right after creating the thread is safe.

This problem wasn't happening for the code path for GLib < 2.31, as there, the "joinable" parameter to g_thread_create() was FALSE, which means it didn't return a GThread * object. On the other hand, it also didn't return 

Docs:
 - https://developer.gnome.org/glib/2.37/glib-Threads.html#g-thread-new
 - https://developer.gnome.org/glib/2.37/glib-Deprecated-Thread-APIs.html#g-thread-create

Related to this, for the GLib < 2.31 case, the returned value didn't have a reference, so the memory leak didn't happen there. But as we were keeping the pointer to the return value anyway, this was probably a bad idea (from the docs: "If joinable is FALSE then you should probably not touch the return value."). We didn't use the return value, so instead of managing it and its reference, we just keep the GThread * local, have a "has_thread" boolean on the job's private data to make sure it only has one thread and set joinable to TRUE for g_thread_create() as well, and then just g_thread_unref() the thread in all cases.

I'll attach a proposed patch.
Comment 1 Thomas Perl 2013-11-11 15:12:21 UTC 
Created attachment 89034 [details] [review]
Proposed patch -- Call g_thread_unref() on the return value

Proposed patch - also changed the "GThread *" to just a gboolean to check if we have created a thread at some point, as the object referenced by the "GThread *" will in general not be accessible after g_thread_unref() is called (it will be removed as soon as the thread exits).
Comment 2 Richard Hughes 2013-11-11 16:37:32 UTC 
I've fixed this in a slightly different way:

commit 6e582a38ce6da13fda74cfabb41865eabd57dbf0
Author: Richard Hughes <richard@hughsie.com>
Date:   Mon Nov 11 16:36:37 2013 +0000

    Fix memory leak when using new versions of GLib
    
    Unref the thread as the thread has a reference to itself.
    
    Resolves: https://bugs.freedesktop.org/show_bug.cgi?id=71498

Thanks for hunting this one down!

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.