Bug 152043

Summary:

[GStreamer] MainThreadNotifier ASSERTION FAILED: m_boundThread == currentThread() in _WebKitWebSrcPrivate::~_WebKitWebSrcPrivate

Product:

WebKit

Reporter:

Philippe Normand <pnormand>

Component:

Platform

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

aplazas, bugs-noreply, buildbot, calvaris, cgarcia, clopez, eocanha, ht990332, mcatanzaro, mrobinson, zan

Priority:

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

See Also:

https://bugs.webkit.org/show_bug.cgi?id=108088
https://bugs.webkit.org/show_bug.cgi?id=144040
https://bugs.webkit.org/show_bug.cgi?id=167946

Bug Depends on:

144040

Bug Blocks:

149594

Attachments:

Description	Flags
Full backtrace	none
Patch	calvaris: review+

Philippe Normand

Reported 2015-12-09 01:27:22 PST

When debugging Bug 149594 I stumbled upon this: (gdb) bt #0 0x00007f836ab2c8be in WTFCrash () at ../../Source/WTF/wtf/Assertions.cpp:321 #1 0x00007f8372338aeb in WTF::WeakReference<WebCore::MainThreadNotifier<MainThreadSourceNotification> >::get (this=0x7f83109be000) at ../../Source/WTF/wtf/WeakPtr.h:55 #2 0x00007f83723384d2 in WTF::WeakPtr<WebCore::MainThreadNotifier<MainThreadSourceNotification> >::operator bool (this=0x7f82e41013f0) at ../../Source/WTF/wtf/WeakPtr.h:99 #3 0x00007f8372334e4b in WebCore::MainThreadNotifier<MainThreadSourceNotification>::<lambda()>::operator()(void) const (__closure=0x7f82e41013f0) at ../../Source/WebCore/platform/graphics/gstreamer/MainThreadNotifier.h:52 #4 0x00007f8372336296 in std::_Function_handler<void(), WebCore::MainThreadNotifier<T>::notify(T, const F&) [with F = webKitWebSrcChangeState(GstElement*, GstStateChange)::<lambda()>; T = MainThreadSourceNotification]::<lambda()> >::_M_invoke(const std::_Any_data &) (__functor=...) at /usr/include/c++/5/functional:1871 #5 0x00007f83707f5c66 in std::function<void()>::operator()(void) const (this=0x7ffe29c5af80) at /usr/include/c++/5/functional:2271 #6 0x00007f836ab4627d in WTF::RunLoop::performWork (this=0x7f83539fb180) at ../../Source/WTF/wtf/RunLoop.cpp:104 #7 0x00007f836ab7ee3a in WTF::RunLoop::RunLoop()::{lambda(void*)#1}::operator()(void*) const () at ../../Source/WTF/wtf/glib/RunLoopGLib.cpp:66 #8 0x00007f836ab7ee5f in WTF::RunLoop::RunLoop()::{lambda(void*)#1}::_FUN(void*) () at ../../Source/WTF/wtf/glib/RunLoopGLib.cpp:68 #9 0x00007f836ab7edda in WTF::<lambda(GSource*, GSourceFunc, gpointer)>::operator()(GSource *, GSourceFunc, gpointer) const (__closure=0x0, source=0x2bdde90, callback=0x7f836ab7ee42 <WTF::RunLoop::RunLoop()::{lambda(void*)#1}::_FUN(void*)>, userData=0x7f83539fb180) at ../../Source/WTF/wtf/glib/RunLoopGLib.cpp:44 #10 0x00007f836ab7ee09 in WTF::<lambda(GSource*, GSourceFunc, gpointer)>::_FUN(GSource *, GSourceFunc, gpointer) () at ../../Source/WTF/wtf/glib/RunLoopGLib.cpp:45 #11 0x00007f83667b8afa in g_main_dispatch (context=0x245cfb0) at /home/phil/WebKit/WebKitBuild/DependenciesGTK/Source/glib-2.46.0/glib/gmain.c:3154 #12 g_main_context_dispatch (context=context@entry=0x245cfb0) at /home/phil/WebKit/WebKitBuild/DependenciesGTK/Source/glib-2.46.0/glib/gmain.c:3769 #13 0x00007f83667b8e78 in g_main_context_iterate (context=0x245cfb0, block=block@entry=1, dispatch=dispatch@entry=1, self=<optimized out>) at /home/phil/WebKit/WebKitBuild/DependenciesGTK/Source/glib-2.46.0/glib/gmain.c:3840 #14 0x00007f83667b9192 in g_main_loop_run (loop=0x2bcbd70) at /home/phil/WebKit/WebKitBuild/DependenciesGTK/Source/glib-2.46.0/glib/gmain.c:4034 #15 0x00007f836ab7f3d2 in WTF::RunLoop::run () at ../../Source/WTF/wtf/glib/RunLoopGLib.cpp:94 #16 0x00007f8370d95517 in WebKit::ChildProcessMain<WebKit::WebProcess, WebKit::WebProcessMain> (argc=2, argv=0x7ffe29c5b3c8) at ../../Source/WebKit2/Shared/unix/ChildProcessMain.h:61 #17 0x00007f8370d9537e in WebKit::WebProcessMainUnix (argc=2, argv=0x7ffe29c5b3c8) at ../../Source/WebKit2/WebProcess/gtk/WebProcessMainGtk.cpp:77 #18 0x0000000000400c5a in main (argc=2, argv=0x7ffe29c5b3c8) at ../../Source/WebKit2/WebProcess/EntryPoint/unix/WebProcessMain.cpp:44 1. Start the wk http server 2. MiniBrowser http://localhost:8000/media/hls/hls-audio-tracks.html

Attachments
Full backtrace (599.79 KB, text/plain) 2017-02-11 12:08 PST, Michael Catanzaro	no flags	Details
Patch (19.25 KB, patch) 2017-06-16 00:28 PDT, Carlos Garcia Campos	calvaris: review+	Details Formatted Diff Diff
View All Add attachment proposed patch, testcase, etc.

Carlos Garcia Campos

Comment 1 2015-12-09 03:53:21 PST

I don't think this is a bug in the notifier, it's the caller using the notifier to send tasks to the main thread while it was not created in the main thread. So, I think we shouldn't create WebKitWebSourceGStreamer objects in secondary threads.

Martin Robinson

Comment 2 2015-12-09 05:12:33 PST

Does this depend on a new version of GStreamer to be fixed? Is there any way to get it working in a backward compatible way?

Philippe Normand

Comment 3 2015-12-09 05:23:46 PST

It works until gst 1.4.x but in 1.6.x it breaks for adaptive streaming, like mentioned in quite a few bugs already... :( The best long-term solution I can think of is to properly fix bug 108088.

Carlos Alberto Lopez Perez

Comment 4 2016-02-01 16:02:09 PST

All http/tests/media/hls/* tests are failing on the GTK+ bots. I have updated the expectations on http://trac.webkit.org/changeset/195986. On a Debug build the following assert is triggered: ASSERTION FAILED: m_boundThread == currentThread() ../../Source/WTF/wtf/WeakPtr.h(55) : T *WTF::WeakReference<WebCore::MainThreadNotifier<MainThreadSourceNotification> >::get() const [T = WebCore::MainThreadNotifier<MainThreadSourceNotification>] 1 0x7fa2fb607570 /home/clopez/webkit/webkit/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(WTFCrash+0x20) [0x7fa2fb607570] 2 0x7fa3024c35ae /home/clopez/webkit/webkit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(_ZNK3WTF13WeakReferenceIN7WebCore18MainThreadNotifierI28MainThreadSourceNotificationEEE3getEv+0x4e) [0x7fa3024c35ae] 3 0x7fa3024c34ad /home/clopez/webkit/webkit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(_ZNK3WTF7WeakPtrIN7WebCore18MainThreadNotifierI28MainThreadSourceNotificationEEEcvbEv+0x1d) [0x7fa3024c34ad] 4 0x7fa3024be059 /home/clopez/webkit/webkit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(+0x6115059) [0x7fa3024be059] 5 0x7fa3024bddbd /home/clopez/webkit/webkit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(+0x6114dbd) [0x7fa3024bddbd] 6 0x7fa30033d83e /home/clopez/webkit/webkit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(_ZNKSt8functionIFvvEEclEv+0x3e) [0x7fa30033d83e] 7 0x7fa2fb62732a /home/clopez/webkit/webkit/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(_ZN3WTF7RunLoop11performWorkEv+0x13a) [0x7fa2fb62732a] 8 0x7fa2fb66c48c /home/clopez/webkit/webkit/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(+0x1aa848c) [0x7fa2fb66c48c] 9 0x7fa2fb66c468 /home/clopez/webkit/webkit/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(+0x1aa8468) [0x7fa2fb66c468] 10 0x7fa2fb66c523 /home/clopez/webkit/webkit/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(+0x1aa8523) [0x7fa2fb66c523] 11 0x7fa2fb66c4c8 /home/clopez/webkit/webkit/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(+0x1aa84c8) [0x7fa2fb66c4c8] 12 0x7fa2f82eaecd /home/clopez/webkit/webkit/WebKitBuild/DependenciesGTK/Root/lib/libglib-2.0.so.0(g_main_context_dispatch+0x13d) [0x7fa2f82eaecd] 13 0x7fa2f82eb268 /home/clopez/webkit/webkit/WebKitBuild/DependenciesGTK/Root/lib/libglib-2.0.so.0(+0x49268) [0x7fa2f82eb268] 14 0x7fa2f82eb582 /home/clopez/webkit/webkit/WebKitBuild/DependenciesGTK/Root/lib/libglib-2.0.so.0(g_main_loop_run+0xc2) [0x7fa2f82eb582] 15 0x7fa2fb66bd78 /home/clopez/webkit/webkit/WebKitBuild/Debug/lib/libjavascriptcoregtk-4.0.so.18(_ZN3WTF7RunLoop3runEv+0xb8) [0x7fa2fb66bd78] 16 0x7fa300a1da7d /home/clopez/webkit/webkit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(_ZN6WebKit16ChildProcessMainINS_10WebProcessENS_14WebProcessMainEEEiiPPc+0xfd) [0x7fa300a1da7d] 17 0x7fa300a1d96b /home/clopez/webkit/webkit/WebKitBuild/Debug/lib/libwebkit2gtk-4.0.so.37(WebProcessMainUnix+0x1b) [0x7fa300a1d96b] 18 0x400d66 /home/clopez/webkit/webkit/WebKitBuild/Debug/bin/WebKitWebProcess(main+0x46) [0x400d66] 19 0x7fa2f4302b45 /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf5) [0x7fa2f4302b45] 20 0x400c49 /home/clopez/webkit/webkit/WebKitBuild/Debug/bin/WebKitWebProcess() [0x400c49]

Zan Dobersek

Comment 5 2016-02-02 05:44:28 PST

TIL WeakPtr doesn't support cross-thread usage.

Carlos Garcia Campos

Comment 6 2016-02-02 06:02:44 PST

I still think it's the caller that is creating a WebKitWebSourceGStreamer in a secondary thread the one to be fixed.

Philippe Normand

Comment 7 2016-02-02 06:09:59 PST

(In reply to comment #6) > I still think it's the caller that is creating a WebKitWebSourceGStreamer in > a secondary thread the one to be fixed. Sadly this is beyond WebKit's scope, I'm afraid. For the time being, would it be possible to make WebKitWebSourceGStreamer behave in non-main threads?

Carlos Garcia Campos

Comment 8 2016-02-26 04:46:49 PST

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

Carlos Garcia Campos

Comment 9 2016-06-27 08:46:17 PDT

Fix for bug #144040 should fix this too.

Michael Catanzaro

Comment 10 2017-02-11 11:58:42 PST

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

Michael Catanzaro

Comment 11 2017-02-11 12:04:59 PST

(In reply to comment #9) > Fix for bug #144040 should fix this too. Nope. With today's trunk, I hit this crash almost every time I close a tab containing any article on http://www.vox.com, and every time I load any article on http://www.riverfronttimes.com/.

Michael Catanzaro

Comment 12 2017-02-11 12:08:25 PST

Created attachment 301265 [details] Full backtrace

Michael Catanzaro

Comment 13 2017-02-11 12:15:30 PST

Actually it looks like the crash in the first post is different than the one in comment #4. Oh well.

Michael Catanzaro

Comment 14 2017-02-11 12:22:13 PST

I don't think MainThreadNotifier should be using a WeakPtrFactory at all. Currently if the MainThreadNotifier is destroyed after (or during) a call to MainThreadNotifier::notify() but before the callback dispatches on the main thread, the callback will be implicitly canceled, even if the caller never called MainThreadNotifier::cancelPendingNotifications. That seems like fertile ground for subtle bugs. Shouldn't it be using a protector RefPtr to keep itself alive until the callback is dispatched, rather than a weak pointer? If the callback would be unsafe to dispatch after the destruction of the MainThreadNotifier, then it should be canceled explicitly, right? Note that here removePendingNotification() will return false if the notification has been explicitly canceled, it's kinda subtle: RunLoop::main().dispatch([weakThis, notificationType, callback] { if (weakThis && weakThis->removePendingNotification(notificationType)) callback(); });

Michael Catanzaro

Comment 15 2017-02-16 09:19:44 PST

This should be easy to reproduce if you have a debug build: (In reply to comment #11) > Nope. With today's trunk, I hit this crash almost every time I close a tab > containing any article on http://www.vox.com, and every time I load any > article on http://www.riverfronttimes.com/.

Xabier Rodríguez Calvar

Comment 16 2017-02-17 09:47:07 PST

I managed to reproduce the bug, I still have to fix it and be lucky if gdb does not crash on me with it

Enrique Ocaña

Comment 17 2017-02-24 08:45:07 PST

(In reply to comment #6) > I still think it's the caller that is creating a WebKitWebSourceGStreamer in > a secondary thread the one to be fixed. The creation of WebKitWebSourceGStreamer isn't under the control of the application. That GstElement is created automatically by GStreamer during the pipeline negotiation, and that can happen on a streaming (ie: non-main) thread depending on the pipeline topology.

Carlos Garcia Campos

Comment 18 2017-02-25 00:29:35 PST

(In reply to comment #17) > (In reply to comment #6) > > I still think it's the caller that is creating a WebKitWebSourceGStreamer in > > a secondary thread the one to be fixed. > > The creation of WebKitWebSourceGStreamer isn't under the control of the > application. That GstElement is created automatically by GStreamer during > the pipeline negotiation, and that can happen on a streaming (ie: non-main) > thread depending on the pipeline topology. And we detect it now, so It's still the caller that creates the object in a secondary the thread the one who should delete it in the same thread.

Xabier Rodríguez Calvar

Comment 19 2017-02-28 01:51:21 PST

(In reply to comment #18) > > The creation of WebKitWebSourceGStreamer isn't under the control of the > > application. That GstElement is created automatically by GStreamer during > > the pipeline negotiation, and that can happen on a streaming (ie: non-main) > > thread depending on the pipeline topology. > > And we detect it now, so It's still the caller that creates the object in a > secondary the thread the one who should delete it in the same thread. We are facing here two requirements colluding. First is that GStreamer has the right to create and destroy objects in the threads they see fit, which is what is happening. Second is the requirement of WeakPtrFactory of begin created and destroyed from the same thread. We'll think about the proper fix.

Michael Catanzaro

Comment 20 2017-02-28 11:39:27 PST

(In reply to comment #19) > We are facing here two requirements colluding. First is that GStreamer has > the right to create and destroy objects in the threads they see fit, which > is what is happening. It seems very unexpected to me that the object can be destroyed in a thread other than the thread in which it is created. That's almost sure to cause thread-safety bugs. Is that behavior documented with a very big warning? It sounds to me like this is a GStreamer bug.

Xabier Rodríguez Calvar

Comment 21 2017-03-01 00:40:14 PST

(In reply to comment #20) > (In reply to comment #19) > > We are facing here two requirements colluding. First is that GStreamer has > > the right to create and destroy objects in the threads they see fit, which > > is what is happening. > > It seems very unexpected to me that the object can be destroyed in a thread > other than the thread in which it is created. That's almost sure to cause > thread-safety bugs. Is that behavior documented with a very big warning? It > sounds to me like this is a GStreamer bug. This is a GStreamer element and GStreamer elements, specially the ones created by GStreamer itself are supposed to be thread safe.

Michael Catanzaro

Comment 22 2017-03-01 06:44:34 PST

Can we get rid of the WeakPtrFactory, then? (In reply to comment #14) > I don't think MainThreadNotifier should be using a WeakPtrFactory at all. > Currently if the MainThreadNotifier is destroyed after (or during) a call to > MainThreadNotifier::notify() but before the callback dispatches on the main > thread, the callback will be implicitly canceled, even if the caller never > called MainThreadNotifier::cancelPendingNotifications. That seems like > fertile ground for subtle bugs. Shouldn't it be using a protector RefPtr to > keep itself alive until the callback is dispatched, rather than a weak > pointer? If the callback would be unsafe to dispatch after the destruction > of the MainThreadNotifier, then it should be canceled explicitly, right? > > Note that here removePendingNotification() will return false if the > notification has been explicitly canceled, it's kinda subtle: > > RunLoop::main().dispatch([weakThis, notificationType, callback] { > if (weakThis && weakThis->removePendingNotification(notificationType)) > callback(); > });

Carlos Garcia Campos

Comment 23 2017-06-15 23:40:02 PDT

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

Carlos Garcia Campos

Comment 24 2017-06-16 00:28:25 PDT

Created attachment 313058 [details] Patch

Build Bot

Comment 25 2017-06-16 00:30:50 PDT

Attachment 313058 [details] did not pass style-queue: ERROR: Source/WebCore/platform/graphics/gstreamer/TrackPrivateBaseGStreamer.cpp:82: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/TrackPrivateBaseGStreamer.cpp:103: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:471: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:520: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamerBase.cpp:646: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:608: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:662: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:673: More than one command on the same line [whitespace/newline] [4] ERROR: Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:726: More than one command on the same line [whitespace/newline] [4] Total errors found: 9 in 9 files If any of these errors are false positives, please file a bug against check-webkit-style.

Xabier Rodríguez Calvar

Comment 26 2017-06-16 03:21:09 PDT

Comment on attachment 313058 [details] Patch The style issues are ok so feel free to fix them or leave them.

Michael Catanzaro

Comment 27 2017-06-16 07:32:49 PDT

Comment on attachment 313058 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=313058&action=review > Source/WebCore/platform/graphics/gstreamer/MainThreadNotifier.h:55 > + RunLoop::main().dispatch([this, protectedThis = makeRef(*this), notificationType, callback = std::function<void()>(callbackFunctor)] { > + if (!m_isValid.load()) > + return; > + if (removePendingNotification(notificationType)) > callback(); > }); So now there are two cases when the callback might never be called. It's standard practice in such cases for the callback to be called with a cancellation indicator (e.g. G_IO_ERROR_CANCELLED), but we just drop it. This makes MainThreadNotifier unsafe to use in any case where execution of the callback is required. It can only be used for optional tasks. This is a preexisting problem, but it this patch exacerbates it slightly, and it seems like the sort of thing that could cause many difficult-to-debug issues.

Carlos Garcia Campos

Comment 28 2017-06-16 07:41:49 PDT

(In reply to Michael Catanzaro from comment #27) > Comment on attachment 313058 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=313058&action=review > > > Source/WebCore/platform/graphics/gstreamer/MainThreadNotifier.h:55 > > + RunLoop::main().dispatch([this, protectedThis = makeRef(*this), notificationType, callback = std::function<void()>(callbackFunctor)] { > > + if (!m_isValid.load()) > > + return; > > + if (removePendingNotification(notificationType)) > > callback(); > > }); > > So now there are two cases when the callback might never be called. It's > standard practice in such cases for the callback to be called with a > cancellation indicator (e.g. G_IO_ERROR_CANCELLED), but we just drop it. > This makes MainThreadNotifier unsafe to use in any case where execution of > the callback is required. It can only be used for optional tasks. This is a > preexisting problem, but it this patch exacerbates it slightly, and it seems > like the sort of thing that could cause many difficult-to-debug issues. The same two cases than before, I've only split the if in two. Anyway, this is not a completion callback, and the notifier is only expected to be invalidated when callers die, to behave the same way than the weakptr, so we never want to run any callback after the caller has died, since that's indeed the whole point of the weak pointer.

Carlos Garcia Campos

Comment 29 2017-06-16 07:43:59 PDT

That's also why I added the asserts to the public methods, btw, you are not supposed to use the notifier after invalidating it, because your are supposed to die.

Michael Catanzaro

Comment 30 2017-06-16 14:07:49 PDT

OK. I would add comments to clarify this behavior. It is unexpected to me.

Carlos Garcia Campos

Comment 31 2017-06-18 22:12:52 PDT

Committed r218471: <http://trac.webkit.org/changeset/218471>

Note You need to log in before you can comment on or make changes to this bug.