Closed Bug 867089 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: Web Audio, defect)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: posidron, Assigned: padenot)

References

Details

(Keywords: crash, testcase)

Crash Data

Attachments

(4 files, 1 obsolete file)

Attached file 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
Attached file callstack
Assignee: nobody → paul
On Windows: bp-6d206d97-c7b5-4ffd-ae17-2ae212130430.
Assignee: paul → nobody
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
Assignee: nobody → paul
Status: NEW → ASSIGNED
oops.
Attachment #743562 - Flags: review?(ehsan)
Attached patch Test. (obsolete) — Splinter Review
and a crashtest.
Attachment #743577 - Flags: review?(ehsan)
Attached patch Test.Splinter Review
Sorry, forgot to `hg add`.
Attachment #743585 - Flags: review?(ehsan)
Attachment #743577 - Attachment is obsolete: true
Attachment #743577 - Flags: review?(ehsan)
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.
Attachment #743562 - Flags: review?(ehsan) → review+
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.
Attachment #743585 - Flags: review?(ehsan) → review+
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?
Yes, consistently. Should I put a |setTimeout| before finishing, for good measure?
(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!
That's just a typo in the test, will reland now.
Blocks: webaudio
https://hg.mozilla.org/mozilla-central/rev/79220f949406
https://hg.mozilla.org/mozilla-central/rev/a0d7d78facd4
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Mass moving Web Audio bugs to the Web Audio component.  Filter on duckityduck.
Component: Video/Audio → Web Audio
You need to log in before you can comment on or make changes to this bug.