959007 - firefox crashes when starting html5 web player

Status: VERIFIED FIXED

(Core :: Audio/Video, defect)

Description

[9er]

Attachment: strace -T -ttt

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:26.0) Gecko/20100101 Firefox/26.0 (Beta/Release)
Build ID: 20131212154635

Steps to reproduce:

Click "Play" on a certain web-Player on a podcast site.
Player is Open Source, HTML5 "Podlove" Player, http://podlove.org/podlove-web-player/

The Player will start to load the audio file and then crash Firefox.

Example player on website (anchorlink makes the player play the file automatically) http://die-sondersendung.de/sz004-die-kleinen-werden-so-schnell-erwachsen#t=0:0

Stacktrace is attatched.

Mozilla/5.0 (X11; Linux x86_64; rv:26.0) Gecko/20100101 Firefox/26.0
Linux tp 3.12.6-1-ARCH #1 SMP PREEMPT Fri Dec 20 19:39:00 CET 2013 x86_64 GNU/Linux


Actual results:

SIGSEGV (FF is just gone immediately)


Expected results:

Play the audio file.

Comment 1

[Alice0775 White]

bp-dd5ee59a-28a5-410a-9891-b6bf82140113

Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/e9ee43da0c16
Mozilla/5.0 (X11; Linux i686; rv:26.0) Gecko/20100101 Firefox/26.0 ID:20130911162107
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/0192ffffc633
Mozilla/5.0 (X11; Linux i686; rv:26.0) Gecko/20100101 Firefox/26.0 ID:20130911162531
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=e9ee43da0c16&tochange=0192ffffc633

Regressed by:
0192ffffc633	Edwin Flores — Bug 886181 - Pref on gstreamer backend r=cpearce

Comment 2

[bhavana bajaj [:bajaj]]

Tracking this reproducible crash regression and passing this onto :edwin, given this is a fallout from Bug 886181.

Comment 3

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: 959007.patch

Looks like the stream in the given link has an image/jpeg track for "Chapter Images".

Two bugs here:

1) GStreamer sees this image/jpeg track and tries to output video, but we don't get an ImageContainer when we're inside <audio> elements => nullptr dereference.

2) We were only whitelisting caps _after_ playbin had built the decoding pipeline, which doesn't actually make much sense as we sometimes want to support a subset of the tracks in a stream. This patch checks all of the caps that decodebin sees, and tells decodebin to not connect any caps that aren't on our whitelist.

Comment 4

[Alessandro Decina]

Comment on attachment 8360736 [details] [diff] [review]
959007.patch

The patch is good in principle. As discussed on IRC with Edwin I think we should use autoplug-select and not autoplug-continue though. 

While decoding, gstreamer sometimes goes through some intermediary caps that we don't necessarily want to whitelist (because they are implementation details really). For example during mp3 decoding autoplug-continue is typically first called for "application/x-id3" (to parse ID3 tags) and then "application/mpeg". This is why in ::CheckSupportedFormats now we only check caps on proper demuxers and decoders, and skip tag demuxers, codec parsers etc.

Using autoplug-select we can still apply our whitelist, and we can still let gst plug intermediary tag demuxers, parsers etc (by not applying the whitelist to those kind of elements). Also, by using autoplug-select we can avoid exposing the sub streams we don't support instead of just skipping decoding and exposing them as undecoded streams.

Comment 5

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

(In reply to Alessandro Decina from comment #4)
> The patch is good in principle. As discussed on IRC with Edwin I think we
> should use autoplug-select and not autoplug-continue though. 

I had a go at this the other day, but found my signal handler wasn't being called when I tried attaching to the autoplug-select signal. I notice from the documentation:

> Only the signal handler that is connected first will ever by invoked.

Does uridecodebin or something usually attach to decodebin's autoplug-select?

Comment 6

[Sebastian Dröge (:slomo)]

As discussed on IRC with Alessandro, the future and ideal solution for this would be to use autoplug-select for this and then properly distinguish between demuxers, decoders and everything else. This needs a fix in GStreamer though as currently only one thing can connect to autoplug-select (and playbin does that).

For older GStreamer versions the best is probably to use this autoplug-continue solution for now and just whitelist all the relevant caps (like the 20 different MP4 and Matroska caps, ID3, APE tags, etc). autoplug-sort could also be used instead of autoplug-select (as a workaround), but that would lead to an assertion and is not really nice (and will crash for people who build without assertions... don't do that!).

Comment 7

[Sebastian Dröge (:slomo)]

Oh and independent from that it would be good to have information at the GStreamer level if this is an <audio> or <video> tag, i.e. if any video should be shown at all. Because if no video should be shown you could just unset the video tag on playbin and stop worrying a bit more.

Comment 8

[Alessandro Decina]

Attachment: bug959007.patch

Here's a slightly tweaked version of the patch that uses autoplug-sort (until autoplug-select is fixed).

At this point I don't have a strong preference between this and using autoplug-continue as a stop gap before autoplug-select is fixed. If we do go for autoplug-continue, we need to whitelist (a lot) more gst caps though.

Comment 9

[Sebastian Dröge (:slomo)]

Comment on attachment 8365661 [details] [diff] [review]
bug959007.patch

Review of attachment 8365661 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/gstreamer/GStreamerReader.cpp
@@ +1098,5 @@
> +
> +  // Attach |AutoplugContinueCb| to the "autoplug-continue" signal of the
> +  // decodebin.
> +  if (!strncmp(name, sDecodePrefix, sizeof(sDecodePrefix) - 1)) {
> +    g_signal_connect(G_OBJECT(aElement), "autoplug-sort",

Why don't you just use autoplug-sort on the uridecodebin? (also you say autoplug-continue in the comment)

Comment 10

[Alessandro Decina]

Attachment: bug959007.patch

Comment 11

[Alessandro Decina]

(In reply to Sebastian Dröge (slomo) from comment #9)
> Comment on attachment 8365661 [details] [diff] [review]
> bug959007.patch
> 
> Review of attachment 8365661 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/gstreamer/GStreamerReader.cpp
> @@ +1098,5 @@
> > +
> > +  // Attach |AutoplugContinueCb| to the "autoplug-continue" signal of the
> > +  // decodebin.
> > +  if (!strncmp(name, sDecodePrefix, sizeof(sDecodePrefix) - 1)) {
> > +    g_signal_connect(G_OBJECT(aElement), "autoplug-sort",
> 
> Why don't you just use autoplug-sort on the uridecodebin? (also you say
> autoplug-continue in the comment)

No reason. The new patch uses uridecodebin directly.

Comment 12

[Alessandro Decina]

This is how it looks with 1.0 + the autoplug-select fix in gstreamer https://github.com/alessandrod/gecko-dev/commit/d531e9d50a057c5eb7710949b7c3bbaced34568e

Comment 13

[bhavana bajaj [:bajaj]]

We are ready to go to build with our final beta and release candidate today and may have to eat this crasher for this cycle. Please request uplift on Fx28- aurora(soon to be beta) as soon as this is ready once it lands on MC and has had the needed bake time.

Comment 14

[Alessandro Decina]

Comment on attachment 8365793 [details] [diff] [review]
bug959007.patch

I think this is good enough to be checked in and i'm about to land the autoplug-select change in gst upstream so we can make the code less ugly in the future

Comment 15

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8365793 [details] [diff] [review]
bug959007.patch

Review of attachment 8365793 [details] [diff] [review]:
-----------------------------------------------------------------

Edwin should review this, he'll understand more about the implications of these changes than I.

::: content/media/gstreamer/GStreamerReader.cpp
@@ +1101,5 @@
> +GStreamerReader::ShouldAutoplugFactory(GstElementFactory* aFactory, GstCaps* aCaps)
> +{
> +  bool autoplug;
> +  const gchar *klass = gst_element_factory_get_klass(aFactory);
> +  if (strstr(klass, "Demuxer") && !strstr(klass, "Metadata"))

Braces on all if/else statements please, including single line ones.

::: content/media/gstreamer/GStreamerReader.h
@@ +142,5 @@
> +  static void PlayElementAddedCb(GstBin *aBin, GstElement *aElement,
> +                                 gpointer *aUserData);
> +
> +
> +  static bool ShouldAutoplugFactory(GstElementFactory* aFactory, GstCaps* aCaps);

Delete extra newline before ShouldAutoplugFactory, add one after.

Comment 16

[Alessandro Decina]

Attachment: bug959007.patch

style fixes

Comment 17

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Comment on attachment 8369378 [details] [diff] [review]
bug959007.patch

Review of attachment 8369378 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!

::: content/media/gstreamer/GStreamerReader.cpp
@@ +1124,5 @@
> +GStreamerReader::AutoplugSortCb(GstElement* aElement, GstPad* aPad,
> +                                GstCaps* aCaps, GValueArray* aFactories)
> +{
> +  if (!aFactories->n_values)
> +    return nullptr;

Braces please. :)

Comment 18

[Alessandro Decina]

Attachment: bug959007.patch

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8c9465855f26

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8c9465855f26

Comment 21

[Lukas Blakk [:lsblakk] use ?needinfo]

Looks like we need uplift noms here for Aurora/Beta

Comment 22

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Comment on attachment 8369818 [details] [diff] [review]
bug959007.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 886181
User impact if declined: Firefox continuing to crash on certain MP3 streams.
Testing completed (on m-c, etc.): Has been on m-c for almost a week.
Risk to taking this patch (and alternatives if risky): Very slim chance that MP3/AVC/AAC decoding fails for some Linux users.
String or IDL/UUID changes made by this patch: None.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/69a772ab5bcb
https://hg.mozilla.org/releases/mozilla-beta/rev/8a5f7e0a222d

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/8a5f7e0a222d

Comment 25

[Petruta Horea [:phorea], Desktop QA]

Reproduced on Firefox 26 under Ubuntu 13.04 64-bit.
Verified as fixed on Firefox 28 beta 4 (20140213172947), latest Aurora 29.0a2 (20140219004002) and latest Nightly 30.0a1 (20140219030203).