Closed Bug 1245542 Opened 4 years ago Closed 4 years ago

Firefox 46 & 47 crashes in [@ memmove | mozilla::AudioStream::GetUnprocessed ]

Categories

(Core :: Audio/Video: Playback, defect, critical)

46 Branch
x86_64
Windows
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla47
Tracking Status
firefox45 --- unaffected
firefox46 + verified
firefox47 + verified

People

(Reporter: Virtual, Assigned: jwwang)

References

Details

(Keywords: crash, nightly-community, regression)

Crash Data

Attachments

(1 file)

[Tracking Requested - why for this release]: Regression

STR:
1. Open some video on YouTube
2. Change played time a few times
3. Open some other website in other tab
4. Go back to YouTube tab and change played time a few times
is rare cases crash will happen

Crashlog report:
https://crash-stats.mozilla.com/report/index/f1b348ec-ad87-4147-9a46-1eb392160203
Crash Signature: [@ memmove | mozilla::AudioStream::GetUnprocessed ]
Flags: needinfo?(jwwang)
Do you know which specific Youtube video causes the crash?
Assignee: nobody → jwwang
Flags: needinfo?(jwwang) → needinfo?(bernesb)
Unfortunately, I don't remember correctly on which YT video did it crash, probably:
https://www.youtube.com/watch?v=9e-ONlPkCUU
https://www.youtube.com/watch?v=pbJ4nBkb6WA
but nothing specific for sure.
I also checked in crash reporter option to send URLs of visited websites when it crashed, maybe it will be the hint, as I can't see it by myself in this sent crashreport anywhere.
Flags: needinfo?(bernesb)
Hardware: All → x86_64
Comment on attachment 8716096 [details]
MozReview Request: Bug 1245542 - I suspect AudioData::mAudioData/mFrames are poisoned when sample format doesn't match the metadata. Let's ignore these samples to see if crash volume can be reduced. r=kinetik.

https://reviewboard.mozilla.org/r/33709/#review30429

From the description and patch, I don't really understand what the problem is or how this could help.  Can you please explain?

It looks like a use-after-free, and without having dug into this, I'm concerned about the safety of using AudioQueue.Peek*() in this code (called from arbitrary libcubeb threads), which can only be safe if we're sure the MDSM machinery does not remove elements (e.g. by calling Reset()).
Attachment #8716096 - Flags: review?(kinetik)
AudioStream::DataCallback() happens before AudioStream::Shutdown() [1] which is called indirectly by StopMediaSink() [2] which happens before AudioQueue().Reset(). So AudioQueue.Peek*() should be safe to call in libcubeb callback threads. I will change the assertion AudioStream::DataCallback() to MOZ_RELEASE_ASSERT to make sure DataCallback() nevers come after Shutdown().

[1] https://hg.mozilla.org/mozilla-central/file/584870f1cbc5d060a57e147ce249f736956e2b62/dom/media/AudioStream.cpp#l636
[2] https://hg.mozilla.org/mozilla-central/file/584870f1cbc5d060a57e147ce249f736956e2b62/dom/media/MediaDecoderStateMachine.cpp#l2395
https://crash-stats.mozilla.com/report/index/f1b348ec-ad87-4147-9a46-1eb392160203

The crash reason is EXCEPTION_ACCESS_VIOLATION_READ so I think c->Data() returns an invalid pointer at [1]. Since c->Data() is calculated by AudioData::mAudioData/mFrames, I suspect they have abnormal values when format mismatch happens.

[1] http://hg.mozilla.org/mozilla-central/annotate/f2f8fc172f4c/dom/media/AudioStream.cpp#l588
Comment on attachment 8716096 [details]
MozReview Request: Bug 1245542 - I suspect AudioData::mAudioData/mFrames are poisoned when sample format doesn't match the metadata. Let's ignore these samples to see if crash volume can be reduced. r=kinetik.

https://reviewboard.mozilla.org/r/33709/#review30435

Thanks, so this change is a band-aid and the real problem is hiding somewhere else.
Attachment #8716096 - Flags: review+
Thanks for the review. Per comment 7, I will open a follow-up to add more assertions to detect if there are any unexpected activities in libcubeb callback threads.
ni me to watch if the volume of crash is reduced after landing.
Flags: needinfo?(jwwang)
https://hg.mozilla.org/mozilla-central/rev/ffdf364dd295
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Comment on attachment 8716096 [details]
MozReview Request: Bug 1245542 - I suspect AudioData::mAudioData/mFrames are poisoned when sample format doesn't match the metadata. Let's ignore these samples to see if crash volume can be reduced. r=kinetik.

Approval Request Comment
[Feature/regressing bug #]:1240420
[User impact if declined]:Crash might happen in a low rate while watching youtube.
[Describe test coverage new/current, TreeHerder]:TreeHerder
[Risks and why]: Low. The change is very simple.
[String/UUID change made/needed]:none
Attachment #8716096 - Flags: approval-mozilla-aurora?
New crash in 46, tracking.
Comment on attachment 8716096 [details]
MozReview Request: Bug 1245542 - I suspect AudioData::mAudioData/mFrames are poisoned when sample format doesn't match the metadata. Let's ignore these samples to see if crash volume can be reduced. r=kinetik.

Crash fix for recent regression, verified on nightly. Please uplift to aurora.
Attachment #8716096 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
failed to uplift:

ced. r=kinetik."
merging dom/media/mediasink/DecodedAudioDataSink.cpp
warning: conflicts while merging dom/media/mediasink/DecodedAudioDataSink.cpp! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use hg resolve and hg graft --continue)
I can apply the patch to today's Aurora correctly. Which repo did you apply the patch to?
Flags: needinfo?(cbook)
(In reply to JW Wang [:jwwang] from comment #18)
> I can apply the patch to today's Aurora correctly. Which repo did you apply
> the patch to?

to aurora (bascially tried to uplift hg graft the commit from comment #13)
Flags: needinfo?(cbook)
Weird... Can you try |hg qimport -P https://hg.mozilla.org/mozilla-central/rev/ffdf364dd295|?
Per comment 3 and comment 8, it looks like sometimes windows decoder produces poisoned AudioData::mAudioData/mFrames values and causes crashes. It is hard to debug this issue since the repro rate is very low.

Hi jya,
Do you have any advice to debug abnormal AudioData::mAudioData/mFrames values?
Flags: needinfo?(jyavenard)
what do you mean by abnormal or poisoned audio data?

I personally never seen this.

The number of frames set in the AudioData in the windows audio decoder is based on the size of the decoded audio buffered returned; that is length / sizeof(int16_t) / number_of_channels: so it can't ever be abnormal or invalid.

Additionally, on YouTube with 46 and 47 you should always get Opus audio, so the WMF audio decoder isn't involved.
Flags: needinfo?(jyavenard)
In this bug (https://hg.mozilla.org/mozilla-central/rev/ffdf364dd295), we skip audio samples when the rate or channel count doesn't match the ones specified in the metadata to avoid the crash in https://crash-stats.mozilla.com/report/index/f1b348ec-ad87-4147-9a46-1eb392160203.

I don't know the cause of the crash, but this band-aid patch did work successfully to prevent crash.

I suspect AudioData::mFrames is a very large number to produce an invalid address from (AudioData::mAudioData + some offset) and cause EXCEPTION_ACCESS_VIOLATION_READ.
(In reply to JW Wang [:jwwang] from comment #24)
> In this bug (https://hg.mozilla.org/mozilla-central/rev/ffdf364dd295), we
> skip audio samples when the rate or channel count doesn't match the ones
> specified in the metadata to avoid the crash in
> https://crash-stats.mozilla.com/report/index/f1b348ec-ad87-4147-9a46-
> 1eb392160203.
> 
> I don't know the cause of the crash, but this band-aid patch did work
> successfully to prevent crash.
> 
> I suspect AudioData::mFrames is a very large number to produce an invalid
> address from (AudioData::mAudioData + some offset) and cause
> EXCEPTION_ACCESS_VIOLATION_READ.

I don't see how that's possible seeing that as mentioned, mFrames is size_of_(mAudioData) / 2 / channels

so mAudioData + x where x < mFrames will always be within bound.
Flags: needinfo?(jwwang)
You need to log in before you can comment on or make changes to this bug.