Closed
Bug 1204483
Opened 7 years ago
Closed 7 years ago
crash in ACodec (deleted)@0x59ffe
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(firefox41 wontfix, firefox42 verified, firefox43 verified, firefox44 verified)
VERIFIED
FIXED
Firefox 44
People
(Reporter: TeoVermesan, Assigned: snorp)
References
Details
(Keywords: crash, topcrash-android-armv7)
Crash Data
Attachments
(3 files, 2 obsolete files)
1.74 KB,
patch
|
Details | Diff | Splinter Review | |
83.42 KB,
text/plain
|
Details | |
2.24 KB,
patch
|
snorp
:
review+
Sylvestre
:
approval-mozilla-aurora+
lizzard
:
approval-mozilla-release+
|
Details | Diff | Splinter Review |
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 2•7 years ago
|
||
http://hg.mozilla.org/releases/mozilla-beta/annotate/0a913f6cd19b/dom/media/platforms/android/AndroidDecoderModule.cpp#l561 I see snorp and jchen in this file.
Updated•7 years ago
|
Crash Signature: [@ ACodec (deleted)@0x59ffe] → [@ ACodec (deleted)@0x59ffe]
[@ ACodec (deleted)@0x80ffe]
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.
Open: http://people.mozilla.org/~atrain/mobile/tests/media.html This crash reproduces every time when trying to play the mp3 file
(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•7 years ago
|
Crash Signature: [@ ACodec (deleted)@0x59ffe]
[@ ACodec (deleted)@0x80ffe] → [@ ACodec (deleted)@0x59ffe]
[@ ACodec (deleted)@0x80ffe]
[@ ACodec @0x59ffe]
[@ ACodec @0x80ffe]
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
Assignee | ||
Comment 7•7 years ago
|
||
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)
Assignee | ||
Comment 8•7 years ago
|
||
I actually have a MediaTek tablet here, I'll see if I can repro there.
Assignee | ||
Comment 9•7 years ago
|
||
The device here I have is using a MT8125 and doesn't crash.
Comment 10•7 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.
Comment 11•7 years ago
|
||
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)
Assignee | ||
Comment 12•7 years ago
|
||
I have a device that can reproduce this, as it turns out, so I'll take the bug.
Assignee: esawin → snorp
Comment 13•7 years ago
|
||
crash happened after about 9 seconds when playing a mp3 file
Assignee | ||
Comment 14•7 years ago
|
||
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 15•7 years ago
|
||
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.
Assignee | ||
Comment 16•7 years ago
|
||
(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!
Updated•7 years ago
|
Attachment #8679124 -
Flags: review?(esawin) → review-
Assignee | ||
Comment 17•7 years ago
|
||
Fix up review comments
Attachment #8679458 -
Flags: review?(esawin)
Assignee | ||
Updated•7 years ago
|
Attachment #8679124 -
Attachment is obsolete: true
Comment 18•7 years ago
|
||
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•7 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?
Comment 21•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/393566e4fd28
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox44:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
Assignee | ||
Comment 22•7 years ago
|
||
Attachment #8680025 -
Flags: review+
Assignee | ||
Updated•7 years ago
|
Attachment #8679458 -
Attachment is obsolete: true
Assignee | ||
Comment 23•7 years ago
|
||
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?
Assignee | ||
Comment 24•7 years ago
|
||
(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•7 years ago
|
||
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 26•7 years ago
|
||
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+
Updated•7 years ago
|
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]
Keywords: topcrash-android-armv7
Updated•7 years ago
|
Comment 27•7 years ago
|
||
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)
Assignee | ||
Comment 28•7 years ago
|
||
(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)
Comment 29•7 years ago
|
||
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)
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
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
Updated•7 years ago
|
Flags: needinfo?(mark.finkle)
Comment 34•7 years ago
|
||
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•7 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.
Reporter | ||
Comment 36•7 years ago
|
||
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.
Reporter | ||
Comment 37•7 years ago
|
||
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•7 years ago
|
||
Verified as fixed on Firefox 43 Beta 1 using Alcatel One Touch (Android 4.1.2)
Status: RESOLVED → VERIFIED
Updated•1 year ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•