GNOME Bugzilla – Bug 726206
Handle GI_TRANSFER_EVERYTHING for returns of foreign structures
Last modified: 2014-03-13 13:27:57 UTC
Any (transfer full) return of a cairo type other than a path was leaked. Pass the transfer type PyGIArgOverrideFromGIArgumentFunc and handle it for the cairo foreign type. For paths we can only handle (transfer full) so throw an error for (transfer none).
Created attachment 271664 [details] [review] Handle GI_TRANSFER_EVERYTHING for returns of foreign structures
Review of attachment 271664 [details] [review]: Thanks for this. I verified this indeed cleans up the following leaks when running tests/test_everything.py which hits cairo marshaling: 2 bytes, 1 blocks malloc ??? ??? regress_test_cairo_context_full_return ffi_call_unix64 ffi_call _invoke_callable pygi_callable_info_invoke _wrap_g_callable_info_invoke _callable_info_call _function_info_call PyObject_Call 1 bytes, 1 blocks malloc ??? ??? regress_test_cairo_surface_full_out ffi_call_unix64 ffi_call _invoke_callable pygi_callable_info_invoke _wrap_g_callable_info_invoke _callable_info_call _function_info_call PyObject_Call 1 bytes, 1 blocks malloc ??? ??? regress_test_cairo_surface_full_return ffi_call_unix64 ffi_call _invoke_callable pygi_callable_info_invoke _wrap_g_callable_info_invoke _callable_info_call _function_info_call PyObject_Call ::: gi/pygi-foreign-cairo.c @@ +148,3 @@ + if (transfer == GI_TRANSFER_NOTHING) { + PyErr_SetString(PyExc_TypeError, "Unsupported annotation (transfer none) for cairo.Path return"); Shouldn't we do a cairo_copy_path to handle this case? e.g passing copied memory to PycairoPath_FromPath which it will then manage (not sure if there is a real world use for this but it still seems valid...)
> + if (transfer == GI_TRANSFER_NOTHING) { > + PyErr_SetString(PyExc_TypeError, "Unsupported annotation (transfer > none) for cairo.Path return"); > > Shouldn't we do a cairo_copy_path to handle this case? e.g passing copied > memory to PycairoPath_FromPath which it will then manage (not sure if there is > a real world use for this but it still seems valid...) cairo_copy_path() isn't what it sounds like, if you look at it's signature it's: cairo_path_t *cairo_copy_path (cairo_t *cr); What it does is makes a copy of the current path for the context (the cairo_t). In theory you could: - Creating a 1x1 image surface - Creating a context for it - Using cairo_append_path() path to append the path on it - Using cairo_copy_path() to get the path back - Destroy the surface and context But practically speaking, there's no way to copy a cairo_path_t, so an API that returns a cairo_path_t (transfer none) is buggy and unbindable.
Comment on attachment 271664 [details] [review] Handle GI_TRANSFER_EVERYTHING for returns of foreign structures OK, thanks for the clarification.
Attachment 271664 [details] pushed as bbfcebd - Handle GI_TRANSFER_EVERYTHING for returns of foreign structures