crash in ACodec (deleted)@0x59ffe

VERIFIED FIXED in Firefox 42

Status

()

defect
--
critical
VERIFIED FIXED
4 years ago
3 years ago

People

(Reporter: TeoVermesan, Assigned: snorp)

Tracking

({crash, topcrash-android-armv7})

Trunk
Firefox 44
ARM
Android
Points:
---

Firefox Tracking Flags

(firefox41 wontfix, firefox42 verified, firefox43 verified, firefox44 verified)

Details

(crash signature)

Attachments

(3 attachments, 2 obsolete attachments)

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
#1 or #2 crash on beta 41
Flags: needinfo?(snorp)
Crash Signature: [@ ACodec (deleted)@0x59ffe] → [@ ACodec (deleted)@0x59ffe] [@ ACodec (deleted)@0x80ffe]

Comment 3

4 years ago
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

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

Comment 5

4 years ago
(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)

Updated

4 years ago
Crash Signature: [@ ACodec (deleted)@0x59ffe] [@ ACodec (deleted)@0x80ffe] → [@ ACodec (deleted)@0x59ffe] [@ ACodec (deleted)@0x80ffe] [@ ACodec @0x59ffe] [@ ACodec @0x80ffe]

Comment 6

4 years ago
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
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?
Assignee: nobody → esawin
Flags: needinfo?(snorp) → needinfo?(esawin)
I actually have a MediaTek tablet here, I'll see if I can repro there.
The device here I have is using a MT8125 and doesn't crash.

Comment 10

4 years ago
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.
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?
Flags: needinfo?(esawin)
I have a device that can reproduce this, as it turns out, so I'll take the bug.
Assignee: esawin → snorp

Comment 13

4 years ago
crash happened after about 9 seconds when playing a mp3 file
This was really wrong, I think. The size is the number of bytes, not samples.
Also, we were not using the offset correctly.
Attachment #8679124 - Flags: review?(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.
(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!
Attachment #8679124 - Flags: review?(esawin) → review-
Fix up review comments
Attachment #8679458 - Flags: review?(esawin)
Attachment #8679124 - Attachment is obsolete: true
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.
Attachment #8679458 - Flags: review?(esawin) → review+

Comment 20

4 years ago
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?
https://hg.mozilla.org/mozilla-central/rev/393566e4fd28
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
Attachment #8679458 - Attachment is obsolete: true
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.
Attachment #8680025 - Flags: approval-mozilla-beta?
Attachment #8680025 - Flags: approval-mozilla-aurora?
(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 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
Flags: needinfo?(snorp)
Attachment #8680025 - Flags: approval-mozilla-beta? → approval-mozilla-release?
Comment on attachment 8680025 [details] [diff] [review]
Fix busted audio decoding output on Android

Taking to aurora anyway.
Attachment #8680025 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Crash Signature: [@ ACodec (deleted)@0x59ffe] [@ ACodec (deleted)@0x80ffe] [@ ACodec @0x59ffe] [@ ACodec @0x80ffe] → [@ ACodec (deleted)@0x59ffe] [@ ACodec (deleted)@0x80ffe] [@ ACodec (deleted)@0x7effe] [@ ACodec @0x59ffe] [@ ACodec @0x80ffe]
Margaret, Mark, do you have an opinion on comment #25? Thanks (this is urgent as you can imagine)
Flags: needinfo?(mark.finkle)
Flags: needinfo?(margaret.leibovic)
(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.
Flags: needinfo?(snorp)
I defer to snorp. Sounds like it would be fine to uplift.
Flags: needinfo?(margaret.leibovic)
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.
Attachment #8680025 - Flags: approval-mozilla-release? → approval-mozilla-release+
Wes - Heads up this needs to land today.
Flags: needinfo?(wkocher)
Flags: needinfo?(mark.finkle)

Comment 35

4 years ago
(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.
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.
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

4 years ago
Verified as fixed on Firefox 43 Beta 1 using Alcatel One Touch (Android 4.1.2)
Status: RESOLVED → VERIFIED
Duplicate of this bug: 1210657
Duplicate of this bug: 1173360
You need to log in before you can comment on or make changes to this bug.