Summary: | [GStreamer] Crashes during plugin installation | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Michael Catanzaro <mcatanzaro> | ||||||||||
Component: | Media | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Major | CC: | cgarcia, commit-queue, mcatanzaro, pnormand | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | PC | ||||||||||||
OS: | Linux | ||||||||||||
See Also: | https://bugzilla.redhat.com/show_bug.cgi?id=1212604 | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 135973, 146993, 147000, 147103 | ||||||||||||
Attachments: |
|
Description
Michael Catanzaro
2015-04-23 08:18:00 PDT
Would it be too much asking for a debug build trace? :) Seems like the m_pipeline is somehow null? 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. 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. 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. 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. 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. 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. 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. 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). 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). Created attachment 256973 [details]
Patch
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.
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. (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. Created attachment 257079 [details]
Updated patch
Just made the MediaPlayerRequestInstallMissingPluginsCallback constructor private.
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 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.
Created attachment 257448 [details]
Updated patch
Moved the callback class definition to its own header.
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.
Committed r187338: <http://trac.webkit.org/changeset/187338> |