Created attachment 160538 [details] [review]
[PATCH] fix refcounting in callbacks

 gi/pygi-closure.c |   30 ++++++++++++++----------------
 1 files changed, 14 insertions(+), 16 deletions(-)

Comment on attachment 160538 [details] [review]
[PATCH] fix refcounting in callbacks

>0ca68e36bc5bda392633718d9aeb9c97409e0920
>diff --git a/gi/pygi-closure.c b/gi/pygi-closure.c
>index 2c8d0b3..a8f67e2 100644
>--- a/gi/pygi-closure.c
>+++ b/gi/pygi-closure.c
>@@ -79,17 +79,19 @@ _pygi_closure_handle (ffi_cif *cif,
>                 {
>                     if (g_type_info_is_pointer(arg_type)) {
>                         if (closure->user_data == NULL) {
>-                            Py_INCREF(Py_None);
>                             if(PyTuple_SetItem(py_args, n_in_args, Py_None) != 0) {
>                                 PyErr_Clear();
>                                 goto end;
>                             }
>-                        } else if (PyTuple_SetItem(py_args, n_in_args, closure->user_data) != 0) {
>-                            PyErr_Print();
>-                            goto end;

Ref None after putting it in the tuple so if we get an error we don't have a dangling ref

>+                            Py_INCREF(Py_None);
>+                        } else {
>+                        	if (PyTuple_SetItem(py_args, n_in_args, closure->user_data) != 0) {
>+                                PyErr_Print();
>+                                goto end;
>+                        	}

ref userdata only if it is added to the tuple

>+                        	// decreffing the tuple will take care of user_data ref
>+                        	Py_XINCREF(closure->user_data);
>                         }
>-                        
>-                        Py_XINCREF(closure->user_data);
> 
>                         n_in_args++;
>                         continue;
>@@ -104,17 +106,17 @@ _pygi_closure_handle (ffi_cif *cif,
>                     pyarg = _pygi_argument_to_object (args[i],
>                                                       arg_type,
>                                                       arg_transfer);
>-                    
>+
>                     if(PyTuple_SetItem(py_args, n_in_args, pyarg) != 0) {
>                         PyErr_Print();
>                         goto end;
>                     }
>                     n_in_args++;
>                     g_base_info_unref((GIBaseInfo*)arg_info);
>-                    g_base_info_unref((GIBaseInfo*)arg_type);                                    
>+                    g_base_info_unref((GIBaseInfo*)arg_type);
>                 }
>         }
>-        
>+
>     }
> 
>     if(_PyTuple_Resize (&py_args, n_in_args) != 0) {
>@@ -124,8 +126,6 @@ _pygi_closure_handle (ffi_cif *cif,
> 
>     retval = PyObject_CallObject((PyObject *)closure->function, py_args);
> 

Move this to the end: label so if there is an error we also decref the tuple

>-    Py_DECREF(py_args);
>-
>     if (retval == NULL) {
>         PyErr_Print();
>         goto end;
>@@ -134,22 +134,20 @@ _pygi_closure_handle (ffi_cif *cif,
>     *(GArgument*)result = _pygi_argument_from_object(retval, return_type, return_transfer);
> 
> end:

tuple decref moved here

>+    Py_XDECREF(py_args);
>     g_base_info_unref((GIBaseInfo*)return_type);
> 
>     PyGILState_Release(state);
> 

Don't need this since decreffing the tuple automatically decrefs the userdata inside the tuple

>-    if (closure->user_data)
>-        Py_XDECREF(closure->user_data);
>-
>     /* Now that the closure has finished we can make a decision about how
>        to free it.  Scope call gets free'd at the end of wrap_g_function_info_invoke
>-       scope notified will be freed,  when the notify is called and we can free async 
>+       scope notified will be freed,  when the notify is called and we can free async
>        anytime we want as long as its after we return from this function (you can't free the closure
>        you are currently using!)
>     */
>     switch (closure->scope) {
>     case GI_SCOPE_TYPE_CALL:
>-    case GI_SCOPE_TYPE_NOTIFIED:        
>+    case GI_SCOPE_TYPE_NOTIFIED:
>         break;
>     case GI_SCOPE_TYPE_ASYNC:
>         /* Append this PyGICClosure to a list of closure that we will free

Writing a test case for this

Note that the patch at https://bugzilla.gnome.org/show_bug.cgi?id=617780 mostly rewrites the code this patch touches.

Mode comments on Tomeu's patch.  His fixes some of the issues with some other issues I noted and asked him to add to his patch.  I will base my patch off of his which will basically add a decref to _pygi_invoke_closure_free and add a test case.

Created attachment 160776 [details] [review]
[PATCH] make sure to decref userdata when closure is destroyed


* add refcounting test for userdata
---
 gi/pygi-closure.c        |    1 +
 tests/test_everything.py |   18 ++++++++++++++++++
 2 files changed, 19 insertions(+), 0 deletions(-)

Above patch adds the decref and user data refcount unit test.  The rest of the refcount fix is already reflected in Tomeu's patch in comment 4

Going to hold off on including this one in 0.5.1 until Tomeu's refactor of callback argument processing lands (probably for 0.6).

Just reviewed tomeu's new patch.  It incorporates the last bit of functionality in this patch.  The only piece of use here is the tests.  Will commit the tests once tomue commits his patch

Tests pushed