Bug 144099 – [GStreamer] Crashes during plugin installation

• Component: Media
• Status: RESOLVED FIXED

[Michael Catanzaro]

WebKitGTK+ 2.8.1 crashes 100% of the time when attempting to play a YouTube video if an H.264 codec is not installed:

Truncated backtrace:
Thread no. 1 (7 frames)
 #0 gst_element_get_state at gstelement.c:2158
 #1 WebCore::MediaPlayerPrivateGStreamer::changePipelineState at /usr/src/debug/webkitgtk-2.8.0/Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:384
 #2 WebCore::MediaPlayerPrivateGStreamer::handlePluginInstallerResult at /usr/src/debug/webkitgtk-2.8.0/Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1067
 #3 gst_install_plugins_installer_exited at install-plugins.c:690
 #4 g_child_watch_dispatch at gmain.c:5177
 #9 WebKit::ChildProcessMain<WebKit::WebProcess, WebKit::WebProcessMain> at /usr/src/debug/webkitgtk-2.8.0/Source/WebKit2/Shared/unix/ChildProcessMain.h:61
 #11 _start

See the downstream bug for a full backtrace.

[Philippe Normand]

Would it be too much asking for a debug build trace? :)
Seems like the m_pipeline is somehow null?

[Michael Catanzaro]

Unfortunately that is indeed asking too much, because our debug builds have been very buggy for as long as I remember. They can't load youtube.com at all, bug #144115.

[Michael Catanzaro]

Created attachment 251703 [details]
backtrace

I have a debug build of r183311. It doesn't crash like 2.8.1, but hits an invalid (NULL) pointer instance critical that looks related.

[Carlos Garcia Campos]

Looking at the code, (In reply to comment #3)
> Created attachment 251703 [details]
> backtrace
> 
> I have a debug build of r183311. It doesn't crash like 2.8.1, but hits an
> invalid (NULL) pointer instance critical that looks related.

This looks unrelated to plugin installation.

[Carlos Garcia Campos]

The gst_install_plugins_async() is always started with a valid pipeline, so the only think I can think of is that the media player is deleted when the callback is called. The plugins async installation can't be cancelled (or I don't see any cancellable object, nor API to cancel it in the docs), so I guess the only option we have is taking a reference of the media player while the installation happen. In any case it would be nice if someone using fedora could confirm that the media player is indeed deleted before the callback is called.

[Michael Catanzaro]

Here is the order of some relevant function calls:

virtual GstElement *WebCore::MediaPlayerPrivateGStreamer::createAudioSink(): player=0x7f2a2f088000

(WebKitWebProcess:14021): GLib-GObject-WARNING **: invalid (NULL) pointer instance

(WebKitWebProcess:14021): GLib-GObject-CRITICAL **: g_signal_connect_data: assertion 'G_TYPE_CHECK_INSTANCE (instance)' failed

(WebKitWebProcess:14021): GStreamer-CRITICAL **: gst_element_link_pads_full: assertion 'GST_IS_ELEMENT (dest)' failed
gboolean WebCore::MediaPlayerPrivateGStreamer::handleMessage(GstMessage *): Beginning plugin installation, player=0x7f2a2f088000
virtual WebCore::MediaPlayerPrivateGStreamer::~MediaPlayerPrivateGStreamer(): player=0x7f2a2f088000

So you are right on both counts, the invalid (NULL) pointer instance warning is unrelated to the plugin installation crash, and the media player object is deleted prior to the plugin installation callback (though that does not cause any crash in trunk). I have filed bug #146999 for the invalid (NULL) pointer instance issue. Since the plugin installation crash doesn't occur in trunk, I will mark this bug as [Stable].

By the way, in 2.8 I expect you would be able to reproduce by uninstalling gstreamer1.0-libav.

[Carlos Garcia Campos]

I uninstalled gstreamer1.0-libav, but then videos are fully black and I only listen the audio. No errors on stdout nor missing plugins installation at all.

[Carlos Garcia Campos]

Ok, I didn't have gstreamer1.0-packagekit installed either. I think we could show the details in stdout when gst_install_plugins_async returns GST_INSTALL_PLUGINS_HELPER_MISSING.

[Carlos Garcia Campos]

This is not specific to stable, I can't reproduce the crashes neither with master nor with 2.8, but the problem is there anyway, the install plugins callback can be called for a deleted media player. I think the installation of the plugins should be moved to the UI process, to provide a proper launch context, and to ensure there can be two installers for the same application (gst_install_plugins_installation_in_progress() doesn't really work when using multiple web processes).

[Michael Catanzaro]

It needs to be done regardless to make plugin installation work with seccomp filters, see bug #146993. It used to be on my to-do list when I was working on sandboxing, but it's not anymore, since it would not be a small task.

Another thing we should clean up at the same time is bug #135973, to get rid of the automatic codec notifications (we should use in-window notifications instead, since the system notifications are only intended to be used as a fallback; that implies adding new public API).

[Carlos Garcia Campos]

Created attachment 256973 [details]
Patch

[WebKit Commit Bot]

Attachment 256973 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h:66:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h:71:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h:89:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 3 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Catanzaro]

Awesome, thanks!

I'm not sure about having both MediaPlayerRequestInstallMissingPluginsCallback::create and also a public constructor; shouldn't the constructor be private, to ensure the ref is always adopted?

In WebPageProxy::requestInstallMissingMediaPlugins, can't you use a protector Ref<WebPageProxy> and capture it in the lambda, instead of the manual ref/deref?

What happens when two HTMLMediaElements in the same web process attempt to install codecs at the same time? Looks like the first media element will never begin to play, right? I guess that's OK; that's quite a corner case.

[Carlos Garcia Campos]

(In reply to comment #13)
> Awesome, thanks!
> 
> I'm not sure about having both
> MediaPlayerRequestInstallMissingPluginsCallback::create and also a public
> constructor; shouldn't the constructor be private, to ensure the ref is
> always adopted?
> 

Yes, not a big issue since nobody else is going to use that, but yes, I can make the constructor private.

> In WebPageProxy::requestInstallMissingMediaPlugins, can't you use a
> protector Ref<WebPageProxy> and capture it in the lambda, instead of the
> manual ref/deref?
> 

That's what I did first, but then I need to leak the ref and I get a compile error because I don't use the returned value of leakRef(). So, I could use a variable that I won't use, or do the ref/deref manually which is what I did in the end.

> What happens when two HTMLMediaElements in the same web process attempt to
> install codecs at the same time? Looks like the first media element will
> never begin to play, right? I guess that's OK; that's quite a corner case.

This hasn't actually changed. In current code gst_install_plugins_async() returns GST_INSTALL_PLUGINS_INSTALL_IN_PROGRESS for the second media player, so m_missingPlugins is set to false and the callback is not called. With this patch the callback is called with GST_INSTALL_PLUGINS_INSTALL_IN_PROGRESS, which is pretty much the same.

[Carlos Garcia Campos]

Created attachment 257079 [details]
Updated patch

Just made the MediaPlayerRequestInstallMissingPluginsCallback constructor private.

[WebKit Commit Bot]

Attachment 257079 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h:66:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h:85:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h:90:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 3 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Philippe Normand]

Comment on attachment 257079 [details]
Updated patch

LGTM but perhaps we could have the callback declaration in a dedicated header file? I find it a bit sad to include the whole MediaPlayerPrivate header on WK2 side.

[Carlos Garcia Campos]

Created attachment 257448 [details]
Updated patch

Moved the callback class definition to its own header.

[WebKit Commit Bot]

Attachment 257448 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/gstreamer/MediaPlayerRequestInstallMissingPluginsCallback.h:31:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/platform/graphics/gstreamer/MediaPlayerRequestInstallMissingPluginsCallback.h:50:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/platform/graphics/gstreamer/MediaPlayerRequestInstallMissingPluginsCallback.h:55:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 3 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Carlos Garcia Campos]

Committed r187338: <http://trac.webkit.org/changeset/187338>