Bug 118473

Summary: [GTK] Leak: GstElement* leaking in MediaPlayerPrivateGStreamer::createAudioSink()
Product: WebKit Reporter: Brian Holt <brian.holt>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: RESOLVED INVALID    
Severity: Normal CC: cdumez, cgarcia, commit-queue, eric.carlson, glenn, gustavo, jer.noble, kbalazs, menard, mrobinson, pnormand
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 116317    
Attachments:
Description Flags
Patch none

Brian Holt
Reported 2013-07-08 09:09:53 PDT
In Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp: Leaks found using the "--leak" option in the Gtk port: Command: /home/likewise-open/SERILOCAL/brian.holt/Code/gnome3/WebKit/WebKitBuild/Release/Programs/DumpRenderTree - Leak_StillReachable 3,328 bytes in 13 blocks are still reachable in loss record 939 of 1,061 realloc (/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) g_realloc (/WebKitBuild/Dependencies/Source/glib-2.36.0/glib/gmem.c:224) g_array_maybe_expand (/WebKitBuild/Dependencies/Source/glib-2.36.0/glib/garray.c:785) g_array_append_vals (/WebKitBuild/Dependencies/Source/glib-2.36.0/glib/garray.c:423) gst_structure_id_set_value (/WebKitBuild/Dependencies/Source/gstreamer/gst/gststructure.c:476) gst_element_class_set_static_metadata (/WebKitBuild/Dependencies/Source/gstreamer/gst/gstelement.c:1345) gst_audio_convert_class_intern_init (/WebKitBuild/Dependencies/Source/gst-plugins-base/gst/audioconvert/gstaudioconvert.c:204) g_type_class_ref (/WebKitBuild/Dependencies/Source/glib-2.36.0/gobject/gtype.c:2239) g_object_newv (/WebKitBuild/Dependencies/Source/glib-2.36.0/gobject/gobject.c:1624) gst_element_factory_create (/WebKitBuild/Dependencies/Source/gstreamer/gst/gstelementfactory.c:377) gst_element_factory_make (/WebKitBuild/Dependencies/Source/gstreamer/gst/gstelementfactory.c:446) WebCore::MediaPlayerPrivateGStreamer::createAudioSink() (/home/likewise-open/SERILOCAL/brian.holt/Code/gnome3/WebKit/WebKitBuild/Release/.libs/libwebkitgtk-3.0.so.0.19.1) WebCore::MediaPlayerPrivateGStreamer::createGSTPlayBin() (/home/likewise-open/SERILOCAL/brian.holt/Code/gnome3/WebKit/WebKitBuild/Release/.libs/libwebkitgtk-3.0.so.0.19.1) WebCore::MediaPlayerPrivateGStreamer::load(WTF::String const&) [clone .part.24] (/home/likewise-open/SERILOCAL/brian.holt/Code/gnome3/WebKit/WebKitBuild/Release/.libs/libwebkitgtk-3.0.so.0.19.1) WebCore::MediaPlayer::loadWithNextMediaEngine(WebCore::MediaPlayerFactory*) (/home/likewise-open/SERILOCAL/brian.holt/Code/gnome3/WebKit/WebKitBuild/Release/.libs/libwebkitgtk-3.0.so.0.19.1) WebCore::MediaPlayer::load(WebCore::KURL const&, WebCore::ContentType const&, WTF::String const&) (/home/likewise-open/SERILOCAL/brian.holt/Code/gnome3/WebKit/WebKitBuild/Release/.libs/libwebkitgtk-3.0.so.0.19.1) WebCore::HTMLMediaElement::loadResource(WebCore::KURL const&, WebCore::ContentType&, WTF::String const&) (/home/likewise-open/SERILOCAL/brian.holt/Code/gnome3/WebKit/WebKitBuild/Release/.libs/libwebkitgtk-3.0.so.0.19.1) WebCore::HTMLMediaElement::selectMediaResource() (/home/likewise-open/SERILOCAL/brian.holt/Code/gnome3/WebKit/WebKitBuild/Release/.libs/libwebkitgtk-3.0.so.0.19.1) WebCore::HTMLMediaElement::loadTimerFired(WebCore::Timer<WebCore::HTMLMediaElement>*) (/home/likewise-open/SERILOCAL/brian.holt/Code/gnome3/WebKit/WebKitBuild/Release/.libs/libwebkitgtk-3.0.so.0.19.1) WebCore::ThreadTimers::sharedTimerFiredInternal() (/home/likewise-open/SERILOCAL/brian.holt/Code/gnome3/WebKit/WebKitBuild/Release/.libs/libwebkitgtk-3.0.so.0.19.1) WebCore::timeout_cb(void*) (/home/likewise-open/SERILOCAL/brian.holt/Code/gnome3/WebKit/WebKitBuild/Release/.libs/libwebkitgtk-3.0.so.0.19.1) g_timeout_dispatch (/WebKitBuild/Dependencies/Source/glib-2.36.0/glib/gmain.c:4413) g_main_context_dispatch (/WebKitBuild/Dependencies/Source/glib-2.36.0/glib/gmain.c:3054) g_main_context_iterate.isra.22 (/WebKitBuild/Dependencies/Source/glib-2.36.0/glib/gmain.c:3701) g_main_loop_run (/WebKitBuild/Dependencies/Source/glib-2.36.0/glib/gmain.c:3895) gtk_main (/WebKitBuild/Dependencies/Source/gtk+-3.6.0/gtk/gtkmain.c:1163) runTest(std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) (/home/likewise-open/SERILOCAL/brian.holt/Code/gnome3/WebKit/WebKitBuild/Release/Programs/DumpRenderTree) runTestingServerLoop() (/home/likewise-open/SERILOCAL/brian.holt/Code/gnome3/WebKit/WebKitBuild/Release/Programs/DumpRenderTree) Suppression (error hash=#FDF24213CEA9E828#): For more info on using suppressions see http://dev.chromium.org/developers/tree-sheriffs/sheriff-details-chromium/memory-sheriff#TOC-Suppressing-memory-reports { <insert_a_suppression_name_here> Memcheck:Leak fun:realloc fun:g_realloc fun:g_array_maybe_expand fun:g_array_append_vals fun:gst_structure_id_set_value fun:gst_element_class_set_static_metadata fun:gst_audio_convert_class_intern_init fun:g_type_class_ref fun:g_object_newv fun:gst_element_factory_create fun:gst_element_factory_make fun:_ZN7WebCore27MediaPlayerPrivateGStreamer15createAudioSinkEv fun:_ZN7WebCore27MediaPlayerPrivateGStreamer16createGSTPlayBinEv fun:_ZN7WebCore27MediaPlayerPrivateGStreamer4loadERKN3WTF6StringE.part.24 fun:_ZN7WebCore11MediaPlayer23loadWithNextMediaEngineEPNS_18MediaPlayerFactoryE fun:_ZN7WebCore11MediaPlayer4loadERKNS_4KURLERKNS_11ContentTypeERKN3WTF6StringE fun:_ZN7WebCore16HTMLMediaElement12loadResourceERKNS_4KURLERNS_11ContentTypeERKN3WTF6StringE fun:_ZN7WebCore16HTMLMediaElement19selectMediaResourceEv fun:_ZN7WebCore16HTMLMediaElement14loadTimerFiredEPNS_5TimerIS0_EE fun:_ZN7WebCore12ThreadTimers24sharedTimerFiredInternalEv fun:_ZN7WebCoreL10timeout_cbEPv fun:g_timeout_dispatch }
Attachments
Patch (3.30 KB, patch)
2013-07-08 09:14 PDT, Brian Holt 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Brian Holt
Comment 1 2013-07-08 09:14:08 PDT
Created attachment 206249 [details] Patch
Carlos Garcia Campos
Comment 2 2013-07-08 09:29:20 PDT
I don't know much about gst, but I thought elements had a floating reference that is consumed when added to a container.
Chris Dumez
Comment 3 2013-07-08 09:31:40 PDT
(In reply to comment #2) > I don't know much about gst, but I thought elements had a floating reference that is consumed when added to a container. Indeed, and the proof of that is that you don't actually "adopt". So which one(s) were actually leaking?
Chris Dumez
Comment 4 2013-07-08 09:34:24 PDT
Comment on attachment 206249 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=206249&action=review > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1578 > + GRefPtr<GstElement> audioSink = adoptGRef(gst_bin_new("audio-sink")); The doc says that gst_bin_new() returns a floating reference: http://gstreamer.freedesktop.org/data/doc/gstreamer/head/gstreamer/html/GstBin.html#gst-bin-new How come adopting doesn't assert here? template <> GRefPtr<GstElement> adoptGRef(GstElement* ptr) { ASSERT(!ptr || !gstObjectIsFloating(GST_OBJECT(ptr))); return GRefPtr<GstElement>(ptr, GRefPtrAdopt); }
Brian Holt
Comment 5 2013-07-08 09:52:49 PDT
Comment on attachment 206249 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=206249&action=review >> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1578 >> + GRefPtr<GstElement> audioSink = adoptGRef(gst_bin_new("audio-sink")); > > The doc says that gst_bin_new() returns a floating reference: > http://gstreamer.freedesktop.org/data/doc/gstreamer/head/gstreamer/html/GstBin.html#gst-bin-new > > How come adopting doesn't assert here? > template <> GRefPtr<GstElement> adoptGRef(GstElement* ptr) > { > ASSERT(!ptr || !gstObjectIsFloating(GST_OBJECT(ptr))); > return GRefPtr<GstElement>(ptr, GRefPtrAdopt); > } Thanks very much for the link - I should have checked first whether gst_bin_new returns a floating reference. Regarding the ASSERT, I'm working with a release build so it's ifdef'ed out.
Chris Dumez
Comment 6 2013-07-08 09:55:05 PDT
Comment on attachment 206249 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=206249&action=review >>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1578 >>> + GRefPtr<GstElement> audioSink = adoptGRef(gst_bin_new("audio-sink")); >> >> The doc says that gst_bin_new() returns a floating reference: >> http://gstreamer.freedesktop.org/data/doc/gstreamer/head/gstreamer/html/GstBin.html#gst-bin-new >> >> How come adopting doesn't assert here? >> template <> GRefPtr<GstElement> adoptGRef(GstElement* ptr) >> { >> ASSERT(!ptr || !gstObjectIsFloating(GST_OBJECT(ptr))); >> return GRefPtr<GstElement>(ptr, GRefPtrAdopt); >> } > > Thanks very much for the link - I should have checked first whether gst_bin_new returns a floating reference. > > Regarding the ASSERT, I'm working with a release build so it's ifdef'ed out. Sometimes to streamer doc is wrong about these things so it should be double checked. Also note that the behaviour is sometimes different between streamer 0.10 and gstreamer 1.0. It is important that you test with a debug build.
Brian Holt
Comment 7 2013-07-08 09:57:11 PDT
(In reply to comment #3) > (In reply to comment #2) > > I don't know much about gst, but I thought elements had a floating reference that is consumed when added to a container. > > Indeed, and the proof of that is that you don't actually "adopt". So which one(s) were actually leaking? I think all of them were leaking: gst_element_factory_make () returns a new GstElement or NULL if unable to create element. [transfer floating]
Brian Holt
Comment 8 2013-07-08 09:57:53 PDT
(In reply to comment #6) > (From update of attachment 206249 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=206249&action=review > > >>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1578 > >>> + GRefPtr<GstElement> audioSink = adoptGRef(gst_bin_new("audio-sink")); > >> > >> The doc says that gst_bin_new() returns a floating reference: > >> http://gstreamer.freedesktop.org/data/doc/gstreamer/head/gstreamer/html/GstBin.html#gst-bin-new > >> > >> How come adopting doesn't assert here? > >> template <> GRefPtr<GstElement> adoptGRef(GstElement* ptr) > >> { > >> ASSERT(!ptr || !gstObjectIsFloating(GST_OBJECT(ptr))); > >> return GRefPtr<GstElement>(ptr, GRefPtrAdopt); > >> } > > > > Thanks very much for the link - I should have checked first whether gst_bin_new returns a floating reference. > > > > Regarding the ASSERT, I'm working with a release build so it's ifdef'ed out. > > Sometimes to streamer doc is wrong about these things so it should be double checked. Also note that the behaviour is sometimes different between streamer 0.10 and gstreamer 1.0. > It is important that you test with a debug build. I definitely will in the future. Thanks for the helpful comments.
Chris Dumez
Comment 9 2013-07-08 10:01:20 PDT
(In reply to comment #7) > (In reply to comment #3) > > (In reply to comment #2) > > > I don't know much about gst, but I thought elements had a floating reference that is consumed when added to a container. > > > > Indeed, and the proof of that is that you don't actually "adopt". So which one(s) were actually leaking? > > I think all of them were leaking: gst_element_factory_make () returns a new GstElement or NULL if unable to create element. [transfer floating] gst_element_factory() returns a floating reference and get_bin_add_many() should take ownership of the returned values later on: http://gstreamer.freedesktop.org/data/doc/gstreamer/head/gstreamer/html/GstBin.html#gst-bin-add Therefore, I don't understand why it leaks.
Carlos Garcia Campos
Comment 10 2013-07-08 10:01:49 PDT
(In reply to comment #7) > (In reply to comment #3) > > (In reply to comment #2) > > > I don't know much about gst, but I thought elements had a floating reference that is consumed when added to a container. > > > > Indeed, and the proof of that is that you don't actually "adopt". So which one(s) were actually leaking? > > I think all of them were leaking: gst_element_factory_make () returns a new GstElement or NULL if unable to create element. [transfer floating] a GstElement is a GInitiallyUnowned, so it's returning a new element with a floating reference. You only need to convert it into a normal ref (which is what GRefPtr is does) if it's not added to a container that consumes the floating ref.
Brian Holt
Comment 11 2013-07-09 02:37:10 PDT
Some more detail about the origins of the leaks from the debug build: Leak_StillReachable 40 bytes in 1 blocks are still reachable in loss record 335 of 1,176 malloc (/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ... gst_element_factory_make (/WebKitBuild/Dependencies/Source/gstreamer/gst/gstelementfactory.c:446) WebCore::MediaPlayerPrivateGStreamer::createAudioSink() (/WebKitBuild/Debug/../../Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1564) GstElement* scale = gst_element_factory_make("scaletempo", 0); Leak_StillReachable 554 bytes in 41 blocks are still reachable in loss record 855 of 1,176 malloc (/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ... gst_element_factory_make (/WebKitBuild/Dependencies/Source/gstreamer/gst/gstelementfactory.c:446) WebCore::MediaPlayerPrivateGStreamer::createAudioSink() (/WebKitBuild/Debug/../../Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1570) GstElement* convert = gst_element_factory_make("audioconvert", 0); Leak_StillReachable 576 bytes in 4 blocks are still reachable in loss record 836 of 1,139 realloc (/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ... gst_element_factory_make (/WebKitBuild/Dependencies/Source/gstreamer/gst/gstelementfactory.c:446) WebCore::MediaPlayerPrivateGStreamer::createAudioSink() (/WebKitBuild/Debug/../../Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1571) GstElement* resample = gst_element_factory_make("audioresample", 0); Leak_StillReachable 304 bytes in 10 blocks are still reachable in loss record 750 of 1,176 malloc (/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ... gst_element_factory_make (/WebKitBuild/Dependencies/Source/gstreamer/gst/gstelementfactory.c:446) WebCore::MediaPlayerPrivateGStreamer::createAudioSink() (/WebKitBuild/Debug/../../Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1572) GstElement* sink = gst_element_factory_make("autoaudiosink", 0); Leak_StillReachable 120 bytes in 3 blocks are still reachable in loss record 525 of 1,139 malloc (/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ... gst_element_factory_make (/WebKitBuild/Dependencies/Source/gstreamer/gst/gstelementfactory.c:446) WebCore::MediaPlayerPrivateGStreamer::createAudioSink() (/WebKitBuild/Debug/../../Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1578) GstElement* audioSink = gst_bin_new("audio-sink"); Additionally there are leaks at Leak_StillReachable 3,928 bytes in 49 blocks are still reachable in loss record 1,016 of 1,138 calloc (/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ... gst_element_factory_make (/WebKitBuild/Dependencies/Source/gstreamer/gst/gstelementfactory.c:446) WebCore::MediaPlayerPrivateGStreamer::createGSTPlayBin() (/WebKitBuild/Debug/../../Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1599) m_playBin = gst_element_factory_make(gPlaybinName, "play"); Leak_StillReachable 3,104 bytes in 28 blocks are still reachable in loss record 1,008 of 1,139 calloc (/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ... gst_element_factory_make (/WebKitBuild/Dependencies/Source/gstreamer/gst/gstelementfactory.c:446) WebCore::MediaPlayerPrivateGStreamerBase::createVideoSink(_GstElement*) (/WebKitBuild/Debug/../../Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:563) m_fpsSink = gst_element_factory_make("fpsdisplaysink", "sink");
Balazs Kelemen
Comment 12 2013-07-09 02:53:52 PDT
(In reply to comment #11) > Some more detail about the origins of the leaks from the debug build: > > Leak_StillReachable > 40 bytes in 1 blocks are still reachable in loss record 335 of 1,176 > malloc (/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) > ... > gst_element_factory_make (/WebKitBuild/Dependencies/Source/gstreamer/gst/gstelementfactory.c:446) > WebCore::MediaPlayerPrivateGStreamer::createAudioSink() (/WebKitBuild/Debug/../../Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1564) > GstElement* scale = gst_element_factory_make("scaletempo", 0); > still-reachebles are not leaks, at least in WebKit sense. It's normal that there are unfreed heap objects at termination because WebKit is intentionally does not care about that (the idea is to terminate faster, the OS frees up the memory anyway). It means that for example singletons are usually created on the heap and never freed. We should be sure that the objects holding these pointers are actually destroyed. Only in that case can we say that these are real leaks. Given that in this case valgrind would rather report definitely-lost failures, I assume these are not leaks.
Brian Holt
Comment 13 2013-07-09 03:03:33 PDT
(In reply to comment #12) > (In reply to comment #11) > > Some more detail about the origins of the leaks from the debug build: > > > > Leak_StillReachable > > 40 bytes in 1 blocks are still reachable in loss record 335 of 1,176 > > malloc (/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) > > ... > > gst_element_factory_make (/WebKitBuild/Dependencies/Source/gstreamer/gst/gstelementfactory.c:446) > > WebCore::MediaPlayerPrivateGStreamer::createAudioSink() (/WebKitBuild/Debug/../../Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1564) > > GstElement* scale = gst_element_factory_make("scaletempo", 0); > > > > still-reachebles are not leaks, at least in WebKit sense. > It's normal that there are unfreed heap objects at termination because WebKit is intentionally does not care about that (the idea is to terminate faster, the OS frees up the memory anyway). It means that for example singletons are usually created on the heap and never freed. > We should be sure that the objects holding these pointers are actually destroyed. Only in that case can we say that these are real leaks. Given that in this case valgrind would rather report definitely-lost failures, I assume these are not leaks. That's a fair point. I will restrict my efforts to Definitely-Lost in the meantime. Should I mark this as wontfix?
Balazs Kelemen
Comment 14 2013-07-09 03:26:54 PDT
(In reply to comment #13) > (In reply to comment #12) > > (In reply to comment #11) > > > Some more detail about the origins of the leaks from the debug build: > > > > > > Leak_StillReachable > > > 40 bytes in 1 blocks are still reachable in loss record 335 of 1,176 > > > malloc (/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) > > > ... > > > gst_element_factory_make (/WebKitBuild/Dependencies/Source/gstreamer/gst/gstelementfactory.c:446) > > > WebCore::MediaPlayerPrivateGStreamer::createAudioSink() (/WebKitBuild/Debug/../../Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1564) > > > GstElement* scale = gst_element_factory_make("scaletempo", 0); > > > > > > > still-reachebles are not leaks, at least in WebKit sense. > > It's normal that there are unfreed heap objects at termination because WebKit is intentionally does not care about that (the idea is to terminate faster, the OS frees up the memory anyway). It means that for example singletons are usually created on the heap and never freed. > > We should be sure that the objects holding these pointers are actually destroyed. Only in that case can we say that these are real leaks. Given that in this case valgrind would rather report definitely-lost failures, I assume these are not leaks. > > That's a fair point. I will restrict my efforts to Definitely-Lost in the meantime. > > Should I mark this as wontfix? I would rather mark it invalid.
Note You need to log in before you can comment on or make changes to this bug.