[webkit-reviews] review granted: [Bug 235529] ASSERTION FAILED: m_processCallback in WebCore::AudioWorkletProcessor::process : [Attachment 450161] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 235529: ASSERTION FAILED: m_processCallback in
WebCore::AudioWorkletProcessor::process
https://bugs.webkit.org/show_bug.cgi?id=235529

Attachment 450161: Patch

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




--- Comment #23 from Darin Adler <darin at apple.com> ---
Comment on attachment 450161
  --> https://bugs.webkit.org/attachment.cgi?id=450161
Patch

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

> Source/WTF/wtf/WeakHashSet.h:101
> +	   return m_set.add(*static_cast<const
T&>(value).weakPtrFactory().template createWeakPtr<T>(const_cast<U&>(value),
enableWeakPtrThreadingAssertions).m_impl);

This seems like such a long argument name. Is there any shorter name that would
be sufficiently unambiguous? I might just call the local "assertions" and not
worry that was too confusing. After all we still have the strong typing of
EnableWeakPtrThreadingAssertions that will follow us around.

> Source/WebCore/Modules/webaudio/AudioWorkletGlobalScope.cpp:216
> +    Locker locker { m_processorsLock };
> +    m_processors.forEach([&](auto& processor) {
> +	   visitor.addOpaqueRoot(&processor);
> +    });

If I understand correctly, it’s generally bad for performance to have to take
locks during GC marking. Would be nice to have some lock-free safe way to do
this.

> Source/WebCore/Modules/webaudio/AudioWorkletGlobalScope.h:61
> +    void processorIsNoLongerNeeded(AudioWorkletProcessor&);
> +
> +    void visitProcessors(JSC::AbstractSlotVisitor&);

Not sure these need to be in separate paragraphs.

> Source/WebCore/Modules/webaudio/AudioWorkletProcessor.h:37
> +#include <wtf/Lock.h>

Any way to avoid needing this new include in the header? I don’t see any new
use of Lock below.