GNOME Bugzilla – Bug 695128
GClosure arguments are leaked
Last modified: 2014-07-31 01:06:05 UTC
The GClosure objects that are being created in pyg_closure_new() are not unreffed and thus cleaned up anywhere. This is detected by valgrinding the test_gi.TestGClosure.test_in test: ==24654== 112 (96 direct, 16 indirect) bytes in 1 blocks are definitely lost in loss record 3,015 of 4,720 ==24654== at 0x4C2ABB4: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==24654== by 0xAC73813: standard_calloc (gmem.c:104) ==24654== by 0xAC738A5: g_malloc0 (gmem.c:189) ==24654== by 0xA9D004B: g_closure_new_simple (gclosure.c:206) ==24654== by 0xBD913CF: pyg_closure_new (pygtype.c:1363) ==24654== by 0xA55919A: pygi_marshal_from_py_gclosure (pygi-marshal-from-py.c:1957) ==24654== by 0xA557BF4: _pygi_marshal_from_py_interface_struct (pygi-marshal-from-py.c:1649) ==24654== by 0xA54BD87: _invoke_marshal_in_args (pygi-invoke.c:515) ==24654== by 0xA54C4F1: pygi_callable_info_invoke (pygi-invoke.c:659) ==24654== by 0xA54C6F5: _wrap_g_callable_info_invoke (pygi-invoke.c:685) ==24654== by 0x4814AD: PyEval_EvalFrameEx (ceval.c:4374) ==24654== by 0x47A90D: PyEval_EvalCodeEx (ceval.c:3433) ==24654== ==24654== LEAK SUMMARY: ==24654== definitely lost: 96 bytes in 1 blocks ==24654== indirectly lost: 16 bytes in 1 blocks I suppose we need to write a _pygi_marshal_cleanup_from_py_closure() and set this in the cache generation. Right now, there is no cleanup handler at all: (gdb) p *arg_cache $6 = {arg_name = 0x2aaaaabd5840 "closure", meta_type = PYGI_META_ARG_TYPE_PARENT, is_pointer = 1, is_caller_allocates = 0, is_skipped = 0, allow_none = 0, direction = PYGI_DIRECTION_FROM_PYTHON, transfer = GI_TRANSFER_NOTHING, type_tag = GI_TYPE_TAG_INTERFACE, type_info = 0xefb8a0, from_py_marshaller = 0x2aaaae3f3b51 <_pygi_marshal_from_py_interface_struct>, to_py_marshaller = 0x0, from_py_cleanup = 0x0, to_py_cleanup = 0x0, destroy_notify = 0x2aaaae3e8a97 <_interface_cache_free_func>, c_arg_index = 0, py_arg_index = 0}
Just to avoid me mixing up callbacks and closures: Can GClosures be valid after calling a function without that function ref'ing the closure itself? I. e. do we need to free them asynchonously in a callback, or is it safe to unref them right after the function call?
(In reply to comment #1) > Just to avoid me mixing up callbacks and closures: Can GClosures be valid after > calling a function without that function ref'ing the closure itself? I. e. do > we need to free them asynchonously in a callback, or is it safe to unref them > right after the function call? Since it is ref counted, I think we can treat it like any other object during marshaling: for transfer-full give the ref to the callee and we're done; for transfer-none, assume the callee will call the func or add its own ref if it wants to hang on to it, and then we call unref after invoke is complete (in marshaling cleanup code).
The following fixes have been pushed: f30001f Add GClosure marshalling cleanup cf4e830 Remove decrementing argument index for failed marshalling cleanup 662a442 Use cleanup data for argument marshalling failures
Created attachment 282119 [details] [review] Add GClosure marshalling cleanup Add marshalling cleanup for Python callables and boxed GClosures passed as arguments. Make sure the marshaller owns a reference until clean. Fix transfer everything case by adding a new reference. Remove unused header declaration: pygi_arg_gclosure_from_py_marshal
Created attachment 282120 [details] [review] Remove decrementing argument index for failed marshalling cleanup Remove index decrement when cleanup function is called for failed argument marshalling. The decrement is incorrect and causes the failed argument cleanup to be skipped. The decrement also causes cleanup for arguments prior to the failed argument to receive "was_successful" as FALSE, which is also incorrect.
Created attachment 282121 [details] [review] Use cleanup data for argument marshalling failures Use state->args_cleanup_data when cleaning up failed argument marshalling. This was overlooked when cleanup data tracking was implemented (commit 7407367f).