Bug 223444 – Avoid heap allocations under BiquadDSPKernel::updateCoefficientsIfNecessary()

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Audio

[Chris Dumez]

Avoid heap allocations under BiquadDSPKernel::updateCoefficientsIfNecessary() since this runs on the audio thread.

[Chris Dumez]

Thread 10 Crashed:: offline renderer
0   com.apple.JavaScriptCore            0x00000007b7d88aee 0x7b7d86000 + 10990
1   com.apple.JavaScriptCore            0x00000007b952629b 0x7b7d86000 + 24773275
2   com.apple.JavaScriptCore            0x00000007b7dc6a5f 0x7b7d86000 + 264799
3   com.apple.WebCore                   0x000000079abfe795 WebCore::AudioArray<float>::resize(WTF::Checked<unsigned long, WTF::CrashOnOverflow>) + 133 (AudioArray.h:65)
4   com.apple.WebCore                   0x000000079abfe701 WebCore::AudioArray<float>::AudioArray(unsigned long) + 65 (AudioArray.h:45)
5   com.apple.WebCore                   0x000000079abd7b2d WebCore::AudioArray<float>::AudioArray(unsigned long) + 29 (AudioArray.h:44)
6   com.apple.WebCore                   0x000000079ac41bb1 WebCore::BiquadDSPKernel::updateCoefficientsIfNecessary(unsigned long) + 129 (BiquadDSPKernel.cpp:45)
7   com.apple.WebCore                   0x000000079ac42571 WebCore::BiquadDSPKernel::process(float const*, float*, unsigned long) + 161 (BiquadDSPKernel.cpp:140)
8   com.apple.WebCore                   0x000000079ac43789 WebCore::BiquadProcessor::process(WebCore::AudioBus const*, WebCore::AudioBus*, unsigned long) + 233 (BiquadProcessor.cpp:106)
9   com.apple.WebCore                   0x000000079abc0f3d WebCore::AudioBasicProcessorNode::process(unsigned long) + 253 (AudioBasicProcessorNode.cpp:86)
10  com.apple.WebCore                   0x000000079abdc5be WebCore::AudioNode::processIfNecessary(unsigned long) + 462 (AudioNode.cpp:474)
11  com.apple.WebCore                   0x000000079abdee77 WebCore::AudioNodeOutput::pull(WebCore::AudioBus*, unsigned long) + 407 (AudioNodeOutput.cpp:124)
12  com.apple.WebCore                   0x000000079abdec46 WebCore::AudioNodeInput::sumAllConnections(WebCore::AudioBus*, unsigned long) + 566 (AudioNodeInput.cpp:197)
13  com.apple.WebCore                   0x000000079abd7477 WebCore::AudioNodeInput::pull(WebCore::AudioBus*, unsigned long) + 295 (AudioNodeInput.cpp:225)
14  com.apple.WebCore                   0x000000079abd7015 WebCore::AudioDestinationNode::render(WebCore::AudioBus*, WebCore::AudioBus*, unsigned long, WebCore::AudioIOPosition const&) + 469 (AudioDestinationNode.cpp:94)
15  com.apple.WebCore                   0x000000079ac9e11d WebCore::OfflineAudioDestinationNode::offlineRender() + 877 (OfflineAudioDestinationNode.cpp:164)
16  com.apple.WebCore                   0x000000079acbe213 WebCore::OfflineAudioDestinationNode::startRendering(WTF::CompletionHandler<void (WTF::Optional<WebCore::Exception>&&)>&&)::$_2::operator()() + 35 (OfflineAudioDestinationNode.cpp:103)
17  com.apple.WebCore                   0x000000079acbfbee WTF::Detail::CallableWrapper<WebCore::OfflineAudioDestinationNode::startRendering(WTF::CompletionHandler<void (WTF::Optional<WebCore::Exception>&&)>&&)::$_2, void>::call() + 30 (Function.h:52)

[Chris Dumez]

Created attachment 423603 [details]
Patch

[Peng Liu]

Comment on attachment 423603 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=423603&action=review

> Source/WebCore/Modules/webaudio/BiquadDSPKernel.cpp:61
> +            RELEASE_ASSERT(framesToProcess <= AudioUtilities::renderQuantumSize);

Is this change intentional?

[Darin Adler]

Comment on attachment 423603 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=423603&action=review

>> Source/WebCore/Modules/webaudio/BiquadDSPKernel.cpp:61
>> +            RELEASE_ASSERT(framesToProcess <= AudioUtilities::renderQuantumSize);
> 
> Is this change intentional?

I had assumed it was, and I think it’s a good idea to check this at runtime unconditionally.

[Darin Adler]

Comment on attachment 423603 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=423603&action=review

> Source/WebCore/Modules/webaudio/BiquadDSPKernel.cpp:41
> +static bool hasConstantValue(float* values, size_t framesToProcess)

Could consider returning false if framesToProcess is 0, rather than reading an uninitialized value and then returning true.

[Chris Dumez]

(In reply to Darin Adler from comment #4)
> Comment on attachment 423603 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=423603&action=review
> 
> >> Source/WebCore/Modules/webaudio/BiquadDSPKernel.cpp:61
> >> +            RELEASE_ASSERT(framesToProcess <= AudioUtilities::renderQuantumSize);
> > 
> > Is this change intentional?
> 
> I had assumed it was, and I think it’s a good idea to check this at runtime
> unconditionally.

Yes, definitely intentional as we we get a security bug if this assertion did not hold.

[Chris Dumez]

(In reply to Darin Adler from comment #5)
> Comment on attachment 423603 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=423603&action=review
> 
> > Source/WebCore/Modules/webaudio/BiquadDSPKernel.cpp:41
> > +static bool hasConstantValue(float* values, size_t framesToProcess)
> 
> Could consider returning false if framesToProcess is 0, rather than reading
> an uninitialized value and then returning true.

Will make this change before landing.

[Chris Dumez]

Created attachment 423626 [details]
Patch

[EWS]

Committed r274664: <https://commits.webkit.org/r274664>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 423626 [details].

[Radar WebKit Bug Importer]

<rdar://problem/75584924>