Bug 173864 – MediaStreamTrackPrivate observers map is used in various thread without protection.

• Status: NEW
• Component: Media

[youenn fablet]

We could either ensure that observers are used on the main thread or add a lock.

[youenn fablet]

Created attachment 313897 [details]
Patch

[Chris Dumez]

Comment on attachment 313897 [details]
Patch

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

Please carefully protect ALL uses of m_observers.

> Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.cpp:101
> +    ASSERT(isMainThread());

This is iterating over m_observers below but not locking, why?

> Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.cpp:114
> +    ASSERT(isMainThread());

This is iterating over m_observers below but not locking, why?

> Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.cpp:171
> +    ASSERT(isMainThread());

This is iterating over m_observers below but not locking, why?

> Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.cpp:179
> +    ASSERT(isMainThread());

This is iterating over m_observers below but not locking, why?

> Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.cpp:199
>      for (auto& observer : m_observers)

This is iterating over m_observers but not locking, why?

> Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.cpp:211
>      for (auto& observer : m_observers)

This is iterating over m_observers but not locking, why?

[Chris Dumez]

Comment on attachment 313897 [details]
Patch

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

> Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.cpp:194
> +        callOnMainThread([protectedThis = makeRef(*this)] {

MediaStreamTrackPrivate is NOT ThreadSafeRefCounted :((

[Chris Dumez]

Comment on attachment 313897 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        MediaStreamTrackPrivate observers map is used in various thread without protection.

typo: threads

s/protection/synchronization

[Chris Dumez]

Comment on attachment 313897 [details]
Patch

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

> Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.cpp:252
>          observer->audioSamplesAvailable(*this, mediaTime, data, description, sampleCount);

This is not a good API contract. Despite your locking, sometimes observers will be notified on the main thread, sometimes on a background thread. This is a bug waiting to happen.
Why cannot be update the call sites of these 2 MediaStreamTrackPrivate methods to make sure MediaStreamTrackPrivate is *always* used on the main thread?

[youenn fablet]

(In reply to Chris Dumez from comment #5)
> Comment on attachment 313897 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=313897&action=review
> 
> > Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.cpp:252
> >          observer->audioSamplesAvailable(*this, mediaTime, data, description, sampleCount);
> 
> This is not a good API contract. Despite your locking, sometimes observers
> will be notified on the main thread, sometimes on a background thread. This
> is a bug waiting to happen.

Agreed.
We need to find a good tradeoff between the desire to optimize and the desire for simple to understand design.
The current model is too complex.

> Why cannot be update the call sites of these 2 MediaStreamTrackPrivate
> methods to make sure MediaStreamTrackPrivate is *always* used on the main
> thread?

We could try to go to the main thread and measure the perf impact.
We could also split the observer contract. One issue is that we might lose some timing order between the observer API calls.

[youenn fablet]

Created attachment 314979 [details]
Patch