[webkit-reviews] review granted: [Bug 232430] Fix potential race in AudioMediaStreamTrackRendererCocoa::pushSamples : [Attachment 442696] Patch

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has granted youenn fablet
<youennf at gmail.com>'s request for review:
Bug 232430: Fix potential race in
AudioMediaStreamTrackRendererCocoa::pushSamples
https://bugs.webkit.org/show_bug.cgi?id=232430

Attachment 442696: Patch

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




--- Comment #3 from Eric Carlson <eric.carlson at apple.com> ---
Comment on attachment 442696
  --> https://bugs.webkit.org/attachment.cgi?id=442696
Patch

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

>
Source/WebCore/platform/mediastream/cocoa/AudioMediaStreamTrackRendererCocoa.cp
p:65
>  void AudioMediaStreamTrackRendererCocoa::stop()
>  {
> -    if (m_dataSource)
> -	  
AudioMediaStreamTrackRendererUnit::singleton().removeSource(*m_dataSource);
> +    if (m_registeredDataSource)
> +	  
AudioMediaStreamTrackRendererUnit::singleton().removeSource(*m_registeredDataSo
urce);

Should we assert that this is called on the main thread?

>
Source/WebCore/platform/mediastream/cocoa/AudioMediaStreamTrackRendererCocoa.cp
p:80
> +    if (m_registeredDataSource)
> +	   m_registeredDataSource->setVolume(volume);

Ditto

>
Source/WebCore/platform/mediastream/cocoa/AudioMediaStreamTrackRendererCocoa.cp
p:97
> +void
AudioMediaStreamTrackRendererCocoa::setRegisteredDataSource(RefPtr<AudioSampleD
ataSource>&& source)
> +{

Ditto

>
Source/WebCore/platform/mediastream/cocoa/AudioMediaStreamTrackRendererCocoa.cp
p: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.cp
p:108
> +#if !RELEASE_LOG_DISABLED

RELEASE_LOG_DISABLED is always defined for PLATFORM(COCOA), so I think this is
unnecessary.