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.
Created attachment 376778 [details] Patch
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.
(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)]() {
(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)]() {
> > 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.
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).
I'll also do a follow-up to remove m_weakThis.
Comment on attachment 376778 [details] Patch Clearing flags on attachment: 376778 Committed r249000: <https://trac.webkit.org/changeset/249000>
All reviewed patches have been landed. Closing bug.
<rdar://problem/54591668>