Bug 220091 – [MSE][GStreamer] SIGSEV in webKitMediaSrcFreeStream

• Component: Media
• Status: RESOLVED FIXED

[Sergio Villar Senin]

Created attachment 416673 [details]
Backtrace

Steps to reproduce:

1- Go to https://praza.gal/politica/a-audiencia-nacional-absolve-as-12-integrantes-de-causa-galiza-e-ceivar-e-di-que-o-independentismo-merece-proteccion-constitucional
2- Scroll down

I'm attaching a file with the crash backtrace.

[Philippe Normand]

This page has no videos though?

[Sergio Villar Senin]

(In reply to Philippe Normand from comment #1)
> This page has no videos though?

Maybe ads?

[Philippe Normand]

Still can't repro after disabling the ad-blocker in Ephy and disabling my PiHole.

[Philippe Normand]

Does this repro with Flatpak SDK builds too?

[Ivan Molodetskikh]

I got a similar-looking crash when full-screening a YouTube video that has finished playing.

Program terminated with signal SIGSEGV, Segmentation fault.
#0  0x00007f43ec32c012 in webKitMediaSrcFreeStream (source=source@entry=0x556ea7fbe140, stream=0x556ea76d2b40) at ../Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:512
512	    if (GST_IS_APP_SRC(stream->appsrc)) {
> bt full
#0  0x00007f43ec32c012 in webKitMediaSrcFreeStream(_WebKitMediaSrc*, _Stream*) (source=source@entry=0x556ea7fbe140 [WebKitMediaSrc], stream=0x556ea76d2b40) at ../Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:512
        __inst = 0x556ea73e5550
        __t = 0x556ea7c67900 [GstAppSrc/GstBaseSrc/GstElement/GstObject/GInitiallyUnowned]
        __r = <optimized out>
        __FUNCTION__ = "webKitMediaSrcFreeStream"
#1  0x00007f43ec32c7c7 in webKitMediaSrcFinalize(_GObject*) (object=0x556ea7fbe140 [WebKitMediaSrc]) at ../Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:278
        stream = <optimized out>
        __for_range = <synthetic pointer>: <optimized out>
        __for_begin = 0x7f428851b700
        __for_end = 0x7f428851b710
        source = 0x556ea7fbe140 [WebKitMediaSrc]
        priv = 0x556ea7fbe0b0
#2  0x00007f43eb0f2552 in g_object_unref (_object=<optimized out>) at ../gobject/gobject.c:3524
        weak_locations = <optimized out>
        old_ref = <optimized out>
        __func__ = "g_object_unref"
        object = 0x556ea7fbe140 [WebKitMediaSrc]
        __func__ = "g_object_unref"
#3  g_object_unref (_object=0x556ea7fbe140) at ../gobject/gobject.c:3416
        object = 0x556ea7fbe140 [WebKitMediaSrc]
        __func__ = "g_object_unref"
#4  0x00007f43e84b21a5 in gst_object_unref (object=<optimized out>) at ../gst/gstobject.c:266
        __func__ = "gst_object_unref"
#5  0x00007f43ec32cc3e in WTF::derefGPtr<_WebKitMediaSrc>(_WebKitMediaSrc*) (ptr=<optimized out>) at ../Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:799
#6  0x00007f43edc8acae in WTF::GRefPtr<_WebKitMediaSrc>::operator=(_WebKitMediaSrc*) (optr=<optimized out>, this=<optimized out>) at DerivedSources/ForwardingHeaders/wtf/glib/GRefPtr.h:150
        ptr = <optimized out>
        __FUNCTION__ = "setWebKitMediaSrc"
#7  WebCore::PlaybackPipeline::setWebKitMediaSrc(_WebKitMediaSrc*) (this=<optimized out>, webKitMediaSrc=<optimized out>) at ../Source/WebCore/platform/graphics/gstreamer/mse/PlaybackPipeline.cpp:96
        __FUNCTION__ = "setWebKitMediaSrc"
#8  0x00007fff86bd9d10 in  ()
#9  0x00007fff86bd9b50 in  ()
#10 0x0000000000000000 in  ()

F33 Wayland, Web 40.alpha-66-gf266e76f+ from gnome-nightly Flatpak

[Alicia Boya García]

Tried on current trunk both with the reported page and with YouTube but after many repeats I still couldn't reproduce the issue.

[Michael Catanzaro]

It's not reproducible, but it's still happening with 2.31.91. Just hit this after a reddit.com video finished playing.

[Philippe Normand]

I can reproduce easily here:

1. seek near end of a youtube video
2. let it EOS
3. wait a few seconds
4. try to replay same vide (reload button)

A seek to 0 is triggered which triggers a READY->PAUSED transition of the pipeline. Because uridecodebin already had a source element, it removes it (remove_source() called from setup_source()). So our MSE src element is being dereferenced by gst_bin_remove() while we still have its pointer stored in the player m_source...

[Philippe Normand]

*** Bug 203378 has been marked as a duplicate of this bug. ***

[Philippe Normand]

*** Bug 196902 has been marked as a duplicate of this bug. ***

[Philippe Normand]

Created attachment 423827 [details]
Patch

[Michael Catanzaro]

*** Bug 223514 has been marked as a duplicate of this bug. ***

[Xabier Rodríguez Calvar]

Comment on attachment 423827 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=423827&action=review

Other than my comments, maybe Alicia has something more to say.

> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:520
> +        gst_object_unref(stream->appsrc);

Can't we make this a GRefPtr?

> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:574
>      delete stream;

It might be a a good moment to make Vector<Stream*> streams in the Private.h as a Vector<std::unique_ptr<Stream>>...

> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:709
> +        stream->appsrc = gst_element_factory_make("appsrc", nullptr);

If we are going to keep a reference here, we should ref_sink it, otherwise in line 720 we end up having the appsrc with 2 references but still the floating flag just after evaluating the arguments before calling the function, which is semantically wrong.

Well, after calling gst_bin_add things go to their place because the gst_bin_add is going to perform a ref_sink so it is going to remove the floating ref and the result is the same, but it is semantically wrong. You should ref_sink here, avoid reffing below, and when the gst_bin_add does its ref_sink, it will just increase the reference because the floating ref was already sinked before.

Turning it into a GRefPtr, as I said above would solve this issue.

> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:720
> +        gst_bin_add(GST_BIN_CAST(newSource), GST_ELEMENT_CAST(gst_object_ref(stream->appsrc)));

Please read above.

[Philippe Normand]

I thought about switch the appsrc to a GRefPtr but wasn't sure because this could not ease further rebases of the new MSE backend on top. Your call though :)

[Xabier Rodríguez Calvar]

(In reply to Philippe Normand from comment #14)
> I thought about switch the appsrc to a GRefPtr but wasn't sure because this
> could not ease further rebases of the new MSE backend on top. Your call
> though :)

I think we should change it unless you tell me you're going to change it next month. Anyway, if you prefer to keep it as raw pointer, please change what I told you regarding the ref/ref_sink.

[Philippe Normand]

I'd like to avoid too big changes in this code until the new backend lands.

I worked on this mostly for the 2.32 release because the new backend missed that window.

[Xabier Rodríguez Calvar]

Comment on attachment 423827 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=423827&action=review

>> Source/WebCore/platform/graphics/gstreamer/mse/WebKitMediaSourceGStreamer.cpp:709
>> +        stream->appsrc = gst_element_factory_make("appsrc", nullptr);
> 
> If we are going to keep a reference here, we should ref_sink it, otherwise in line 720 we end up having the appsrc with 2 references but still the floating flag just after evaluating the arguments before calling the function, which is semantically wrong.
> 
> Well, after calling gst_bin_add things go to their place because the gst_bin_add is going to perform a ref_sink so it is going to remove the floating ref and the result is the same, but it is semantically wrong. You should ref_sink here, avoid reffing below, and when the gst_bin_add does its ref_sink, it will just increase the reference because the floating ref was already sinked before.
> 
> Turning it into a GRefPtr, as I said above would solve this issue.

Ok, please solve this then and land.

[Philippe Normand]

Created attachment 424015 [details]
Patch

[EWS]

Committed r274870: <https://commits.webkit.org/r274870>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 424015 [details].

[Radar WebKit Bug Importer]

<rdar://problem/75738646>