Fix potential race in AudioMediaStreamTrackRendererCocoa::pushSamples
Created attachment 442693 [details] Patch
Created attachment 442696 [details] Patch
Comment on attachment 442696 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=442696&action=review > Source/WebCore/platform/mediastream/cocoa/AudioMediaStreamTrackRendererCocoa.cpp:65 > void AudioMediaStreamTrackRendererCocoa::stop() > { > - if (m_dataSource) > - AudioMediaStreamTrackRendererUnit::singleton().removeSource(*m_dataSource); > + if (m_registeredDataSource) > + AudioMediaStreamTrackRendererUnit::singleton().removeSource(*m_registeredDataSource); Should we assert that this is called on the main thread? > Source/WebCore/platform/mediastream/cocoa/AudioMediaStreamTrackRendererCocoa.cpp:80 > + if (m_registeredDataSource) > + m_registeredDataSource->setVolume(volume); Ditto > Source/WebCore/platform/mediastream/cocoa/AudioMediaStreamTrackRendererCocoa.cpp:97 > +void AudioMediaStreamTrackRendererCocoa::setRegisteredDataSource(RefPtr<AudioSampleDataSource>&& source) > +{ Ditto > Source/WebCore/platform/mediastream/cocoa/AudioMediaStreamTrackRendererCocoa.cpp:106 > + if (!m_outputDescription) > + return; > + > + m_registeredDataSource = WTFMove(source); > + if (!m_registeredDataSource) > + return; Shouldn't the order of these be switched so we always update m_registeredDataSource, and e.g. not set it to NULL when there is no source? > Source/WebCore/platform/mediastream/cocoa/AudioMediaStreamTrackRendererCocoa.cpp:108 > +#if !RELEASE_LOG_DISABLED RELEASE_LOG_DISABLED is always defined for PLATFORM(COCOA), so I think this is unnecessary.
(In reply to Eric Carlson from comment #3) > Comment on attachment 442696 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=442696&action=review > > > Source/WebCore/platform/mediastream/cocoa/AudioMediaStreamTrackRendererCocoa.cpp:65 > > void AudioMediaStreamTrackRendererCocoa::stop() > > { > > - if (m_dataSource) > > - AudioMediaStreamTrackRendererUnit::singleton().removeSource(*m_dataSource); > > + if (m_registeredDataSource) > > + AudioMediaStreamTrackRendererUnit::singleton().removeSource(*m_registeredDataSource); > > Should we assert that this is called on the main thread? OK > > Source/WebCore/platform/mediastream/cocoa/AudioMediaStreamTrackRendererCocoa.cpp:80 > > + if (m_registeredDataSource) > > + m_registeredDataSource->setVolume(volume); > > Ditto OK > > Source/WebCore/platform/mediastream/cocoa/AudioMediaStreamTrackRendererCocoa.cpp:97 > > +void AudioMediaStreamTrackRendererCocoa::setRegisteredDataSource(RefPtr<AudioSampleDataSource>&& source) > > +{ > > Ditto OK > > Source/WebCore/platform/mediastream/cocoa/AudioMediaStreamTrackRendererCocoa.cpp:106 > > + if (!m_outputDescription) > > + return; > > + > > + m_registeredDataSource = WTFMove(source); > > + if (!m_registeredDataSource) > > + return; > > Shouldn't the order of these be switched so we always update > m_registeredDataSource, and e.g. not set it to NULL when there is no source? I think it is good to set m_registeredDataSource to nullptr so that we unregister it only once for instance. And we tentatively destroy it as soon as we no longer need it. > > Source/WebCore/platform/mediastream/cocoa/AudioMediaStreamTrackRendererCocoa.cpp:108 > > +#if !RELEASE_LOG_DISABLED > > RELEASE_LOG_DISABLED is always defined for PLATFORM(COCOA), so I think this > is unnecessary. OK
Created attachment 442801 [details] Patch for landing
Committed r285024 (243668@main): <https://commits.webkit.org/243668@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 442801 [details].
<rdar://problem/84803096>