Bug 144099 - [GStreamer] Crashes during plugin installation
Summary: [GStreamer] Crashes during plugin installation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Major
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 135973 146993 147000 147103
  Show dependency treegraph
 
Reported: 2015-04-23 08:18 PDT by Michael Catanzaro
Modified: 2015-07-24 05:08 PDT (History)
4 users (show)

See Also:


Attachments
backtrace (76.56 KB, text/plain)
2015-04-26 12:42 PDT, Michael Catanzaro
no flags Details
Patch (25.99 KB, patch)
2015-07-17 09:27 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Updated patch (25.99 KB, patch)
2015-07-19 22:37 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Updated patch (27.38 KB, patch)
2015-07-24 04:25 PDT, Carlos Garcia Campos
pnormand: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2015-04-23 08:18:00 PDT
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.
Comment 1 Philippe Normand 2015-04-23 08:35:11 PDT
Would it be too much asking for a debug build trace? :)
Seems like the m_pipeline is somehow null?
Comment 2 Michael Catanzaro 2015-04-23 12:36:01 PDT
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.
Comment 3 Michael Catanzaro 2015-04-26 12:42:33 PDT
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.
Comment 4 Carlos Garcia Campos 2015-07-16 04:01:24 PDT
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.
Comment 5 Carlos Garcia Campos 2015-07-16 04:25:00 PDT
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.
Comment 6 Michael Catanzaro 2015-07-16 10:05:11 PDT
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.
Comment 7 Carlos Garcia Campos 2015-07-17 02:19:37 PDT
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.
Comment 8 Carlos Garcia Campos 2015-07-17 03:09:16 PDT
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.
Comment 9 Carlos Garcia Campos 2015-07-17 09:15:39 PDT
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).
Comment 10 Michael Catanzaro 2015-07-17 09:24:31 PDT
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).
Comment 11 Carlos Garcia Campos 2015-07-17 09:27:00 PDT
Created attachment 256973 [details]
Patch
Comment 12 WebKit Commit Bot 2015-07-17 09:29:25 PDT
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.
Comment 13 Michael Catanzaro 2015-07-17 13:30:30 PDT
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.
Comment 14 Carlos Garcia Campos 2015-07-18 01:02:27 PDT
(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.
Comment 15 Carlos Garcia Campos 2015-07-19 22:37:08 PDT
Created attachment 257079 [details]
Updated patch

Just made the MediaPlayerRequestInstallMissingPluginsCallback constructor private.
Comment 16 WebKit Commit Bot 2015-07-19 22:39:45 PDT
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.
Comment 17 Philippe Normand 2015-07-24 00:31:22 PDT
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.
Comment 18 Carlos Garcia Campos 2015-07-24 04:25:18 PDT
Created attachment 257448 [details]
Updated patch

Moved the callback class definition to its own header.
Comment 19 WebKit Commit Bot 2015-07-24 04:26:50 PDT
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.
Comment 20 Carlos Garcia Campos 2015-07-24 05:08:04 PDT
Committed r187338: <http://trac.webkit.org/changeset/187338>