RESOLVED FIXED 166733
[GStreamer] Crash in WebCore::MediaPlayerRequestInstallMissingPluginsCallback::complete
https://bugs.webkit.org/show_bug.cgi?id=166733
Summary [GStreamer] Crash in WebCore::MediaPlayerRequestInstallMissingPluginsCallback...
Michael Catanzaro
Reported 2017-01-05 10:09:13 PST
Crash in WebCore::MediaPlayerRequestInstallMissingPluginsCallback::complete. We have 43 reports of it in Fedora. Downstream reporter says: "This and a few other crashes seems to happen when loading video to be streamed." I asked if it is reproducible. Reporter says: "Not really. It has happened twice in different websites but trying to reproduce them it does not crash." It's odd because the codec installer should be totally broken and not running at all due to bug #147822. Full backtrace on the downstream bug. Here's the head: Thread 1 (Thread 0x7fec8c735fc0 (LWP 4034)): #0 0x00007fec8b44f544 in WTF::RefCountedBase::derefBase() const (this=0x40000000200) at /usr/src/debug/webkitgtk-2.14.2/Source/WTF/wtf/RefCounted.h:99 #1 0x00007fec8b44f544 in WTF::RefCounted<WebCore::MediaPlayerRequestInstallMissingPluginsCallback>::deref() const (this=0x40000000200) at /usr/src/debug/webkitgtk-2.14.2/Source/WTF/wtf/RefCounted.h:144 this = 0x7febd9854480 #2 0x00007fec8b44f544 in WTF::derefIfNotNull<WebCore::MediaPlayerRequestInstallMissingPluginsCallback>(WebCore::MediaPlayerRequestInstallMissingPluginsCallback*) (ptr=<optimized out>) at /usr/src/debug/webkitgtk-2.14.2/Source/WTF/wtf/PassRefPtr.h:40 this = 0x7febd9854480 #3 0x00007fec8b44f544 in WTF::RefPtr<WebCore::MediaPlayerRequestInstallMissingPluginsCallback>::operator=(decltype(nullptr)) (this=0x7febd9854678) at /usr/src/debug/webkitgtk-2.14.2/Source/WTF/wtf/RefPtr.h:150 this = 0x7febd9854480 #4 0x00007fec8b44f544 in WebCore::MediaPlayerPrivateGStreamer::<lambda(uint32_t)>::operator() (result=4, __closure=0x7febd8109a80) at /usr/src/debug/webkitgtk-2.14.2/Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1002 this = 0x7febd9854480 #5 0x00007fec8b44f544 in std::_Function_handler<void(unsigned int), WebCore::MediaPlayerPrivateGStreamer::handleMessage(GstMessage*)::<lambda(uint32_t)> >::_M_invoke(const std::_Any_data &, <unknown type in /var/cache/abrt-di/usr/lib/debug/usr/lib64/libwebkit2gtk-4.0.so.37.14.9.debug, CU 0x479a25f7, DIE 0x47a047a5>) (__functor=..., __args#0=<unknown type in /var/cache/abrt-di/usr/lib/debug/usr/lib64/libwebkit2gtk-4.0.so.37.14.9.debug, CU 0x479a25f7, DIE 0x47a047a5>) at /usr/include/c++/6.2.1/functional:1740 #6 0x00007fec8a6a9069 in std::function<void (unsigned int)>::operator()(unsigned int) const (__args#0=<optimized out>, this=0x7febd8109a80) at /usr/include/c++/6.2.1/functional:2136 #7 0x00007fec8a6a9069 in WebCore::MediaPlayerRequestInstallMissingPluginsCallback::complete(unsigned int) (result=<optimized out>, this=0x7febd8109a78) at /usr/src/debug/webkitgtk-2.14.2/Source/WebCore/platform/graphics/gstreamer/MediaPlayerRequestInstallMissingPluginsCallback.h:45 #8 0x00007fec8a6a9069 in WebKit::WebPage::didEndRequestInstallMissingMediaPlugins(unsigned int) (this=0x7fec74dce000, result=<optimized out>) at /usr/src/debug/webkitgtk-2.14.2/Source/WebKit2/WebProcess/WebPage/gstreamer/WebPageGStreamer.cpp:53 #9 0x00007fec8a709d4e in IPC::callMemberFunctionImpl<WebKit::WebPage, void (WebKit::WebPage::*)(unsigned int), std::tuple<unsigned int>, 0ul>(WebKit::WebPage*, void (WebKit::WebPage::*)(unsigned int), std::tuple<unsigned int>&&, std::integer_sequence<unsigned long, 0ul>) (args=<optimized out>, function=<optimized out>, object=0x7fec74dce000) at /usr/src/debug/webkitgtk-2.14.2/Source/WebKit2/Platform/IPC/HandleMessage.h:13 arguments = std::tuple containing = {[1] = 4} #10 0x00007fec8a709d4e in IPC::callMemberFunction<WebKit::WebPage, void (WebKit::WebPage::*)(unsigned int), std::tuple<unsigned int>, std::integer_sequence<unsigned long, 0ul> >(std::tuple<unsigned int>&&, WebKit::WebPage*, void (WebKit::WebPage::*)(unsigned int)) (function=<optimized out>, object=0x7fec74dce000, args=<unknown type in /var/cache/abrt-di/usr/lib/debug/usr/lib64/libwebkit2gtk-4.0.so.37.14.9.debug, CU 0xf494260, DIE 0xf5131cf>) at /usr/src/debug/webkitgtk-2.14.2/Source/WebKit2/Platform/IPC/HandleMessage.h:19 arguments = std::tuple containing = {[1] = 4}
Attachments
Patch (8.24 KB, patch)
2017-02-17 09:45 PST, Xabier Rodríguez Calvar
no flags
Patch (8.31 KB, patch)
2017-02-24 05:07 PST, Xabier Rodríguez Calvar
no flags
Patch (8.47 KB, patch)
2017-09-04 04:55 PDT, Xabier Rodríguez Calvar
no flags
Test WIP (169.67 KB, patch)
2017-09-07 00:34 PDT, Xabier Rodríguez Calvar
no flags
Patch (8.49 KB, patch)
2018-02-18 23:13 PST, Philippe Normand
no flags
Xabier Rodríguez Calvar
Comment 1 2017-02-10 08:51:57 PST
I tried to reproduce this bug by forcing the conditions but I couldn't so far. By reading the backtrace it seems that the object that is being dereferenced is not valid but I don't know if it is like that because the the object is destroyed by a function that it owns or because it was not valid (maybe a double missing plugin request).
Zan Dobersek
Comment 2 2017-02-10 09:33:24 PST
Theory, assuming one MediaPlayerPrivateGStreamer instance and two missing plugin messages on its pipeline (which I assume can occur under some circumstances, like missing plugins for both the video and audio tracks, or two different files being loaded through the player): 1. When the first message arrives, the first MediaPlayerRequestInstallMissingPluginsCallback is created, stored in m_missingPluginsCallback. Gets relayed to WebPage::requestInstallMissingPlugins(), where the second reference gets stored, and the request gets relayed to WebPageProxy. 2. When the second message arrives, the second MediaPlayerRequestInstallMissingPluginsCallback object is created, overriding the reference to the first one in m_missingPluginsCallback. The second object gets relayed to WebPage::requestInstallMissingPlugins(), where the callback is immediately dispatched. 3. In the callback, the m_missingPluginsCallback RefPtr<> gets nulled out, meaning the second MediaPlayerRequestInstallMissingPluginsCallback is now destroyed. That means the first MediaPlayerRequestInstallMissingPluginsCallback object only has one reference left, the one that's held in the WebPage object. 4. The MediaPlayerPrivateGStreamer object gets destroyed. The obvious problem is that while it would preferably invalidate the first MediaPlayerRequestInstallMissingPluginsCallback object, it can't, because the creation of the second one dropped the reference to the first one. So MediaPlayerPrivateGStreamer is destroyed, but MediaPlayerRequestInstallMissingPluginsCallback lives on without being invalidated. 5. WebPageProxy responds, invoking WebPage::didEndRequestInstallMissingMediaPlugins(). The first MediaPlayerRequestInstallMissingPluginsCallback object is still there in a valid state, but with the stored std::function<> now capturing a pointer to the MediaPlayerPrivateGStreamer object that was already freed. So the captured lambda gets invoked, and because that lambda clears out the m_missingPluginsCallback member variable on an already-freed MediaPlayerPrivateGStreamer, things explode.
Xabier Rodríguez Calvar
Comment 3 2017-02-17 09:45:12 PST
Zan Dobersek
Comment 4 2017-02-17 10:46:37 PST
Comment on attachment 301946 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=301946&action=review > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1044 > + RefPtr<MediaPlayerRequestInstallMissingPluginsCallback> missingPluginsCallback = MediaPlayerRequestInstallMissingPluginsCallback::create([weakThis = createWeakPtr()](uint32_t result, const RefPtr<MediaPlayerRequestInstallMissingPluginsCallback>& missingPluginsCallback) { Can the second parameter here be just simply `const MediaPlayerRequestInstallMissingPluginsCallback&`? > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h:252 > + Vector<RefPtr<MediaPlayerRequestInstallMissingPluginsCallback>> m_missingPluginsCallbacks; And can this be a Vector<Ref<MediaPlayerRequestInstallMissingPluginsCallback>>?
Michael Catanzaro
Comment 5 2017-02-17 11:51:30 PST
Comment on attachment 301946 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=301946&action=review > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1046 > + GST_WARNING("got missing pluging installation callback in destroyed player with result %u", result); Is it really worth a warning-level message? It's expected that responses can come after the player is destroyed.
Xabier Rodríguez Calvar
Comment 6 2017-02-20 01:36:24 PST
(In reply to comment #5) > Comment on attachment 301946 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=301946&action=review > > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1046 > > + GST_WARNING("got missing pluging installation callback in destroyed player with result %u", result); > > Is it really worth a warning-level message? It's expected that responses can > come after the player is destroyed. It depends on how expected it is. If it is very expected then I'd add a INFO or DEBUG, if it's expected only sometimes I'd leave the WARNING. Besides, having a warning here is ok because if this happens playback won't happen either so it is better to have this level IMHO.
Michael Catanzaro
Comment 7 2017-02-20 08:49:11 PST
It's expected. Please move it to INFO!
Xabier Rodríguez Calvar
Comment 8 2017-02-24 05:07:15 PST
Xabier Rodríguez Calvar
Comment 9 2017-02-24 06:43:50 PST
(In reply to comment #8) > Created attachment 302668 [details] > I could not convert the RefPtr to Ref because Vector does not have > operator== and compilation fails while with the removeFirst. I mean Ref does not have the operator==
Michael Catanzaro
Comment 10 2017-03-03 07:19:53 PST
Comment on attachment 302668 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=302668&action=review Just one question. > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1048 > + RefPtr<MediaPlayerRequestInstallMissingPluginsCallback> protectedMissingPluginCallback = &missingPluginCallback; I don't think you need to protect it here, because once you remove it from m_missingPluginCallbacks, you do not use it again. The reason you are putting it in a RefPtr is to make it possible to remove it from the vector, right? So I think protectedMissingPluginCallback is not the right name for this variable; it's confusing to call it that when protection is not needed.
Michael Catanzaro
Comment 11 2017-08-30 21:20:25 PDT
I have 122 reports of this crash. (In reply to Michael Catanzaro from comment #10) > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1048 > > + RefPtr<MediaPlayerRequestInstallMissingPluginsCallback> protectedMissingPluginCallback = &missingPluginCallback; > > I don't think you need to protect it here, because once you remove it from > m_missingPluginCallbacks, you do not use it again. The reason you are > putting it in a RefPtr is to make it possible to remove it from the vector, > right? So I think protectedMissingPluginCallback is not the right name for > this variable; it's confusing to call it that when protection is not needed. Calvaris, got time to update and land this patch?
Michael Catanzaro
Comment 12 2017-08-31 18:33:51 PDT
Coincidentally I hit this crash today trying to view https://www.youtube.com/watch?v=OvkL9GjQkoQ. Just click the play button a few times; it will crash sooner or later.
Xabier Rodríguez Calvar
Comment 13 2017-09-04 04:55:31 PDT
Xabier Rodríguez Calvar
Comment 14 2017-09-04 04:58:47 PDT
(In reply to Michael Catanzaro from comment #11) > > I don't think you need to protect it here, because once you remove it from > > m_missingPluginCallbacks, you do not use it again. The reason you are > > putting it in a RefPtr is to make it possible to remove it from the vector, > > right? So I think protectedMissingPluginCallback is not the right name for > > this variable; it's confusing to call it that when protection is not needed. The function I am running is being self deleted with this code. The callback is stored and when called it gets itself to be able to remove it from the vector. I am not completely sure of what can happen if you delete the function object you're just running, maybe nothing but I strongly prefer to protect it here. > Calvaris, got time to update and land this patch? You got lucky today :)
Carlos Garcia Campos
Comment 15 2017-09-04 05:26:49 PDT
Comment on attachment 319847 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=319847&action=review > Source/WebCore/ChangeLog:12 > + First is handling getting more than one missing plugin > + installation request at the same time. For this we add the request > + to a Vector and handle them there. If this can really happen, I think it belongs to a separate patch, or does the crash happen because of multiple requests? We should add a test for that then. > Source/WebCore/ChangeLog:16 > + Second is that if the player is dead and we still get the result, > + bad things happen. For that we "weaked" the pointer capture by the > + lambda. We have invalidate() for that, how can it be dead inside lambda? The destructor calls invalidate(). Or can they run in different threads? > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:-1034 > - m_missingPluginsCallback = MediaPlayerRequestInstallMissingPluginsCallback::create([this](uint32_t result) { > - m_missingPluginsCallback = nullptr; If the problem is that we delete the callback here, which I see it could be a problem, this could be fixed by protecting the callback before setting the member to nullptr.
Xabier Rodríguez Calvar
Comment 16 2017-09-04 07:12:16 PDT
(In reply to Carlos Garcia Campos from comment #15) > > Source/WebCore/ChangeLog:12 > > + First is handling getting more than one missing plugin > > + installation request at the same time. For this we add the request > > + to a Vector and handle them there. > > If this can really happen, I think it belongs to a separate patch, or does > the crash happen because of multiple requests? We should add a test for that > then. The crash happens only with two missing plugins, meaning two calls, so fixing this is part of fixing the crash. > > Source/WebCore/ChangeLog:16 > > + Second is that if the player is dead and we still get the result, > > + bad things happen. For that we "weaked" the pointer capture by the > > + lambda. > > We have invalidate() for that, how can it be dead inside lambda? The > destructor calls invalidate(). Or can they run in different threads? The problem is that it's the invalidating does not help here as the function that is running that is removing itself. Invalidating would be too late. > > Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:-1034 > > - m_missingPluginsCallback = MediaPlayerRequestInstallMissingPluginsCallback::create([this](uint32_t result) { > > - m_missingPluginsCallback = nullptr; > > If the problem is that we delete the callback here, which I see it could be > a problem, this could be fixed by protecting the callback before setting the > member to nullptr. The crash is caused by the two calls because the mediaplayer does not exist anymore when GStreamer calls the second callback.
Xabier Rodríguez Calvar
Comment 17 2017-09-07 00:34:18 PDT
Created attachment 320101 [details] Test WIP I have to switch to something else, I leave my WIP for the tests in this patch.
Philippe Normand
Comment 18 2018-02-09 05:15:59 PST
I think a layout test for this patch feels quite convoluted because the test expectation wouldn't reflect the codec installer output. Patch looks good though, so let's land it?
Xabier Rodríguez Calvar
Comment 19 2018-02-13 01:18:58 PST
(In reply to Philippe Normand from comment #18) > I think a layout test for this patch feels quite convoluted because the test > expectation wouldn't reflect the codec installer output. > > Patch looks good though, so let's land it? Agreed.
Philippe Normand
Comment 20 2018-02-18 23:13:40 PST
WebKit Commit Bot
Comment 21 2018-02-19 02:04:16 PST
Comment on attachment 334138 [details] Patch Rejecting attachment 334138 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 334138, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: rdparty/autoinstalled/mechanize/_urllib2_fork.py", line 332, in _call_chain result = func(*args) File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_urllib2_fork.py", line 1170, in https_open return self.do_open(conn_factory, req) File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_urllib2_fork.py", line 1118, in do_open raise URLError(err) urllib2.URLError: <urlopen error [Errno 60] Operation timed out> Full output: http://webkit-queues.webkit.org/results/6569918
WebKit Commit Bot
Comment 22 2018-02-19 02:47:54 PST
Comment on attachment 334138 [details] Patch Clearing flags on attachment: 334138 Committed r228639: <https://trac.webkit.org/changeset/228639>
WebKit Commit Bot
Comment 23 2018-02-19 02:47:56 PST
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 24 2018-02-19 02:48:33 PST
Note You need to log in before you can comment on or make changes to this bug.