GNOME Bugzilla – Bug 755632
Memory leak in handle_setup_request
Last modified: 2016-10-25 13:04:09 UTC
Created attachment 312154 [details] [review] Possible fix of GstRTSPMedia leak In cases of some errors object of type GstRTSPMedia can leak. A pipeline of media leaks as well.
Thanks for the patch. How did you find/trigger this leak? It might be good to add a unit test for that. The unref in cleanup_transport causes assertion failures in our unit test suite, so something's not quite right with that or with the existing code. The other one isn't in a code path that's covered by the unit test, but it would be good to understand the critical from the last one first anyway. Please also attach a patch in 'git format-patch' format if possible at all.
Also, could you re-check if the leak still happens with the just-released 1.6.0? (windows binaries will be available shortly)
I have restricted the list of supported transports of RTSP media factory to TCP only so RTSP client tries UDP first, gets an error and then retries TCP. I found that find_media refs media object each time it is called but it is not always unrefed later on. GST_TRACE=mem-live highlights the leak. Which test fails? tests\check\gst\client.c passes. At least in 1.4.5 with my patch. And after I bound socket (g_socket_bind) before passing it to gst_rtsp_connection_create_from_socket... Have I just found another bug? I have to work with code commited to our SVN server so using Git just to create a single patch doesn't seem very convenient for me. Is it alright if I'll continue using svn diff for patches? If Git complains GNU patch should probably accept them without problems. I tried 1.6.0 windows binaries but my application failed to run at all. It complains about appsrc element being missing. gst-inspect-1.0 doesn't see it either. libgstapp.dll is also not present in lib\gstreamer-1.0. Is it supposed to be like this?
Yes, we can make simple diff patches work, that's not a problem. I don't know about those problems, perhaps you're missing some plugin modules like gst-plugins-base? If you have a simple test program that reproduces the leak, that would also be helpful.
Created attachment 312312 [details] test-readme.c with leak Reinstalled everything and now it finds all elements. However the leak still seems to be there. I took test-readme.c and added the line: gst_util_set_object_arg(G_OBJECT(factory), "protocols", "tcp"); Then I ran it with GST_TRACE=mem-live, connected 5 times with VLC and terminated it. In the output I see 5 lines like this: GstPipeline : (1) 0000000004522A80
Created attachment 332481 [details] [review] Possible fix leak
As well as reporter, I found the leak with GstRTSPMedia objects. I also found that some objects don't have unref after reffering in find_media function, so I've just added it in patch above. Used 1.8.2 version on Linux.
Comment on attachment 332481 [details] [review] Possible fix leak This is also all covered by the initial patch: cleanup_transport unrefs the media there.
Created attachment 332545 [details] [review] rtsp-client: Fix leaking of media in error cases
Created attachment 332546 [details] [review] rtsp-client: Fix leaking of session in error cases
(In reply to Sebastian Dröge (slomo) from comment #9) > Created attachment 332545 [details] [review] [review] > rtsp-client: Fix leaking of media in error cases This breaks the gst/client unit test
Comment on attachment 332546 [details] [review] rtsp-client: Fix leaking of session in error cases Attachment 332546 [details] pushed as 687301a - rtsp-client: Fix leaking of session in error cases
(In reply to Sebastian Dröge (slomo) from comment #12) > Comment on attachment 332546 [details] [review] [review] > rtsp-client: Fix leaking of session in error cases > > Attachment 332546 [details] pushed as 687301a - rtsp-client: Fix leaking of > session in error cases I'm sure your commit is useful but the initial report was about GstRTSPMedia objects memory leak... and it still here. You can check it like suggested in Nikita's comment #5. My patch resolved the problem at least for me. What about Nikita Bobkov patch I think it is OK too, but for 1.4.5 version. The reason why it failed in new version test suits is that it causes one more unref than needed in the case of 'unsupported_client_transport'. At 1.4.5 version 'unsupported_client_transport' label with 'cleanup_transport' label inside it was before last media object unref.
Created attachment 332564 [details] [review] rtsp-client: Fix leaking of media in error cases With additional fixes by Kseniya Vasilchuk <vailchukkseniia@gmail.com> and myself to make the media refcounting a bit easier to follow.
This one fixes that, and also a few other refcounting issues there.
commit de3d0c4522fb4b29887f43c2f274c27b1d0d2209 Author: Nikita Bobkov <NikitaDBobkov@gmail.com> Date: Fri Sep 25 15:04:00 2015 +0000 rtsp-client: Fix leaking of media in error cases With additional fixes by Kseniya Vasilchuk <vasilchukkseniia@gmail.com> and myself to make the media refcounting a bit easier to follow. https://bugzilla.gnome.org/show_bug.cgi?id=755632