Closed Bug 867174 Opened 8 years ago Closed 8 years ago

WebAudio divide-by-zero crash [@mozilla::dom::AudioBufferSourceNodeEngine::UpdateSampleRateIfNeeded]

Categories

(Core :: Web Audio, defect)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: posidron, Assigned: ehsan)

References

Details

(Keywords: crash, testcase)

Crash Data

Attachments

(4 files, 1 obsolete file)

Attached file testcase
The stack of this crash looks identical as the one in bug 867089. Though, frame #9 shows a different call. The testcase in bug 867089 is not reproducible anymore.

    if (ShouldResample()) {
      SpeexResamplerState* resampler = Resampler(mChannels);

Tested with m-i changeset: 130337:5447d49a2c6f
Attached file callstack
On Windows: bp-0c0dcd4d-2d0a-4702-a11d-d6fdc2130430.
Crash Signature: [@ mozalloc_abort(char const*) | NS_DebugBreak | mozilla::dom::AudioBufferSourceNodeEngine::Resampler(unsigned int)] [@ update_filter ]
OS: Mac OS X → All
Hardware: x86_64 → All
(In reply to Christoph Diehl [:cdiehl] from comment #0)
> Though, frame #9 shows a different call.
It seems the same call, mozilla::dom::AudioBufferSourceNodeEngine::UpdateSampleRateIfNeeded.
For me, it's a duplicate of bug 867089.
Different line number
The following two frames are different:

    #9 0x104cbae87 in mozilla::dom::AudioBufferSourceNodeEngine::UpdateSampleRateIfNeeded AudioBufferSourceNode.cpp:322
    #10 0x104cba73c in mozilla::dom::AudioBufferSourceNodeEngine::ProduceAudioBlock AudioBufferSourceNode.cpp:348
No this is not a dupe.
Blocks: webaudio
Here we're failing to set the proper channel count because playbackRate is set to something other than 1 before we have seen the buffer.
Hrm, the protection added in bug 867089 is not enough.  I have a better patch.
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #743835 - Flags: review?(paul)
Attachment #743836 - Flags: review?(paul)
Attachment #743835 - Flags: review?(paul) → review+
Attachment #743836 - Flags: review?(paul) → review+
https://hg.mozilla.org/mozilla-central/rev/ad407adf94f2
https://hg.mozilla.org/mozilla-central/rev/9351bfd9d8c5
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
This and a bunch of others have been backed out for causing permaorange Android mochitest-2, which resulted in the closure of multiple trees, since it had been merged all over the place :-( Had to back out more than the likely cause (bug 867174) due to conflicts & incorrect disabling of Android tests.

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1bbbd51c40d0
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1c00d9d273c

Example failures:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=6402e13dc9ba&jobname=android.*mochitest-2
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [leave open]
OK, I know what's happening here.  On x86 and x86-64, the result of conversion of +INF and -INF to an integer type is different than on ARM, which makes ComputeFinalOutSampleRate() return a different value on these architectures.  What we need here is a sane way that can cast floating point values to integer values with predictable results on all architectures.
Whiteboard: [leave open]
Blocks: 865642
Comment on attachment 745332 [details] [diff] [review]
Part 2: Protect against invalid sample rates a bit harder

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

r+ with comments addressed.

::: content/media/webaudio/AudioBufferSourceNode.cpp
@@ +301,2 @@
>    {
> +    if (mPlaybackRate < 0 || mPlaybackRate != mPlaybackRate) {

You meant <=, right? We don't protect against divide-by-zero error, here.

@@ +301,5 @@
>    {
> +    if (mPlaybackRate < 0 || mPlaybackRate != mPlaybackRate) {
> +      mPlaybackRate = 1.0f;
> +    }
> +    if (mDopplerShift < 0 || mDopplerShift != mDopplerShift) {

Same here.

@@ +325,5 @@
>      }
>  
> +    // Make sure the playback rate and the doppler shift are something
> +    // our resampler can work with.
> +    if (ComputeFinalOutSampleRate() == 0) {

I remember I thought at some point we should cap the playbackRate to a value, but I can't remember why. This is probably okay.
Attachment #745332 - Flags: review?(paul) → review+
Yes, I meant <=.  Sorry!
Blocks: 869257
https://hg.mozilla.org/mozilla-central/rev/f51b726ef31d
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Mass moving Web Audio bugs to the Web Audio component.  Filter on duckityduck.
Component: Video/Audio → Web Audio
Depends on: 1353246
You need to log in before you can comment on or make changes to this bug.