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

• Component: Platform
• Status: RESOLVED FIXED

[Philippe Normand]

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

[Carlos Garcia Campos]

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]

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]

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]

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]

TIL WeakPtr doesn't support cross-thread usage.

[Carlos Garcia Campos]

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

[Philippe Normand]

(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]

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

[Carlos Garcia Campos]

Fix for bug #144040 should fix this too.

[Michael Catanzaro]

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

[Michael Catanzaro]

(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]

Created attachment 301265 [details]
Full backtrace

[Michael Catanzaro]

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

[Michael Catanzaro]

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]

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]

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]

(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]

(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]

(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]

(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]

(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]

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]

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

[Carlos Garcia Campos]

Created attachment 313058 [details]
Patch

[Build Bot]

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 on attachment 313058 [details]
Patch

The style issues are ok so feel free to fix them or leave them.

[Michael Catanzaro]

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]

(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]

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]

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

[Carlos Garcia Campos]

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