[webkit-reviews] review granted: [Bug 223640] Extend WebAudio heap allocation assertions to cover the pre & post-rendering phases : [Attachment 424069] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Mar 23 17:08:43 PDT 2021


Sam Weinig <sam at webkit.org> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 223640: Extend WebAudio heap allocation assertions to cover the pre &
post-rendering phases
https://bugs.webkit.org/show_bug.cgi?id=223640

Attachment 424069: Patch

https://bugs.webkit.org/attachment.cgi?id=424069&action=review




--- Comment #4 from Sam Weinig <sam at webkit.org> ---
Comment on attachment 424069
  --> https://bugs.webkit.org/attachment.cgi?id=424069
Patch

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

> Source/WebCore/Modules/webaudio/AudioDestinationNode.cpp:60
> +    ForbidMallocUseForCurrentThreadScope forbidMallocUse;

I think giving a comment for these kind of things would be useful, as to
someone unfamiliar with this code / audio thread needs, it is a bit of mystery
why malloc would be forbidden, or explain the reason a bit in the variable
name.

> Source/WebCore/Modules/webaudio/AudioNodeInput.cpp:106
> +    DisableMallocRestrictionsForCurrentThreadScope
disableMallocRestrictions;

Same as above, add a comment or improve the variable name, with why we are
allowing malloc here?

> Source/WebCore/Modules/webaudio/AudioNodeOutput.cpp:71
> +    DisableMallocRestrictionsForCurrentThreadScope
disableMallocRestrictions;

Same as above, add a comment or improve the variable name, with why we are
allowing malloc here?

> Source/WebCore/Modules/webaudio/AudioWorkletGlobalScope.cpp:191
> +	   DisableMallocRestrictionsForCurrentThreadScope
disableMallocRestrictions;

Same as above, add a comment or improve the variable name, with why we are
allowing malloc here?

> Source/WebCore/Modules/webaudio/BaseAudioContext.cpp:874
> +    // FIXME: This may cause heap allocations on the audio thread.
> +    DisableMallocRestrictionsForCurrentThreadScope
disableMallocRestrictions;

This FIXME is a bit misleading. DisableMallocRestrictionsForCurrentThreadScope
won't *cause* the allocations, it only allows them. Adding an explanation as to
why they are being allowed would be useful though. (Same for all the other
ones).


More information about the webkit-reviews mailing list