Bug 220091

Summary: [MSE][GStreamer] SIGSEV in webKitMediaSrcFreeStream
Product: WebKit Reporter: Sergio Villar Senin <svillar>
Component: MediaAssignee: Philippe Normand <pnormand>
Status: RESOLVED FIXED    
Severity: Normal CC: aboya, calvaris, cgarcia, eocanha, eric.carlson, ews-watchlist, glenn, gustavo, jer.noble, mcatanzaro, menard, philipj, pnormand, sergio, vjaquez, webkit-bug-importer, yalterz
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
URL: https://praza.gal/politica/a-audiencia-nacional-absolve-as-12-integrantes-de-causa-galiza-e-ceivar-e-di-que-o-independentismo-merece-proteccion-constitucional
See Also: https://bugs.webkit.org/show_bug.cgi?id=194592
https://bugs.webkit.org/show_bug.cgi?id=223674
Bug Depends on:    
Bug Blocks: 223674    
Attachments:
Description Flags
Backtrace
none
Patch
none
Patch none

Description Sergio Villar Senin 2020-12-22 10:37:55 PST
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.
Comment 1 Philippe Normand 2020-12-25 08:14:13 PST
This page has no videos though?
Comment 2 Sergio Villar Senin 2020-12-28 11:59:52 PST
(In reply to Philippe Normand from comment #1)
> This page has no videos though?

Maybe ads?
Comment 3 Philippe Normand 2021-01-04 02:14:04 PST
Still can't repro after disabling the ad-blocker in Ephy and disabling my PiHole.
Comment 4 Philippe Normand 2021-01-04 02:16:17 PST
Does this repro with Flatpak SDK builds too?
Comment 5 Ivan Molodetskikh 2021-01-29 10:15:33 PST
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
Comment 6 Alicia Boya García 2021-02-05 12:41:31 PST
Tried on current trunk both with the reported page and with YouTube but after many repeats I still couldn't reproduce the issue.
Comment 7 Michael Catanzaro 2021-03-20 10:04:22 PDT
It's not reproducible, but it's still happening with 2.31.91. Just hit this after a reddit.com video finished playing.
Comment 8 Philippe Normand 2021-03-20 10:46:28 PDT
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...
Comment 9 Philippe Normand 2021-03-21 07:54:34 PDT
*** Bug 203378 has been marked as a duplicate of this bug. ***
Comment 10 Philippe Normand 2021-03-21 07:55:12 PDT
*** Bug 196902 has been marked as a duplicate of this bug. ***
Comment 11 Philippe Normand 2021-03-21 09:10:25 PDT
Created attachment 423827 [details]
Patch
Comment 12 Michael Catanzaro 2021-03-21 10:53:38 PDT
*** Bug 223514 has been marked as a duplicate of this bug. ***
Comment 13 Xabier Rodríguez Calvar 2021-03-22 05:49:38 PDT
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.
Comment 14 Philippe Normand 2021-03-22 05:57:31 PDT
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 :)
Comment 15 Xabier Rodríguez Calvar 2021-03-22 07:50:13 PDT
(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.
Comment 16 Philippe Normand 2021-03-22 10:10:29 PDT
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.
Comment 17 Xabier Rodríguez Calvar 2021-03-23 01:51:57 PDT
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.
Comment 18 Philippe Normand 2021-03-23 07:27:45 PDT
Created attachment 424015 [details]
Patch
Comment 19 EWS 2021-03-23 08:32:33 PDT
Committed r274870: <https://commits.webkit.org/r274870>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 424015 [details].
Comment 20 Radar WebKit Bug Importer 2021-03-23 08:33:15 PDT
<rdar://problem/75738646>