Bug 118473 – [GTK] Leak: GstElement* leaking in MediaPlayerPrivateGStreamer::createAudioSink()

• Status: RESOLVED
• Resolution: INVALID
• Component: WebKitGTK

[Brian Holt]

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
}

[Brian Holt]

Created attachment 206249 [details]
Patch

[Carlos Garcia Campos]

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]

(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 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 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 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]

(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]

(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]

(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]

(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]

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]

(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]

(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]

(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.