Closed Bug 959007 Opened 6 years ago Closed 6 years ago

firefox crashes when starting html5 web player

Categories

(Core :: Audio/Video, defect)

26 Branch
x86_64
Linux
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla30
Tracking Status
firefox26 --- wontfix
firefox27 + wontfix
firefox28 + verified
firefox29 + verified
firefox30 --- verified
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed

People

(Reporter: sebastian, Assigned: eflores)

References

Details

(Keywords: crash, regression, reproducible)

Crash Data

Attachments

(2 files, 4 obsolete files)

Attached file 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.
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
Blocks: 886181
Status: UNCONFIRMED → NEW
Crash Signature: [@ mozilla::ReentrantMonitor::Enter()]
Component: Untriaged → Video/Audio
Ever confirmed: true
Product: Firefox → Core
Tracking this reproducible crash regression and passing this onto :edwin, given this is a fallout from Bug 886181.
Flags: needinfo?(edwin)
Attached patch 959007.patch (obsolete) — Splinter Review
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.
Attachment #8360736 - Flags: review?(cpearce)
Attachment #8360736 - Flags: feedback?(alessandro.d)
Flags: needinfo?(edwin)
Attachment #8360736 - Flags: review?(cpearce) → review+
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.
Attachment #8360736 - Flags: feedback?(alessandro.d) → feedback-
(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?
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!).
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.
Attached patch bug959007.patch (obsolete) — Splinter Review
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 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)
Attached patch bug959007.patch (obsolete) — Splinter Review
Attachment #8365661 - Attachment is obsolete: true
(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.
This is how it looks with 1.0 + the autoplug-select fix in gstreamer https://github.com/alessandrod/gecko-dev/commit/d531e9d50a057c5eb7710949b7c3bbaced34568e
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 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
Attachment #8365793 - Flags: review?(edwin)
Attachment #8365793 - Flags: review?(cpearce)
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.
Attachment #8365793 - Flags: review?(cpearce) → feedback+
Attached patch bug959007.patch (obsolete) — Splinter Review
style fixes
Attachment #8360736 - Attachment is obsolete: true
Attachment #8365793 - Attachment is obsolete: true
Attachment #8365793 - Flags: review?(edwin)
Attachment #8369378 - Flags: review?(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. :)
Attachment #8369378 - Flags: review?(edwin) → review+
Attached patch bug959007.patchSplinter Review
Attachment #8369378 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8c9465855f26
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Looks like we need uplift noms here for Aurora/Beta
Flags: needinfo?(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.
Attachment #8369818 - Flags: approval-mozilla-beta?
Attachment #8369818 - Flags: approval-mozilla-aurora?
Flags: needinfo?(edwin)
Attachment #8369818 - Flags: approval-mozilla-beta?
Attachment #8369818 - Flags: approval-mozilla-beta+
Attachment #8369818 - Flags: approval-mozilla-aurora?
Attachment #8369818 - Flags: approval-mozilla-aurora+
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).
Status: RESOLVED → VERIFIED
Keywords: verifyme
QA Contact: petruta.rasa
You need to log in before you can comment on or make changes to this bug.