GNOME Bugzilla – Bug 582707
Fix problems with memory management for in parameters
Last modified: 2018-01-21 22:56:14 UTC
Here's a patch that fixes a couple of problems that were introduced with: 03671d6 Fix ref count of (transfer full) GObject arguments. This (as far as I read the code, by inspection) causes all GObject parameters to be leaked for (transfer none), since we ref but then don't unref them. 771b26 Support GHashTable in parameters. ... Also fix memory management of (transfer full) and (transfer container) in parameters of GList/GSList/GHash types. This broke GList/GSList in parameters where the list contained boxed types or structures; unless we are (transfer full) we should not be copying or freeing them. (Copying always and freeing always would be an option, but making unnecessary copies of in boxed types is something we want to avoid - copying a boxed type is not always cheap.) The freeing part of this patch is a little ugly; I couldn't think of a better way to do it. No test cases, partly from laziness and partly from not wanting to have to extend Everything and cause more delay in getting this fixed.
Created attachment 134681 [details] [review] Fix problems with memory management for in parameters Add a transfer argument to gjs_value_to_g_argument() to distinguish the case where we need to copy arguments (transfer full) from where we only need to create a temporary if that is required when converting from JS to GObject (like for a string.) Use that to: - Catch problematical cases up front: (transfer container) in parameters and (transfer full) in lists of structures. - Not ref objects when not transfering (fixes a leak) When freeing an argument, distinguish freeing an in parameter from freeing something; when we are freeing an in parameter, we only need to free temporaries that we create. This fixes: - Freeing boxed structs/unions that objects that we didn't copy - Bailing out when we have something non-copyable (a non-boxed structure, for example.) Logic for what needs freeing is encapsulated in a new type_needs_release()
Looks OK to me, though I think test cases would be really good and pretty easy to add asynchronously. Otherwise next time someone looks at this code funny it will just break again.
I've pushed the patch. I'll try to come up with test cases this afternoon at least to check that the basic operation works. (Checking that we don't leak is harder.)
Patch seems to make sense; it did take some staring at. My initial inclination would have been to ref/copy all in parameters, so we didn't need the special cases for TRANSFER_IN_NOTHING, but I understand your not wanting to do the unnecessary copy of boxed types.
Comment on attachment 134681 [details] [review] Fix problems with memory management for in parameters (was committed ages ago; but is still open because we want test cases)
I think we probably have acquired some test cases for this in the meantime.