GNOME Bugzilla – Bug 685598
Callback closures and user_data with "call" scope type are leaked
Last modified: 2013-02-03 21:40:27 UTC
Created attachment 225920 [details] Unittest showing callback closure leak with Gtk.Builder.connect_signals Gtk.Builder.connect_signals increments the reference counts of the closure and user_data but does not decrement them. Proper reference counting these objects should be handled as the method is annotated with (scope call) or GI_SCOPE_TYPE_CALL. However, I am not observing the cleanup function being called after the connect_signals method finishes. A break point within gi/pygi-closure.c:_pygi_invoke_closure_free is never be reached. Reading the code paths show _pygi_invoke_closure_free is not called for GI_SCOPE_TYPE_CALL as far as I can tell. However, it is hooked up for async closures with a tracking list but this only seems to be cleared upon the creation of a new closure. Making async closures appear somewhat leaky as well. Perhaps a cleaner strategy for this would be to free the python refs and clear them out of the closure structure within _pygi_closure_handle itself, but leave the freeing of closure instance for another time (which also needs to be fixed in the case of call scope). This would at least make the code read clearly as to when the python objects are decrefed. ./gi/pygi-marshal-from-py.c:_pygi_marshal_from_py_interface_callback shows _pygi_invoke_closure_free being hooked up for callback closures of type GI_SCOPE_TYPE_NOTIFIED. But it is fairly hard to understand because the caching scheme obfuscates things. I still have a lot more reading to do to understand what is going on here. There are also a few functions in these parts of the code which are never used and should be deleted as they only add confusion for readers: ./gi/pygi-callbacks.c:_pygi_create_callback ./gi/pygi-callbacks.c:_pygi_scan_for_callbacks
Created attachment 225925 [details] Updated to show leak right after connect_signals is called
Created attachment 226164 [details] [review] Fix leak with python callables as closure argument The fix adds an extra args_data list to the PyGIInvokeState structure. This list is used to track dynamically generated closures that wrap python callables. This allows the ffi closure and python callable to be freed when call scope has finished. Made additional adjustments to (call notified) based closure args to print a warning when user data is not available and a dummy GDestroyNotify is used in this case to avoid a potential crash later on.
Created attachment 226221 [details] [review] Updated patch with more robust tests This now depends on an addition to gobject-introspection regress test (https://bugzilla.gnome.org/show_bug.cgi?id=685922). Updated patch for testing the warning/error handling in cases where scope notified is used without a user data field.
Created attachment 226223 [details] [review] Fix reference leaks with scope async This patch adds additional cleanup for scope async closures. It works by only clearing the python callable and user_data out of the PyGICClosure structure but not deleting the structure itself at the end of the callbacks invocation. Also cleaned up reference counting problems during error conditions in _pygi_marshal_from_py_interface_callback where user_data had some un-needed DECREFs because PyTuple_GetItem returns a borrowed reference. Furthermore when py_user_data is NULL, leave the interpretation of this to Py_None until _pygi_make_native_closure.
Just a quick review, I noticed two nitpicks: + Scop async closures free only their python data now and the closure later Scop"e" + during the next creation of a closure. This minimized potential ref leaks minimize"s"? + with warnings.catch_warnings(record=True) as w: Did you check that this also works with Python 2.6 and 2.7? Once your patch in bug 685922 lands, feel free to commit with above considerations. Thanks!
Created attachment 226307 [details] [review] Updated patch with comment fixes > + with warnings.catch_warnings(record=True) as w: > Did you check that this also works with Python 2.6 and 2.7? Yes, verified this works in 2.6, 2.7 and 3.3. Although the tests are broken elsewhere with python 2.6 and 2.7 > Once your patch in bug 685922 lands, feel free to commit with above > considerations. Thanks! Depending on how long that takes, I might want to pull the dependent test out of this patch and put it in another ticket or add it as an additional patch on top of this one. There are other leaks I'm finding in this area of the code and would want this applied before working on them.
(In reply to comment #6) > Yes, verified this works in 2.6, 2.7 and 3.3. Great, thanks. > Although the tests are broken elsewhere with python 2.6 and 2.7 Err, indeed. I always test with 2.7 before a release, but now I get three failures in test_gi.TestSize, test_gi.TestUInt64, test_gi.TestULong. I'll look into that today. > Depending on how long that takes I just ack'ed it, so please push. > I might want to pull the dependent test out > of this patch and put it in another ticket or add it as an additional patch on > top of this one. There are other leaks I'm finding in this area of the code and > would want this applied before working on them. Should be fine, please go ahead. Thanks!
*** Bug 692044 has been marked as a duplicate of this bug. ***