Constructing a AudioWorkletNode should construct an AudioWorkletProcessor on the Worklet thread: - https://www.w3.org/TR/webaudio/#AudioWorkletNode-constructors (Step 13)
Created attachment 410773 [details] Patch
Created attachment 410774 [details] Patch
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?
(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?
Created attachment 410789 [details] Patch
Comment on attachment 410789 [details] Patch r=me
> 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.
Committed r268161: <https://trac.webkit.org/changeset/268161> All reviewed patches have been landed. Closing bug and clearing flags on attachment 410789 [details].
<rdar://problem/70070509>
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]
(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.
(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>.