Closed Bug 1090300 Opened 5 years ago Closed 5 years ago

crash in mozilla::MediaCodecDataDecoder::DecoderLoop()

Categories

(Firefox for Android :: General, defect, critical)

36 Branch
All
Android
defect
Not set
critical

Tracking

()

RESOLVED FIXED
Firefox 38
Tracking Status
firefox36 --- wontfix
firefox37 + fixed
firefox38 --- fixed
fennec 35+ ---

People

(Reporter: aaronmt, Assigned: snorp)

References

Details

(Keywords: crash, topcrash-android-armv7)

Crash Data

Attachments

(1 file)

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
Closed: 5 years ago
Resolution: --- → FIXED
Wait maybe not. It could be but now I'm not sure, reopening.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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)
(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)
Hi.
Logcat sent to Snorp and Aaron
Flags: needinfo?(mark.finkle)
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/mozilla-central/rev/2de6cbc18110
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
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
Closed: 5 years ago5 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?
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+
You need to log in before you can comment on or make changes to this bug.