WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(8.31 KB, patch)
2017-02-24 05:07 PST
,
Xabier Rodríguez Calvar
no flags
Details
Formatted Diff
Diff
Patch
(8.47 KB, patch)
2017-09-04 04:55 PDT
,
Xabier Rodríguez Calvar
no flags
Details
Formatted Diff
Diff
Test WIP
(169.67 KB, patch)
2017-09-07 00:34 PDT
,
Xabier Rodríguez Calvar
no flags
Details
Formatted Diff
Diff
Patch
(8.49 KB, patch)
2018-02-18 23:13 PST
,
Philippe Normand
no flags
Details
Formatted Diff
Diff
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
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
Created
attachment 301946
[details]
Patch
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
Created
attachment 302668
[details]
Patch
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
Created
attachment 319847
[details]
Patch
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
Created
attachment 334138
[details]
Patch
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
<
rdar://problem/37665604
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug