Bug 200924 – Fix unsafe usage of MediaStreamTrackPrivate from background thread in MediaStreamTrackPrivate::audioSamplesAvailable()

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

Fix unsafe usage of MediaStreamTrackPrivate from background thread in MediaStreamTrackPrivate::audioSamplesAvailable(). It may ref |this| on the background thread while the destructor is already running on the main thread, which could lead to a double free.

[Chris Dumez]

Created attachment 376778 [details]
Patch

[youenn fablet]

Comment on attachment 376778 [details]
Patch

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

> Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.cpp:78
>      m_source->removeObserver(*this);

We should probably unregister MediaStreamTrackPrivate when endTrack is called since this is no longer useful to receive notifications from the source.
This would make the makeRef safe.

If we go with the weakThis approach, I would change MediaStreamTrackPrivate to RefCounted.
This way, we would remove the temptation to do some refs from the background thread with the new assertions.

[Chris Dumez]

(In reply to youenn fablet from comment #2)
> Comment on attachment 376778 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=376778&action=review
> 
> > Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.cpp:78
> >      m_source->removeObserver(*this);
> 
> We should probably unregister MediaStreamTrackPrivate when endTrack is
> called since this is no longer useful to receive notifications from the
> source.
> This would make the makeRef safe.
> 
> If we go with the weakThis approach, I would change MediaStreamTrackPrivate
> to RefCounted.
> This way, we would remove the temptation to do some refs from the background
> thread with the new assertions.

Would you mind taking over since you are more familiar with this code? I went with what I felt I was confident about given my current understanding of the code.

Note that there are other cases that need to be reviewed as well:
Source/WebCore/platform/mediastream/CaptureDeviceManager.cpp:    callOnMainThread([weakThis = makeWeakPtr(*this)] {
Source/WebCore/platform/mediastream/MediaStreamPrivate.cpp:    callOnMainThread([weakThis = makeWeakPtr(*this), function = WTFMove(function)] {
Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.cpp:        callOnMainThread([this, protectedThis = makeRef(*this)] {
Source/WebCore/platform/mediastream/RealtimeMediaSource.cpp:    callOnMainThread([protectedThis = makeRef(*this), function = WTFMove(function)] {
Source/WebCore/platform/mediastream/RealtimeOutgoingAudioSource.cpp:    callOnMainThread([protectedThis = makeRef(*this)]() {
Source/WebCore/platform/mediastream/RealtimeOutgoingVideoSource.cpp:    callOnMainThread([protectedThis = makeRef(*this)]() {
Source/WebCore/platform/mediastream/RealtimeOutgoingVideoSource.cpp:    callOnMainThread([protectedThis = makeRef(*this)]() {

[youenn fablet]

(In reply to Chris Dumez from comment #3)
> (In reply to youenn fablet from comment #2)
> > Comment on attachment 376778 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=376778&action=review
> > 
> > > Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.cpp:78
> > >      m_source->removeObserver(*this);
> > 
> > We should probably unregister MediaStreamTrackPrivate when endTrack is
> > called since this is no longer useful to receive notifications from the
> > source.
> > This would make the makeRef safe.
> > 
> > If we go with the weakThis approach, I would change MediaStreamTrackPrivate
> > to RefCounted.
> > This way, we would remove the temptation to do some refs from the background
> > thread with the new assertions.
> 
> Would you mind taking over since you are more familiar with this code?

OK, will do.

> O went with what I felt I was confident about given my current understanding
> of the code.
> 
> Note that there are other cases that need to be reviewed as well:

Will do as well.

> Source/WebCore/platform/mediastream/CaptureDeviceManager.cpp:   
> callOnMainThread([weakThis = makeWeakPtr(*this)] {
> Source/WebCore/platform/mediastream/MediaStreamPrivate.cpp:   
> callOnMainThread([weakThis = makeWeakPtr(*this), function =
> WTFMove(function)] {
> Source/WebCore/platform/mediastream/MediaStreamTrackPrivate.cpp:       
> callOnMainThread([this, protectedThis = makeRef(*this)] {
> Source/WebCore/platform/mediastream/RealtimeMediaSource.cpp:   
> callOnMainThread([protectedThis = makeRef(*this), function =
> WTFMove(function)] {
> Source/WebCore/platform/mediastream/RealtimeOutgoingAudioSource.cpp:   
> callOnMainThread([protectedThis = makeRef(*this)]() {
> Source/WebCore/platform/mediastream/RealtimeOutgoingVideoSource.cpp:   
> callOnMainThread([protectedThis = makeRef(*this)]() {
> Source/WebCore/platform/mediastream/RealtimeOutgoingVideoSource.cpp:   
> callOnMainThread([protectedThis = makeRef(*this)]() {

[youenn fablet]

> > Source/WebCore/platform/mediastream/CaptureDeviceManager.cpp:   
> > callOnMainThread([weakThis = makeWeakPtr(*this)] {

Safe for CoreAudioCaptureSource, unclear whether safe in case of suspension for AV devices.
We should try to optimise and not callOnMainThread if not needed, use m_weakThis otherwise.

> > Source/WebCore/platform/mediastream/MediaStreamPrivate.cpp:   
> > callOnMainThread([weakThis = makeWeakPtr(*this), function =
> > WTFMove(function)] {

This should be safe as all call sites should be on main thread.
We could do m_weakThis for extra safety.

> > Source/WebCore/platform/mediastream/RealtimeMediaSource.cpp:   
> > callOnMainThread([protectedThis = makeRef(*this), function =
> > WTFMove(function)] {

Should be safe, RealtimeMediaSource is thread-safe refcounted and the call sites are fine as well.
This seems fragile though and extending the lifetime of RealtimeMediaSource is not great.
m_weakThis seems like a good tradeoff.

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

Should be safe, libwebrtc should call this method while holding a ref to it.

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

Ditto.

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

Ditto.

[youenn fablet]

Digging in further, the approach to stop observing the source sooner will not fix the issue in some cases, at least MediaRecorder.
Let's start by fixing the issue as you are proposing in that patch.
I'll do follow-ups (move to RefCounted and sort observing the source sooner in some cases).

[youenn fablet]

I'll also do a follow-up to remove m_weakThis.

[WebKit Commit Bot]

Comment on attachment 376778 [details]
Patch

Clearing flags on attachment: 376778

Committed r249000: <https://trac.webkit.org/changeset/249000>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/54591668>