Bug 166733 – [GStreamer] Crash in WebCore::MediaPlayerRequestInstallMissingPluginsCallback::complete

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Michael Catanzaro]

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}

[Xabier Rodríguez Calvar]

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]

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]

Created attachment 301946 [details]
Patch

[Zan Dobersek]

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

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

It's expected. Please move it to INFO!

[Xabier Rodríguez Calvar]

Created attachment 302668 [details]
Patch

[Xabier Rodríguez Calvar]

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

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]

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]

Created attachment 319847 [details]
Patch

[Xabier Rodríguez Calvar]

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

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

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]

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]

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

Created attachment 334138 [details]
Patch

[WebKit Commit Bot]

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

Clearing flags on attachment: 334138

Committed r228639: <https://trac.webkit.org/changeset/228639>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/37665604>