Bug 166733 - [GStreamer] Crash in WebCore::MediaPlayerRequestInstallMissingPluginsCallback::complete
Summary: [GStreamer] Crash in WebCore::MediaPlayerRequestInstallMissingPluginsCallback...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: PC Linux
: P2 Normal
Assignee: Xabier Rodríguez Calvar
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-01-05 10:09 PST by Michael Catanzaro
Modified: 2018-02-19 02:48 PST (History)
9 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 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}
Comment 1 Xabier Rodríguez Calvar 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).
Comment 2 Zan Dobersek 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.
Comment 3 Xabier Rodríguez Calvar 2017-02-17 09:45:12 PST
Created attachment 301946 [details]
Patch
Comment 4 Zan Dobersek 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>>?
Comment 5 Michael Catanzaro 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.
Comment 6 Xabier Rodríguez Calvar 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.
Comment 7 Michael Catanzaro 2017-02-20 08:49:11 PST
It's expected. Please move it to INFO!
Comment 8 Xabier Rodríguez Calvar 2017-02-24 05:07:15 PST
Created attachment 302668 [details]
Patch
Comment 9 Xabier Rodríguez Calvar 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==
Comment 10 Michael Catanzaro 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.
Comment 11 Michael Catanzaro 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?
Comment 12 Michael Catanzaro 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.
Comment 13 Xabier Rodríguez Calvar 2017-09-04 04:55:31 PDT
Created attachment 319847 [details]
Patch
Comment 14 Xabier Rodríguez Calvar 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 :)
Comment 15 Carlos Garcia Campos 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.
Comment 16 Xabier Rodríguez Calvar 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.
Comment 17 Xabier Rodríguez Calvar 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.
Comment 18 Philippe Normand 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?
Comment 19 Xabier Rodríguez Calvar 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.
Comment 20 Philippe Normand 2018-02-18 23:13:40 PST
Created attachment 334138 [details]
Patch
Comment 21 WebKit Commit Bot 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
Comment 22 WebKit Commit Bot 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>
Comment 23 WebKit Commit Bot 2018-02-19 02:47:56 PST
All reviewed patches have been landed.  Closing bug.
Comment 24 Radar WebKit Bug Importer 2018-02-19 02:48:33 PST
<rdar://problem/37665604>