Summary: | [MSE][GStreamer] SIGSEV in webKitMediaSrcFreeStream | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Sergio Villar Senin <svillar> | ||||||||
Component: | Media | Assignee: | 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
Sergio Villar Senin
2020-12-22 10:37:55 PST
This page has no videos though? (In reply to Philippe Normand from comment #1) > This page has no videos though? Maybe ads? Still can't repro after disabling the ad-blocker in Ephy and disabling my PiHole. Does this repro with Flatpak SDK builds too? 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
Tried on current trunk both with the reported page and with YouTube but after many repeats I still couldn't reproduce the issue. It's not reproducible, but it's still happening with 2.31.91. Just hit this after a reddit.com video finished playing. 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... *** Bug 203378 has been marked as a duplicate of this bug. *** *** Bug 196902 has been marked as a duplicate of this bug. *** Created attachment 423827 [details]
Patch
*** Bug 223514 has been marked as a duplicate of this bug. *** 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. 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 :) (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. 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 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. Created attachment 424015 [details]
Patch
Committed r274870: <https://commits.webkit.org/r274870> All reviewed patches have been landed. Closing bug and clearing flags on attachment 424015 [details]. |