crash in mozilla::MediaCodecDataDecoder::DecoderLoop()

RESOLVED FIXED in Firefox 37

Status

()

Firefox for Android
General
--
critical
RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: aaronmt, Assigned: snorp)

Tracking

({crash, topcrash-android-armv7})

36 Branch
Firefox 38
All
Android
crash, topcrash-android-armv7
Points:
---

Firefox Tracking Flags

(firefox36 wontfix, firefox37+ fixed, firefox38 fixed, fennec35+)

Details

(crash signature)

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
This bug was filed from the Socorro interface and is 
report bp-411d835a-9eb9-4b8a-b97a-19a762141023.
=============================================================
tracking-fennec: ? → 35+
Fixed with bug 1086693
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Wait maybe not. It could be but now I'm not sure, reopening.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Reporter)

Comment 3

3 years ago
bp-6e578867-5389-46c3-ba3d-36dda2141203

Still happening
status-firefox37: --- → affected
Hmmm, the report says OOM, which is interesting. Aaron can you look into the logcats for some of those and see if you find anything interesting?

This stack may be somewhat bogus if it is a OOM crash, since that's an assertion we throw from mozalloc.
Flags: needinfo?(aaron.train)
(Reporter)

Comment 5

3 years ago
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #4)
> Hmmm, the report says OOM, which is interesting. Aaron can you look into the
> logcats for some of those and see if you find anything interesting?

Single OOM report. About ~75 at [@ mozilla::MediaCodecDataDecoder::DecoderLoop()], see reports in first comment. Finkle has access to logcats.
Flags: needinfo?(aaron.train) → needinfo?(mark.finkle)
(Reporter)

Comment 6

3 years ago
Hi.
Logcat sent to Snorp and Aaron
Flags: needinfo?(mark.finkle)
Keywords: topcrash-android-armv7
Created attachment 8567256 [details] [diff] [review]
Repopulate input buffers when necessary in Android media decoder
Attachment #8567256 - Flags: review?(gpascutto)
I looked closer at the crash stack, and it looks like we get a null ByteBuffer from the input array. The attached patch tries to repopulate the array and try again, and then properly bail out if it still fails after that.
Comment on attachment 8567256 [details] [diff] [review]
Repopulate input buffers when necessary in Android media decoder

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

This actually makes sense after reading the MediaCodec SDK docs. Someone wrote a dequeueInputBuffer implementation but forgot he's not allowed to mess with the buffers if getInputBuffers(void) had been called at any previous point.

It's like staring at a pile of barf.
Attachment #8567256 - Flags: review?(gpascutto) → review+
(In reply to Gian-Carlo Pascutto [:gcp] from comment #10)
> Comment on attachment 8567256 [details] [diff] [review]
> Repopulate input buffers when necessary in Android media decoder
> 
> Review of attachment 8567256 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This actually makes sense after reading the MediaCodec SDK docs. Someone
> wrote a dequeueInputBuffer implementation but forgot he's not allowed to
> mess with the buffers if getInputBuffers(void) had been called at any
> previous point.
> 
> It's like staring at a pile of barf.

Yeah, I thought maybe I had a bug somewhere, but the docs pretty clearly state you only need to get the input buffers once after start(). Ugh.
https://hg.mozilla.org/integration/mozilla-inbound/rev/2de6cbc18110
https://hg.mozilla.org/mozilla-central/rev/2de6cbc18110
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
status-firefox38: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
Ugh, I still see reports in Nightly 20150223004020. Same signature, so this didn't fix it. That JNI address points to GetDirectBufferAddress(), but the only other place we call that is for output, and we already check that it is not null. Strange.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Wait that build date was for 37! Nightly looks fixed. Woo.
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
Resolution: --- → FIXED
Gonna let this bake just a little while then uplift
Comment on attachment 8567256 [details] [diff] [review]
Repopulate input buffers when necessary in Android media decoder

Approval Request Comment
[Feature/regressing bug #]: bug 1014614
[User impact if declined]: crashes
[Describe test coverage new/current, TreeHerder]: nightly, aurora, automation
[Risks and why]: low
[String/UUID change made/needed]: none
Attachment #8567256 - Flags: approval-mozilla-beta?
status-firefox36: affected → wontfix
tracking-firefox37: --- → +
Comment on attachment 8567256 [details] [diff] [review]
Repopulate input buffers when necessary in Android media decoder

(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) from comment #16)
> Gonna let this bake just a little while then uplift

Next time let's uplift a little earlier. :)

Given that this has baked for several weeks on 38 and has been verified, we'll take this topcrash fix in the final Beta of the 37 cycle. Beta+
Attachment #8567256 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/releases/mozilla-beta/rev/2cca5b090036
status-firefox37: affected → fixed
You need to log in before you can comment on or make changes to this bug.