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

Status: RESOLVED FIXED

(Core :: Web Audio, defect)

Description

[Christoph Diehl [:posidron]]

Attachment: 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

Comment 1

[Christoph Diehl [:posidron]]

Attachment: callstack

Comment 2

[Scoobidiver (away)]

On Windows: bp-0c0dcd4d-2d0a-4702-a11d-d6fdc2130430.

Comment 3

[Scoobidiver (away)]

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

Comment 4

[Christoph Diehl [:posidron]]

Different line number

Comment 5

[Christoph Diehl [:posidron]]

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

Comment 6

[(no longer active)]

No this is not a dupe.

Comment 7

[(no longer active)]

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.

Comment 8

[(no longer active)]

Hrm, the protection added in bug 867089 is not enough.  I have a better patch.

Comment 9

[(no longer active)]

Attachment: Part 1: Deliver the correct channel

Comment 10

[(no longer active)]

Attachment: Part 2: Protect against invalid sample rates a bit harder

Comment 11

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ad407adf94f2
https://hg.mozilla.org/integration/mozilla-inbound/rev/9351bfd9d8c5

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ad407adf94f2
https://hg.mozilla.org/mozilla-central/rev/9351bfd9d8c5

Comment 13

[Ed Morley [:emorley]]

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

Comment 14

[(no longer active)]

https://tbpl.mozilla.org/?tree=Try&rev=e6b841ac2bb6

Comment 15

[(no longer active)]

With only part 1: https://tbpl.mozilla.org/?tree=Try&rev=fd8fb54cbc12

Comment 16

[(no longer active)]

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.

Comment 17

[(no longer active)]

Comment on attachment 743835 [details] [diff] [review]
Part 1: Deliver the correct channel

https://hg.mozilla.org/integration/mozilla-inbound/rev/ba4cdf4a1b51

Comment 18

[(no longer active)]

Attachment: Part 2: Protect against invalid sample rates a bit harder

This should fix it.

Mobile: https://tbpl.mozilla.org/?tree=Try&rev=962bab5eadab
Desktop: https://tbpl.mozilla.org/?tree=Try&rev=b8481293f3e3

Comment 19

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/ba4cdf4a1b51

Comment 20

[Paul Adenot (:padenot)]

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.

Comment 21

[(no longer active)]

Yes, I meant <=.  Sorry!

Comment 22

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f51b726ef31d

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f51b726ef31d

Comment 24

[(no longer active)]

Mass moving Web Audio bugs to the Web Audio component.  Filter on duckityduck.