GNOME Bugzilla – Bug 747990
bin/element: gst_bin_add() and gst_element_add_pad() don't take reference in error cases
Last modified: 2017-05-17 12:32:22 UTC
In all error cases they just return FALSE but don't unref the element/pad. Also they give a g_critical() or g_warning(). For bindings that's never going to work, it must be consistent with the annotations always :)
*** Bug 752206 has been marked as a duplicate of this bug. ***
Created attachment 307201 [details] [review] fix pad memory leak
Created attachment 307202 [details] [review] bin: fix element leak
Comment on attachment 307202 [details] [review] bin: fix element leak I'm not sure this is entirely correct because of floating references. I think it should call gst_object_ref_sink() (somewhere, if it doesn't already. gst_object_set_parent() does that IIRC) and only then gst_object_unref(). For both patches.
Created attachment 307204 [details] [review] bin: fix element leak
Created attachment 307205 [details] [review] element: fix pad leak
Comment on attachment 307204 [details] [review] bin: fix element leak You also need to do the same in gst_bin_add_func()
Comment on attachment 307205 [details] [review] element: fix pad leak For the had_parent case, I think gst_object_set_parent() is not behaving correct
Created attachment 307228 [details] [review] bin:fix element leak fix element leak for error cases in gst_bin_add_func and gst_bin_add For element, i guess the changes are ok for the time being, until we decide on the removal of floating references.
Created attachment 307325 [details] [review] object: remove floating reference of object removing floating reference of the object when parent is already present for the object. By doing this here, each caller of this function need not remove the floating reference when this function fails. If this change is accepted, then the other two patches need to be changed such that in case of had_parent, there is no need to remove floating point references.
Sebastian, anything else needs to be done for this?
Vineeth. For the had_parent case, I guess, you don't need to call ref_sink. Because had_parent means that this object is already converted to normal And I'm not sure whether it should call unref for had_parent case. Because it also means that this object is owned by someone else.
More cases here that are inconsistent: https://bugzilla.gnome.org/show_bug.cgi?id=702960#c25
Hi Sebastian, I am not working on Gstreamer anymore.. Sorry about the incomplete fix.. Can you please proceed with the fix..
Sure, no problem at all Vineeth :) Thanks for letting me know
Created attachment 351880 [details] [review] gst: Fix floating reference inconsistencies in error cases If a function takes a floating reference and sinks it, it should also do that in error cases. I.e. call ref_sink() followed by unref(). Otherwise the reference counting behaviour of the function will be different between the good and the error case, and simply inconsistent.
Created attachment 351881 [details] [review] gst: Handle floating references consistently If a function takes a floating reference parameter, it should also be sinked in error cases. Otherwise the function behaves differently between error and normal cases, which is impossible for bindings to handle.
Attachment 351880 [details] pushed as 7c4d3a6 - gst: Fix floating reference inconsistencies in error cases Attachment 351881 [details] pushed as 2e4c82d - gst: Handle floating references consistently