Bug 152043

Summary: [GStreamer] MainThreadNotifier ASSERTION FAILED: m_boundThread == currentThread() in _WebKitWebSrcPrivate::~_WebKitWebSrcPrivate
Product: WebKit Reporter: Philippe Normand <pnormand>
Component: PlatformAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: aplazas, bugs-noreply, buildbot, calvaris, cgarcia, clopez, eocanha, ht990332, mcatanzaro, mrobinson, zan
Priority: P2    
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+

Description Philippe Normand 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
Comment 1 Carlos Garcia Campos 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.
Comment 2 Martin Robinson 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?
Comment 3 Philippe Normand 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.
Comment 4 Carlos Alberto Lopez Perez 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]
Comment 5 Zan Dobersek 2016-02-02 05:44:28 PST
TIL WeakPtr doesn't support cross-thread usage.
Comment 6 Carlos Garcia Campos 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.
Comment 7 Philippe Normand 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?
Comment 8 Carlos Garcia Campos 2016-02-26 04:46:49 PST
*** Bug 154723 has been marked as a duplicate of this bug. ***
Comment 9 Carlos Garcia Campos 2016-06-27 08:46:17 PDT
Fix for bug #144040 should fix this too.
Comment 10 Michael Catanzaro 2017-02-11 11:58:42 PST
*** Bug 167946 has been marked as a duplicate of this bug. ***
Comment 11 Michael Catanzaro 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/.
Comment 12 Michael Catanzaro 2017-02-11 12:08:25 PST
Created attachment 301265 [details]
Full backtrace
Comment 13 Michael Catanzaro 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.
Comment 14 Michael Catanzaro 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();
});
Comment 15 Michael Catanzaro 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/.
Comment 16 Xabier Rodríguez Calvar 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
Comment 17 Enrique Ocaña 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.
Comment 18 Carlos Garcia Campos 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.
Comment 19 Xabier Rodríguez Calvar 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.
Comment 20 Michael Catanzaro 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.
Comment 21 Xabier Rodríguez Calvar 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.
Comment 22 Michael Catanzaro 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();
> });
Comment 23 Carlos Garcia Campos 2017-06-15 23:40:02 PDT
*** Bug 173418 has been marked as a duplicate of this bug. ***
Comment 24 Carlos Garcia Campos 2017-06-16 00:28:25 PDT
Created attachment 313058 [details]
Patch
Comment 25 Build Bot 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.
Comment 26 Xabier Rodríguez Calvar 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.
Comment 27 Michael Catanzaro 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.
Comment 28 Carlos Garcia Campos 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.
Comment 29 Carlos Garcia Campos 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.
Comment 30 Michael Catanzaro 2017-06-16 14:07:49 PDT
OK. I would add comments to clarify this behavior. It is unexpected to me.
Comment 31 Carlos Garcia Campos 2017-06-18 22:12:52 PDT
Committed r218471: <http://trac.webkit.org/changeset/218471>