Fix non thread-safe usage of makeWeakPtr() in MediaPlayerPrivateMediaFoundation

• Component: Media
• Status: RESOLVED FIXED

[Chris Dumez]

Fix non thread-safe usage of makeWeakPtr() in MediaPlayerPrivateMediaFoundation.

[Chris Dumez]

Created attachment 373901 [details]
Patch

[Fujii Hironori]

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.

[Chris Dumez]

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

[Chris Dumez]

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.

[WebKit Commit Bot]

Comment on attachment 373901 [details]
Patch

Clearing flags on attachment: 373901

Committed r247354: <https://trac.webkit.org/changeset/247354>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/52958100>