1090300 - crash in mozilla::MediaCodecDataDecoder::DecoderLoop()

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Aaron Train [:aaronmt]]

This bug was filed from the Socorro interface and is 
report bp-411d835a-9eb9-4b8a-b97a-19a762141023.
=============================================================

Comment 1

[James Willcox (:snorp) ([e-mail]) (he/him)]

Fixed with bug 1086693

Comment 2

[James Willcox (:snorp) ([e-mail]) (he/him)]

Wait maybe not. It could be but now I'm not sure, reopening.

Comment 3

[Aaron Train [:aaronmt]]

bp-6e578867-5389-46c3-ba3d-36dda2141203

Still happening

Comment 4

[James Willcox (:snorp) ([e-mail]) (he/him)]

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.

Comment 5

[Aaron Train [:aaronmt]]

(In reply to James Willcox (:snorp) ([e-mail]) 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.

Comment 6

[Aaron Train [:aaronmt]]

Hi.

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

Logcat sent to Snorp and Aaron

Comment 8

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Repopulate input buffers when necessary in Android media decoder

Comment 9

[James Willcox (:snorp) ([e-mail]) (he/him)]

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 10

[Gian-Carlo Pascutto [:gcp]]

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.

Comment 11

[James Willcox (:snorp) ([e-mail]) (he/him)]

(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.

Comment 12

[James Willcox (:snorp) ([e-mail]) (he/him)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2de6cbc18110

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2de6cbc18110

Comment 14

[James Willcox (:snorp) ([e-mail]) (he/him)]

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.

Comment 15

[James Willcox (:snorp) ([e-mail]) (he/him)]

Wait that build date was for 37! Nightly looks fixed. Woo.

Comment 16

[James Willcox (:snorp) ([e-mail]) (he/him)]

Gonna let this bake just a little while then uplift

Comment 17

[James Willcox (:snorp) ([e-mail]) (he/him)]

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

Comment 18

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8567256 [details] [diff] [review]
Repopulate input buffers when necessary in Android media decoder

(In reply to James Willcox (:snorp) ([e-mail]) 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+

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/2cca5b090036