Created attachment 373901 [details] Patch

MediaPlayerPrivateMediaFoundation has a fundamental bug of ref counting. If the original object and all its WeakPtr is created and destructed by locking a single mutex, it is safe to be done even under multithreads. Unfortunately, MediaPlayerPrivateMediaFoundation is using m_mutexListeners and m_mutex, then they can't prevent data races properly. It doesn't make any sense to replace calling makeWeakPtr in a background thread with copying WeakPtr in a background thread. Because m_weakThis and MediaPlayerPrivateMediaFoundation can be destructed in the main thread at the same time of copying m_weakThis. BTW, MediaPlayerPrivateMediaFoundation is used only by WinCairo port. And this ref counter issue is already in my lengthy to-do list. Feel free to assign this bug to me.

(In reply to Fujii Hironori from comment #2) > MediaPlayerPrivateMediaFoundation has a fundamental bug of ref > counting. > > If the original object and all its WeakPtr is created and > destructed by locking a single mutex, it is safe to be done even > under multithreads. > > Unfortunately, MediaPlayerPrivateMediaFoundation is using > m_mutexListeners and m_mutex, then they can't prevent data races > properly. We do not want to allow using WeakPtr from various threads because it is extremely difficult to do properly and has been a big source of hard-to-debugs thread-safety bugs. For this reason, I am working on adding threading assertions to WeakPtr and refactoring the existing code to allow for such assertions. I have not looked much into whether or not m_mutexListeners / m_mutex would make things safe here. > > It doesn't make any sense to replace calling makeWeakPtr in a > background thread with copying WeakPtr in a background thread. > Because m_weakThis and MediaPlayerPrivateMediaFoundation can be > destructed in the main thread at the same time of copying > m_weakThis. The MediaPlayerPrivateMediaFoundation destructor calls notifyDeleted(), which nulls out m_mediaPlayer in MediaPlayerPrivateMediaFoundation::AsyncCallback. After that AsyncCallback is not longer able to call functions like endGetEvent() on the MediaPlayerPrivateMediaFoundation on the background thread. I therefore believe that my patch which relies on MediaPlayerPrivateMediaFoundation::m_weakThis in functions like endGetEvent() is safe. We do need to make sure that MediaPlayerPrivateMediaFoundation is still alive after returning to the main thread via callOnMainThread(), which is what weakThis is used for. In any event, if your claim was right, then the previous code would have been equally wrong since it called makeWeakPtr() with a potentially dead |this| pointer. I therefore do not think my patch would regress anything. My patch does make sure we only call makeWeakPtr() on the main thread though since the object in question is constructed / destroyed on the main thread.

Comment on attachment 373901 [details] Patch Fujii, please feel free to improve things in a follow-up. In the mean time, this should allow me to enable the WeakPtr threading assertions.

Comment on attachment 373901 [details] Patch Clearing flags on attachment: 373901 Committed r247354: <https://trac.webkit.org/changeset/247354>

All reviewed patches have been landed. Closing bug.

<rdar://problem/52958100>