Fix potential race in AudioMediaStreamTrackRendererCocoa::pushSamples

• Component: WebRTC
• Status: RESOLVED FIXED

[youenn fablet]

Fix potential race in AudioMediaStreamTrackRendererCocoa::pushSamples

[youenn fablet]

Created attachment 442693 [details]
Patch

[youenn fablet]

Created attachment 442696 [details]
Patch

[Eric Carlson]

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.

[youenn fablet]

(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

[youenn fablet]

Created attachment 442801 [details]
Patch for landing

[EWS]

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].

[Radar WebKit Bug Importer]

<rdar://problem/84803096>