Bug 217442 – Constructing a AudioWorkletNode should construct an AudioWorkletProcessor on the Worklet thread

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

Constructing a AudioWorkletNode should construct an AudioWorkletProcessor on the Worklet thread:
- https://www.w3.org/TR/webaudio/#AudioWorkletNode-constructors (Step 13)

[Chris Dumez]

Created attachment 410773 [details]
Patch

[Chris Dumez]

Created attachment 410774 [details]
Patch

[Geoffrey Garen]

Comment on attachment 410774 [details]
Patch

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

> Source/WebCore/Modules/webaudio/AudioWorkletNode.cpp:144
>  AudioWorkletNode::~AudioWorkletNode()
>  {
> +    ASSERT(isMainThread());
> +    if (m_processor) {
> +        if (auto* workletProxy = context().audioWorklet().proxy())
> +            workletProxy->postTaskForModeToWorkletGlobalScope([m_processor = WTFMove(m_processor)](ScriptExecutionContext&) { }, WorkerRunLoop::defaultMode());
> +    }
>      uninitialize();
>  }
>  
> +void AudioWorkletNode::setProcessor(RefPtr<AudioWorkletProcessor>&& processor)
> +{
> +    ASSERT(!isMainThread());
> +    m_processor = WTFMove(processor);
> +}

This doesn't look right to me. m_processor is set on the worklet thread but checked on the main thread, which seems like a data race. Maybe it's not a real problem in practice because something guarantees synchronization before destruction. But I'd prefer not to have think that hard.

Can we have the worklet thread do the construction and then have the main thread do the assignment?

[Chris Dumez]

(In reply to Geoffrey Garen from comment #3)
> Comment on attachment 410774 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=410774&action=review
> 
> > Source/WebCore/Modules/webaudio/AudioWorkletNode.cpp:144
> >  AudioWorkletNode::~AudioWorkletNode()
> >  {
> > +    ASSERT(isMainThread());
> > +    if (m_processor) {
> > +        if (auto* workletProxy = context().audioWorklet().proxy())
> > +            workletProxy->postTaskForModeToWorkletGlobalScope([m_processor = WTFMove(m_processor)](ScriptExecutionContext&) { }, WorkerRunLoop::defaultMode());
> > +    }
> >      uninitialize();
> >  }
> >  
> > +void AudioWorkletNode::setProcessor(RefPtr<AudioWorkletProcessor>&& processor)
> > +{
> > +    ASSERT(!isMainThread());
> > +    m_processor = WTFMove(processor);
> > +}
> 
> This doesn't look right to me. m_processor is set on the worklet thread but
> checked on the main thread, which seems like a data race. Maybe it's not a
> real problem in practice because something guarantees synchronization before
> destruction. But I'd prefer not to have think that hard.
> 
> Can we have the worklet thread do the construction and then have the main
> thread do the assignment?

I don't think your proposal is better for the long term since AudioWorkletNode::process() will use m_processor on the rendering thread to do audio processing.

How about I add a Lock to synchronize?

[Chris Dumez]

Created attachment 410789 [details]
Patch

[Geoffrey Garen]

Comment on attachment 410789 [details]
Patch

r=me

[Geoffrey Garen]

> I don't think your proposal is better for the long term since
> AudioWorkletNode::process() will use m_processor on the rendering thread to
> do audio processing.
> 
> How about I add a Lock to synchronize?

I guess I don't have the full context, so I'll try to evaluate in future patches.

If AudioWorkletNode::process() might need m_processor right away, and can't wait for AudioWorklet::createProcessor() to call back to the main thread, it sounds like the right solution might be for AudioWorklet::createProcessor to wait synchronously on a semaphore until the processor has been created. Otherwise, a lock may have the same issues as a post-back to the main thread.

[EWS]

Committed r268161: <https://trac.webkit.org/changeset/268161>

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

[Radar WebKit Bug Importer]

<rdar://problem/70070509>

[Fujii Hironori]

Comment on attachment 410789 [details]
Patch

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

> Source/WebCore/dom/ScriptExecutionContext.cpp:211
> +#if ENABLE(WEB_AUDIO)

AppleWin and WinCairo debug builder can't compile this. I'm suprised by using #if in ASSERT condition.

https://build.webkit.org/builders/Apple%20Win%2010%20Debug%20%28Build%29/builds/16462

> C:\cygwin\home\buildbot\worker\win10-debug\build\Source\WebCore\dom/ScriptExecutionContext.cpp(214,1): error C2121: '#': invalid character: possibly the result of a macro expansion (compiling source file C:\cygwin\home\buildbot\worker\win10-debug\build\WebKitBuild\Debug\DerivedSources\WebCore\unified-sources\UnifiedSource-be65d27a-16.cpp) [C:\cygwin\home\buildbot\worker\win10-debug\build\WebKitBuild\Debug\Source\WebCore\WebCore.vcxproj]

[Chris Dumez]

(In reply to Fujii Hironori from comment #10)
> Comment on attachment 410789 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=410789&action=review
> 
> > Source/WebCore/dom/ScriptExecutionContext.cpp:211
> > +#if ENABLE(WEB_AUDIO)
> 
> AppleWin and WinCairo debug builder can't compile this. I'm suprised by
> using #if in ASSERT condition.
> 
> https://build.webkit.org/builders/Apple%20Win%2010%20Debug%20%28Build%29/
> builds/16462
> 
> > C:\cygwin\home\buildbot\worker\win10-debug\build\Source\WebCore\dom/ScriptExecutionContext.cpp(214,1): error C2121: '#': invalid character: possibly the result of a macro expansion (compiling source file C:\cygwin\home\buildbot\worker\win10-debug\build\WebKitBuild\Debug\DerivedSources\WebCore\unified-sources\UnifiedSource-be65d27a-16.cpp) [C:\cygwin\home\buildbot\worker\win10-debug\build\WebKitBuild\Debug\Source\WebCore\WebCore.vcxproj]

Ok. Looking into fixing it.

[Chris Dumez]

(In reply to Chris Dumez from comment #11)
> (In reply to Fujii Hironori from comment #10)
> > Comment on attachment 410789 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=410789&action=review
> > 
> > > Source/WebCore/dom/ScriptExecutionContext.cpp:211
> > > +#if ENABLE(WEB_AUDIO)
> > 
> > AppleWin and WinCairo debug builder can't compile this. I'm suprised by
> > using #if in ASSERT condition.
> > 
> > https://build.webkit.org/builders/Apple%20Win%2010%20Debug%20%28Build%29/
> > builds/16462
> > 
> > > C:\cygwin\home\buildbot\worker\win10-debug\build\Source\WebCore\dom/ScriptExecutionContext.cpp(214,1): error C2121: '#': invalid character: possibly the result of a macro expansion (compiling source file C:\cygwin\home\buildbot\worker\win10-debug\build\WebKitBuild\Debug\DerivedSources\WebCore\unified-sources\UnifiedSource-be65d27a-16.cpp) [C:\cygwin\home\buildbot\worker\win10-debug\build\WebKitBuild\Debug\Source\WebCore\WebCore.vcxproj]
> 
> Ok. Looking into fixing it.

Follow-up build fix in <https://trac.webkit.org/changeset/268164>.