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: | 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
Philippe Normand
2015-12-09 01:27:22 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. 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? 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. 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] TIL WeakPtr doesn't support cross-thread usage. I still think it's the caller that is creating a WebKitWebSourceGStreamer in a secondary thread the one to be fixed. (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? *** Bug 154723 has been marked as a duplicate of this bug. *** Fix for bug #144040 should fix this too. *** Bug 167946 has been marked as a duplicate of this bug. *** (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/. Created attachment 301265 [details]
Full backtrace
Actually it looks like the crash in the first post is different than the one in comment #4. Oh well. 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(); }); 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/. I managed to reproduce the bug, I still have to fix it and be lucky if gdb does not crash on me with it (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. (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. (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. (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. (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. 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(); > }); *** Bug 173418 has been marked as a duplicate of this bug. *** Created attachment 313058 [details]
Patch
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 on attachment 313058 [details]
Patch
The style issues are ok so feel free to fix them or leave them.
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. (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. 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. OK. I would add comments to clarify this behavior. It is unexpected to me. Committed r218471: <http://trac.webkit.org/changeset/218471> |