Closed
Bug 959007
Opened 11 years ago
Closed 11 years ago
firefox crashes when starting html5 web player
Categories
(Core :: Audio/Video, defect)
Tracking
()
People
(Reporter: sebastian, Assigned: eflores)
References
Details
(Keywords: crash, regression, reproducible)
Crash Data
Attachments
(2 files, 4 obsolete files)
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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•11 years ago
|
||
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()]
status-firefox26:
--- → affected
tracking-firefox27:
--- → ?
tracking-firefox28:
--- → ?
tracking-firefox29:
--- → ?
Component: Untriaged → Video/Audio
Ever confirmed: true
Product: Firefox → Core
Comment 2•11 years ago
|
||
Tracking this reproducible crash regression and passing this onto :edwin, given this is a fallout from Bug 886181.
Updated•11 years ago
|
Assignee: nobody → edwin
status-firefox27:
--- → affected
status-firefox28:
--- → affected
status-firefox29:
--- → affected
Updated•11 years ago
|
Flags: needinfo?(edwin)
Assignee | ||
Comment 3•11 years ago
|
||
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)
Updated•11 years ago
|
Attachment #8360736 -
Flags: review?(cpearce) → review+
Comment 4•11 years ago
|
||
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-
Assignee | ||
Comment 5•11 years ago
|
||
(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•11 years ago
|
||
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•11 years ago
|
||
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•11 years ago
|
||
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•11 years ago
|
||
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•11 years ago
|
||
Attachment #8365661 -
Attachment is obsolete: true
Comment 11•11 years ago
|
||
(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•11 years ago
|
||
This is how it looks with 1.0 + the autoplug-select fix in gstreamer https://github.com/alessandrod/gecko-dev/commit/d531e9d50a057c5eb7710949b7c3bbaced34568e
Comment 13•11 years ago
|
||
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•11 years ago
|
||
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 15•11 years ago
|
||
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+
Comment 16•11 years ago
|
||
style fixes
Attachment #8360736 -
Attachment is obsolete: true
Attachment #8365793 -
Attachment is obsolete: true
Attachment #8365793 -
Flags: review?(edwin)
Attachment #8369378 -
Flags: review?(edwin)
Assignee | ||
Comment 17•11 years ago
|
||
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+
Comment 18•11 years ago
|
||
Attachment #8369378 -
Attachment is obsolete: true
Updated•11 years ago
|
Keywords: checkin-needed
Comment 19•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c9465855f26
Keywords: checkin-needed
Comment 20•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/8c9465855f26
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Comment 21•11 years ago
|
||
Looks like we need uplift noms here for Aurora/Beta
status-firefox30:
--- → fixed
Flags: needinfo?(edwin)
Assignee | ||
Comment 22•11 years ago
|
||
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)
Updated•11 years ago
|
Attachment #8369818 -
Flags: approval-mozilla-beta?
Attachment #8369818 -
Flags: approval-mozilla-beta+
Attachment #8369818 -
Flags: approval-mozilla-aurora?
Attachment #8369818 -
Flags: approval-mozilla-aurora+
Comment 23•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/69a772ab5bcb https://hg.mozilla.org/releases/mozilla-beta/rev/8a5f7e0a222d
Updated•11 years ago
|
status-b2g-v1.3:
--- → fixed
Comment 25•11 years ago
|
||
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).
Updated•11 years ago
|
status-b2g-v1.3T:
--- → fixed
status-b2g-v1.4:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•