[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

https://crash-stats.mozilla.com/report/list?product=Firefox&range_unit=days&range_value=28&signature=memmove+|+mozilla%3A%3AAudioStream%3A%3AGetUnprocessed#tab-table I seems since 2016012303 So, regression range seems to be https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=10254da3c9da73e834059c1972f2e52b89fafb5f&tochange=5f7c184ccd800b2ed512c23fb609007efd198eaf Suspected: 52b05e612df9 JW Wang — Bug 1240420. Part 1 - move checks of mismatched sample rate or channel numbers to AudioStream. r=kinetik.

Do you know which specific Youtube video causes the crash?

https://crash-stats.mozilla.com/signature/?signature=memmove+%7C+mozilla%3A%3AAudioStream%3A%3AGetUnprocessed It looks like all crashes happen on Windows + AMD64.

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.

Review commit: https://reviewboard.mozilla.org/r/33709/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/33709/

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

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.

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.

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

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.

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?

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

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?

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.

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.