1204483 - crash in ACodec (deleted)@0x59ffe

Status: VERIFIED FIXED

(Firefox for Android :: General, defect)

Description

[Teodora Vermesan (:TeoVermesan)]

This bug was filed from the Socorro interface and is 
report bp-007d2f8c-62f2-42bf-b951-c7a972150914.
=============================================================

Tested with:
Device: Alcatel One Touch (Android 4.1.2)

Steps to reproduce:
1. Go to http://podiobooks.com/title/a-coffee-crusade/
2. Scroll down the page and tap the play button

Actual result:
- Firefox crashes

Comment 1

[Kevin Brosnan [:kbrosnan]]

#1 or #2 crash on beta 41

Comment 2

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

http://hg.mozilla.org/releases/mozilla-beta/annotate/0a913f6cd19b/dom/media/platforms/android/AndroidDecoderModule.cpp#l561

I see snorp and jchen in this file.

Comment 3

[V. Korn]

I have this crash too

https://crash-stats.mozilla.com/report/index/bp-0b67cbb7-59ee-4270-b1f7-14bb02150916
https://crash-stats.mozilla.com/report/index/bp-8ff9a0be-e321-45c4-96e6-fa0e72150911
https://crash-stats.mozilla.com/report/index/bp-21736e90-dbd8-40d8-a606-42a7f2150911

and i want to add something i noticed - have two android devices both with latest Firefox Beta (from Google Play)
One is Android 4.2.1 ARM CPU based phone which is crashing on Bandcamp or similar mp3 streaming sites
and there is Android 4.4.2 tablet (Dell Venue 7) on Intel Atom CPU - its NOT crashing on MP3 streaming.

Comment 4

[u421692]

Open: http://people.mozilla.org/~atrain/mobile/tests/media.html
This crash reproduces every time when trying to play the mp3 file

Comment 5

[u421692]

(In reply to Mihai Pop from comment #4)
> Open: http://people.mozilla.org/~atrain/mobile/tests/media.html
> This crash reproduces every time when trying to play the mp3 file

Tested on Alcatel One Touch 8008D (Android 4.1.2)

Comment 6

[Ralf]

I can reproduce this crash with any mp3 file. Firefox crashes after playing for 1 to 3 seconds.

Phone is based on Mediatek SoC MT6589, running Android 4.2.2.


https://crash-stats.mozilla.com/report/index/50d74b22-d4f2-4c31-967c-2a6812151021

Comment 7

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

Eugen, it looks like the PodCopy() in AudioDataDecoder::Output is overrunning the buffer. Can you see if you can figure out why that would happen?

Comment 8

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

I actually have a MediaTek tablet here, I'll see if I can repro there.

Comment 9

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

The device here I have is using a MT8125 and doesn't crash.

Comment 10

[Ralf]

Looking at the statistics of the crash reports suggests that it happens mostly on the MT6589 and MT6577 cpus.

Logcat does not provide any useful information.

I've just compiled latest development version 44.0a1 and can reproduce the crash there, too.

Comment 11

[Eugen Sawin [:esawin]]

Attachment: 0001-Add-AudioDataDecoder-Output-logs.patch

I don't have a device with the affected hardware and don't see anything wrong with PodCopy. We don't guard against bogus |size| value, but that would fail before during the allocation of |audio|.

This patch adds some logs to |Output|, can someone please attach the logs of a crashing session with the patch applied?

Comment 12

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

I have a device that can reproduce this, as it turns out, so I'll take the bug.

Comment 13

[Ralf]

Attachment: adb_logcat_crash.txt

crash happened after about 9 seconds when playing a mp3 file

Comment 14

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

Attachment: Fix busted audio decoding output on Android

This was really wrong, I think. The size is the number of bytes, not samples.
Also, we were not using the offset correctly.

Comment 15

[Eugen Sawin [:esawin]]

Comment on attachment 8679124 [details] [diff] [review]
Fix busted audio decoding output on Android

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

Great find, but I'm not sure about some of the changes yet.
I assume this does fix the crash because we don't try to read out of the buffer's range?

::: dom/media/platforms/android/AndroidDecoderModule.cpp
@@ +250,4 @@
>      int32_t offset;
>      NS_ENSURE_SUCCESS(rv = aInfo->Offset(&offset), rv);
>  
> +    int32_t numSamples = (size - offset) / 2;

According to the MediaCodec docs, the size is the size of the data, not the buffer, so the buffer is size + offset. We shouldn't subtract the offset.

Maybe we should also change the 2 to sizeof(AudioDataValue) and return early iff size == 0?

@@ +251,5 @@
>      NS_ENSURE_SUCCESS(rv = aInfo->Offset(&offset), rv);
>  
> +    int32_t numSamples = (size - offset) / 2;
> +
> +    const int32_t numFrames = (numSamples / numChannels);

Redundant parentheses.

@@ +253,5 @@
> +    int32_t numSamples = (size - offset) / 2;
> +
> +    const int32_t numFrames = (numSamples / numChannels);
> +    AudioDataValue* audio = new AudioDataValue[numSamples];
> +    PodCopy(audio, static_cast<AudioDataValue*>(aBuffer), numSamples);

We need to account for the offset on the source pointer.

Comment 16

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

(In reply to Eugen Sawin [:esawin] from comment #15)
> Comment on attachment 8679124 [details] [diff] [review]
> Fix busted audio decoding output on Android
> 
> Review of attachment 8679124 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Great find, but I'm not sure about some of the changes yet.
> I assume this does fix the crash because we don't try to read out of the
> buffer's range?

Yup. I guess we just got lucky before? Ugh.

> 
> ::: dom/media/platforms/android/AndroidDecoderModule.cpp
> @@ +250,4 @@
> >      int32_t offset;
> >      NS_ENSURE_SUCCESS(rv = aInfo->Offset(&offset), rv);
> >  
> > +    int32_t numSamples = (size - offset) / 2;
> 
> According to the MediaCodec docs, the size is the size of the data, not the
> buffer, so the buffer is size + offset. We shouldn't subtract the offset.

Yup, good catch.

> 
> Maybe we should also change the 2 to sizeof(AudioDataValue) and return early
> iff size == 0?

I think in other media code we just use #ifdef MOZ_SAMPLE_TYPE_S16, so I'll add some stuff for that and #error if that's not set.

> 
> @@ +251,5 @@
> >      NS_ENSURE_SUCCESS(rv = aInfo->Offset(&offset), rv);
> >  
> > +    int32_t numSamples = (size - offset) / 2;
> > +
> > +    const int32_t numFrames = (numSamples / numChannels);
> 
> Redundant parentheses.

Fixed

> 
> @@ +253,5 @@
> > +    int32_t numSamples = (size - offset) / 2;
> > +
> > +    const int32_t numFrames = (numSamples / numChannels);
> > +    AudioDataValue* audio = new AudioDataValue[numSamples];
> > +    PodCopy(audio, static_cast<AudioDataValue*>(aBuffer), numSamples);
> 
> We need to account for the offset on the source pointer.

Ugh, I guess that's what I get for rushing out a patch just before leaving for the day. Thanks!

Comment 17

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

Attachment: Fix busted audio decoding output on Android

Fix up review comments

Comment 18

[Eugen Sawin [:esawin]]

Comment on attachment 8679458 [details] [diff] [review]
Fix busted audio decoding output on Android

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

r=me with the void* arithmetic fixed.

::: dom/media/platforms/android/AndroidDecoderModule.cpp
@@ +250,5 @@
>      int32_t offset;
>      NS_ENSURE_SUCCESS(rv = aInfo->Offset(&offset), rv);
>  
> +#ifdef MOZ_SAMPLE_TYPE_S16
> +    int32_t numSamples = size / 2;

Can be const.

@@ +257,5 @@
> +#endif
> +
> +    const int32_t numFrames = numSamples / numChannels;
> +    AudioDataValue* audio = new AudioDataValue[numSamples];
> +    PodCopy(audio, static_cast<AudioDataValue*>(aBuffer + offset), numSamples);

That's illegal, you need to (static_)cast aBuffer to something sensible like uint8_t* first, do the pointer arithmetic and then (reinterpret_)cast it to AudioDataValue*. Or, we could do the pionter arithmetic on AudioDataValue* with offset/2.

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/393566e4fd28

Comment 20

[Ralf]

I have applied changeset 393566e4fd28 and can confirm that it fixes the crashes.
Thanks a lot!

Any chance that it will make it into 42.0.x or at least 43?

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/393566e4fd28

Comment 22

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

Attachment: Fix busted audio decoding output on Android

Comment 23

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

Comment on attachment 8680025 [details] [diff] [review]
Fix busted audio decoding output on Android

Approval Request Comment
[Feature/regressing bug #]: N/A
[User impact if declined]: Crashes while decoding audio (perhaps randomly)
[Describe test coverage new/current, TreeHerder]: m-c, nightly
[Risks and why]: Low
[String/UUID change made/needed]: None

This corrects some really stupid behavior. I'm betting we have other crash signatures related to this bug too.

Comment 24

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

(In reply to Ralf from comment #20)
> I have applied changeset 393566e4fd28 and can confirm that it fixes the
> crashes.
> Thanks a lot!
> 
> Any chance that it will make it into 42.0.x or at least 43?

Yup, working on it. Thanks for your help!

Comment 25

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8680025 [details] [diff] [review]
Fix busted audio decoding output on Android

James, we are going to build 42.0 today.
As this bug is top #1 & #6, I am considering taking it in 42. What do you think?
Your risk eval says "Low", could you explain a bit more?
Thanks

Comment 26

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8680025 [details] [diff] [review]
Fix busted audio decoding output on Android

Taking to aurora anyway.

Comment 27

[Sylvestre Ledru [:Sylvestre]]

Margaret, Mark, do you have an opinion on comment #25? Thanks (this is urgent as you can imagine)

Comment 28

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

(In reply to Sylvestre Ledru [:sylvestre] from comment #25)
> Comment on attachment 8680025 [details] [diff] [review]
> Fix busted audio decoding output on Android
> 
> James, we are going to build 42.0 today.
> As this bug is top #1 & #6, I am considering taking it in 42. What do you
> think?
> Your risk eval says "Low", could you explain a bit more?
> Thanks

Well the patch definitely makes this code more correct. I would have a hard time believing it made anything worse, and local/automated testing haven't uncovered any issues.

Comment 29

[:Margaret Leibovic]

I defer to snorp. Sounds like it would be fine to uplift.

Comment 30

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8680025 [details] [diff] [review]
Fix busted audio decoding output on Android

Approved for uplift to 42 (now m-r instead of beta) from conversation with Sylvestre. Crash fix.

Comment 31

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Wes - Heads up this needs to land today.

Comment 32

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to s/RefPtr/nsRefPtf/ for the uplift, but I think that's fine:
https://hg.mozilla.org/releases/mozilla-aurora/rev/72b0db9ced18
https://hg.mozilla.org/releases/mozilla-beta/rev/dc5307ccc909
https://hg.mozilla.org/releases/mozilla-release/rev/dc5307ccc909

Comment 33

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

And followups to fix some bustage:
https://hg.mozilla.org/releases/mozilla-aurora/rev/bf0aacb5affa
https://hg.mozilla.org/releases/mozilla-beta/rev/8e6007ac23a1
https://hg.mozilla.org/releases/mozilla-release/rev/8e6007ac23a1

Comment 34

[V. Korn]

Is this supposed to be fixed in 42.0b10?
Cause for me its still crashing as before
https://crash-stats.mozilla.com/report/index/f99c1131-7004-4926-8a1e-e84692151028
https://crash-stats.mozilla.com/report/index/fbf25d5c-5b08-4b5d-b54f-30fa62151028
https://crash-stats.mozilla.com/report/index/06cff79b-716e-4ef7-9af2-ab2412151028

Comment 35

[Robert Kaiser]

(In reply to V. Korn from comment #34)
> Is this supposed to be fixed in 42.0b10?

No, but 42.0 release builds (which are building right now) should have the fix.

Comment 36

[Teodora Vermesan (:TeoVermesan)]

I am not able to reproduce the crash with steps from description or steps from comment 4, using Alcatel One Touch (Android 4.1.2) on Firefox for Android 42.0 build 3.

Comment 37

[Teodora Vermesan (:TeoVermesan)]

I am not able to reproduce the crash with steps from description or steps from comment 4, using Alcatel One Touch (Android 4.1.2) on Firefox for Android 44.0a2 (2015-11-02)

Comment 38

[u421692]

Verified as fixed on Firefox 43 Beta 1 using Alcatel One Touch (Android 4.1.2)