867089 - WebAudio divide-by-zero crash [mozilla::dom::AudioBufferSourceNodeEngine::Resampler]

Status: RESOLVED FIXED

(Core :: Web Audio, defect)

Description

[Christoph Diehl [:posidron]]

Attachment: testcase

content/media/webaudio/AudioBufferSourceNode.cpp:129

      mResampler = speex_resampler_init(mChannels, mSampleRate,
*                                       ComputeFinalOutSampleRate(),
                                        SPEEX_RESAMPLER_QUALITY_DEFAULT,
                                        nullptr);

Tested with m-i changeset: 130174:ea5490a3bca7

Comment 1

[Christoph Diehl [:posidron]]

Attachment: callstack

Comment 2

[Scoobidiver (away)]

On Windows: bp-6d206d97-c7b5-4ffd-ae17-2ae212130430.

Comment 3

[Paul Adenot (:padenot)]

Attachment: Validate the playbackRate before using it.

oops.

Comment 4

[Paul Adenot (:padenot)]

Attachment: Test.

and a crashtest.

Comment 5

[Paul Adenot (:padenot)]

Attachment: Test.

Sorry, forgot to `hg add`.

Comment 6

[(no longer active)]

Comment on attachment 743562 [details] [diff] [review]
Validate the playbackRate before using it.

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

r=me minus the cubeb changes which don't seem intentional.

::: content/media/webaudio/AudioBufferSourceNode.cpp
@@ +11,5 @@
>  #include "AudioNodeStream.h"
>  #include "AudioDestinationNode.h"
>  #include "PannerNode.h"
>  #include "speex/speex_resampler.h"
> +#include "WebAudioUtils.h"

You shouldn't need this #include here.

Comment 7

[(no longer active)]

Comment on attachment 743585 [details] [diff] [review]
Test.

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

::: content/media/webaudio/test/test_bug867089.html
@@ +27,5 @@
> +  source2.playbackRate.value = -1.0;
> +  source2.connect(ctx.destination);
> +  source2.start(0);
> +
> +  var source2 = ctx.createBufferSource();

Nit: source3.

Comment 8

[(no longer active)]

Actually, you're not waiting for the graph to play back in the test, so it may never get enough time to render before being paused when we go and run the next test.  Does this test crash without your patch?

Comment 9

[Paul Adenot (:padenot)]

Yes, consistently. Should I put a |setTimeout| before finishing, for good measure?

Comment 10

[(no longer active)]

(In reply to comment #9)
> Yes, consistently. Should I put a |setTimeout| before finishing, for good
> measure?

Hmm, no, if the test crashes without your patches, then it's good!

Comment 11

[Paul Adenot (:padenot)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ab9c9d0819a2
https://hg.mozilla.org/integration/mozilla-inbound/rev/5447d49a2c6f

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Backed out for test failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b68073547c0

https://tbpl.mozilla.org/php/getParsedLog.php?id=22414207&tree=Mozilla-Inbound

Comment 13

[(no longer active)]

That's just a typo in the test, will reland now.

Comment 14

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/79220f949406
https://hg.mozilla.org/integration/mozilla-inbound/rev/a0d7d78facd4

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/79220f949406
https://hg.mozilla.org/mozilla-central/rev/a0d7d78facd4

Comment 16

[(no longer active)]

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