199700 2019-07-10 20:11:51 -0700 Fix non thread-safe usage of makeWeakPtr() in MediaPlayerPrivateMediaFoundation 2019-07-12 20:04:28 -0700 1 1 1 Unclassified WebKit Media WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED https://bugs.webkit.org/show_bug.cgi?id=199727 https://bugs.webkit.org/show_bug.cgi?id=199777 InRadar P2 Normal --- 199639 1 cdumez cdumez achristensen commit-queue eric.carlson fujii ggaren jer.noble pvollan rniwa webkit-bug-importer oldest_to_newest 1551968 0 cdumez 2019-07-10 20:11:51 -0700 Fix non thread-safe usage of makeWeakPtr() in MediaPlayerPrivateMediaFoundation. 1551971 1 373901 cdumez 2019-07-10 20:21:43 -0700 Created attachment 373901 Patch 1551990 2 fujii 2019-07-10 23:06:21 -0700 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. 1552067 3 cdumez 2019-07-11 08:40:23 -0700 (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. 1552085 4 373901 cdumez 2019-07-11 09:37:12 -0700 Comment on attachment 373901 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. 1552094 5 373901 commit-queue 2019-07-11 10:10:58 -0700 Comment on attachment 373901 Patch Clearing flags on attachment: 373901 Committed r247354: <https://trac.webkit.org/changeset/247354> 1552095 6 commit-queue 2019-07-11 10:11:00 -0700 All reviewed patches have been landed. Closing bug. 1552096 7 webkit-bug-importer 2019-07-11 10:13:33 -0700 <rdar://problem/52958100> 373901 2019-07-10 20:21:43 -0700 2019-07-11 10:10:58 -0700 Patch bug-199700-20190710202142.patch text/plain 5100 cdumez U3VidmVyc2lvbiBSZXZpc2lvbjogMjQ3MzMxCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggY2M3NGRjYzgxYjU3YTkx YWMxNzMwN2Q3YjA2YWZhMjcxZjUxMDIwMC4uYTQyMzFmZWVhZGIwN2EyMDhmNjkwYWE4NDFjNDQ1 NmYxY2RjZTgxOCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDI0IEBACisyMDE5LTA3LTEwICBDaHJp cyBEdW1leiAgPGNkdW1lekBhcHBsZS5jb20+CisKKyAgICAgICAgRml4IG5vbiB0aHJlYWQtc2Fm ZSB1c2FnZSBvZiBtYWtlV2Vha1B0cigpIGluIE1lZGlhUGxheWVyUHJpdmF0ZU1lZGlhRm91bmRh dGlvbgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTk5 NzAwCisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgVGhl IGNvZGUgd2FzIGNhbGxpbmcgbWFrZVdlYWtQdHIoKSBvbiBhIG1haW4tdGhyZWFkIG9iamVjdCwg ZnJvbSBhIGJhY2tncm91bmQgdGhyZWFkLgorICAgICAgICBUaGlzIGlzIG5vdCB0aHJlYWQgc2Fm ZS4gVG8gYWRkcmVzcyB0aGUgaXNzdWUsIHRoaXMgcGF0Y2hlcyBjcmVhdGVzIHRoZSBXZWFrUHRy IGFoZWFkCisgICAgICAgIG9mIHRpbWUsIG9uIHRoZSBtYWluIHRocmVhZC4gCisKKyAgICAgICAg KiBwbGF0Zm9ybS9ncmFwaGljcy93aW4vTWVkaWFQbGF5ZXJQcml2YXRlTWVkaWFGb3VuZGF0aW9u LmNwcDoKKyAgICAgICAgKFdlYkNvcmU6Ok1lZGlhUGxheWVyUHJpdmF0ZU1lZGlhRm91bmRhdGlv bjo6TWVkaWFQbGF5ZXJQcml2YXRlTWVkaWFGb3VuZGF0aW9uKToKKyAgICAgICAgKFdlYkNvcmU6 Ok1lZGlhUGxheWVyUHJpdmF0ZU1lZGlhRm91bmRhdGlvbjo6ZW5kQ3JlYXRlZE1lZGlhU291cmNl KToKKyAgICAgICAgKFdlYkNvcmU6Ok1lZGlhUGxheWVyUHJpdmF0ZU1lZGlhRm91bmRhdGlvbjo6 ZW5kR2V0RXZlbnQpOgorICAgICAgICAoV2ViQ29yZTo6TWVkaWFQbGF5ZXJQcml2YXRlTWVkaWFG b3VuZGF0aW9uOjpDdXN0b21WaWRlb1ByZXNlbnRlcjo6cHJvY2Vzc0lucHV0Tm90aWZ5KToKKyAg ICAgICAgKiBwbGF0Zm9ybS9ncmFwaGljcy93aW4vTWVkaWFQbGF5ZXJQcml2YXRlTWVkaWFGb3Vu ZGF0aW9uLmg6CisgICAgICAgIChXZWJDb3JlOjpNZWRpYVBsYXllclByaXZhdGVNZWRpYUZvdW5k YXRpb246OlRocmVhZFNhZmVXcmFwcGVyOjpjcmVhdGUpOgorICAgICAgICAoV2ViQ29yZTo6TWVk aWFQbGF5ZXJQcml2YXRlTWVkaWFGb3VuZGF0aW9uOjpUaHJlYWRTYWZlV3JhcHBlcjo6d3JhcHBl ZCk6CisgICAgICAgIChXZWJDb3JlOjpNZWRpYVBsYXllclByaXZhdGVNZWRpYUZvdW5kYXRpb246 OlRocmVhZFNhZmVXcmFwcGVyOjpUaHJlYWRTYWZlV3JhcHBlcik6CisKIDIwMTktMDctMTAgIENo cmlzIER1bWV6ICA8Y2R1bWV6QGFwcGxlLmNvbT4KIAogICAgICAgICBTdG9wIHVzaW5nIEdlbmVy aWNUYXNrUXVldWUgZnJvbSBtdWx0aXBsZSB0aHJlYWRzCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2Vi Q29yZS9wbGF0Zm9ybS9ncmFwaGljcy93aW4vTWVkaWFQbGF5ZXJQcml2YXRlTWVkaWFGb3VuZGF0 aW9uLmNwcCBiL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNzL3dpbi9NZWRpYVBsYXll clByaXZhdGVNZWRpYUZvdW5kYXRpb24uY3BwCmluZGV4IDFiNGM1MzlkNDgwNTcxMjFlYmNkNTkw MWViNGUyM2U5NTg1MjFhMjIuLmFhYzA1NzBlMGM1MjFiOTQyMzAwN2FmZDU1NDk4YWVkYzI5ODJl MGUgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNzL3dpbi9NZWRp YVBsYXllclByaXZhdGVNZWRpYUZvdW5kYXRpb24uY3BwCisrKyBiL1NvdXJjZS9XZWJDb3JlL3Bs YXRmb3JtL2dyYXBoaWNzL3dpbi9NZWRpYVBsYXllclByaXZhdGVNZWRpYUZvdW5kYXRpb24uY3Bw CkBAIC01OCw3ICs1OCw4IEBAIHN0YXRpYyBjb25zdCBkb3VibGUgdGVuTWVnYWhlcnR6ID0gMTAw MDAwMDA7CiBuYW1lc3BhY2UgV2ViQ29yZSB7CiAKIE1lZGlhUGxheWVyUHJpdmF0ZU1lZGlhRm91 bmRhdGlvbjo6TWVkaWFQbGF5ZXJQcml2YXRlTWVkaWFGb3VuZGF0aW9uKE1lZGlhUGxheWVyKiBw bGF5ZXIpIAotICAgIDogbV9wbGF5ZXIocGxheWVyKQorICAgIDogbV93ZWFrVGhpcyhtYWtlV2Vh a1B0cih0aGlzKSkKKyAgICAsIG1fcGxheWVyKHBsYXllcikKICAgICAsIG1fdmlzaWJsZShmYWxz ZSkKICAgICAsIG1fbG9hZGluZ1Byb2dyZXNzKGZhbHNlKQogICAgICwgbV9wYXVzZWQodHJ1ZSkK QEAgLTQyNSw3ICs0MjYsNyBAQCBib29sIE1lZGlhUGxheWVyUHJpdmF0ZU1lZGlhRm91bmRhdGlv bjo6ZW5kQ3JlYXRlZE1lZGlhU291cmNlKElNRkFzeW5jUmVzdWx0KiBhcwogICAgIGhyID0gYXN5 bmNSZXN1bHQtPkdldFN0YXR1cygpOwogICAgIG1fbG9hZGluZ1Byb2dyZXNzID0gU1VDQ0VFREVE KGhyKTsKIAotICAgIGNhbGxPbk1haW5UaHJlYWQoW3dlYWtQdHIgPSBtYWtlV2Vha1B0cigqdGhp cyldIHsKKyAgICBjYWxsT25NYWluVGhyZWFkKFt3ZWFrUHRyID0gbV93ZWFrVGhpc10gewogICAg ICAgICBpZiAoIXdlYWtQdHIpCiAgICAgICAgICAgICByZXR1cm47CiAgICAgICAgIHdlYWtQdHIt Pm9uQ3JlYXRlZE1lZGlhU291cmNlKCk7CkBAIC00NTQsNyArNDU1LDcgQEAgYm9vbCBNZWRpYVBs YXllclByaXZhdGVNZWRpYUZvdW5kYXRpb246OmVuZEdldEV2ZW50KElNRkFzeW5jUmVzdWx0KiBh c3luY1Jlc3VsdCkKIAogICAgIHN3aXRjaCAobWVkaWFFdmVudFR5cGUpIHsKICAgICBjYXNlIE1F U2Vzc2lvblRvcG9sb2d5U2V0OiB7Ci0gICAgICAgIGNhbGxPbk1haW5UaHJlYWQoW3dlYWtQdHIg PSBtYWtlV2Vha1B0cigqdGhpcyldIHsKKyAgICAgICAgY2FsbE9uTWFpblRocmVhZChbd2Vha1B0 ciA9IG1fd2Vha1RoaXNdIHsKICAgICAgICAgICAgIGlmICghd2Vha1B0cikKICAgICAgICAgICAg ICAgICByZXR1cm47CiAgICAgICAgICAgICB3ZWFrUHRyLT5vblRvcG9sb2d5U2V0KCk7CkBAIC00 NjMsNyArNDY0LDcgQEAgYm9vbCBNZWRpYVBsYXllclByaXZhdGVNZWRpYUZvdW5kYXRpb246OmVu ZEdldEV2ZW50KElNRkFzeW5jUmVzdWx0KiBhc3luY1Jlc3VsdCkKICAgICB9CiAKICAgICBjYXNl IE1FU2Vzc2lvblN0YXJ0ZWQ6IHsKLSAgICAgICAgY2FsbE9uTWFpblRocmVhZChbd2Vha1B0ciA9 IG1ha2VXZWFrUHRyKCp0aGlzKV0geworICAgICAgICBjYWxsT25NYWluVGhyZWFkKFt3ZWFrUHRy ID0gbV93ZWFrVGhpc10gewogICAgICAgICAgICAgaWYgKCF3ZWFrUHRyKQogICAgICAgICAgICAg ICAgIHJldHVybjsKICAgICAgICAgICAgIHdlYWtQdHItPm9uU2Vzc2lvblN0YXJ0ZWQoKTsKQEAg LTQ3Miw3ICs0NzMsNyBAQCBib29sIE1lZGlhUGxheWVyUHJpdmF0ZU1lZGlhRm91bmRhdGlvbjo6 ZW5kR2V0RXZlbnQoSU1GQXN5bmNSZXN1bHQqIGFzeW5jUmVzdWx0KQogICAgIH0KIAogICAgIGNh c2UgTUVCdWZmZXJpbmdTdGFydGVkOiB7Ci0gICAgICAgIGNhbGxPbk1haW5UaHJlYWQoW3dlYWtQ dHIgPSBtYWtlV2Vha1B0cigqdGhpcyldIHsKKyAgICAgICAgY2FsbE9uTWFpblRocmVhZChbd2Vh a1B0ciA9IG1fd2Vha1RoaXNdIHsKICAgICAgICAgICAgIGlmICghd2Vha1B0cikKICAgICAgICAg ICAgICAgICByZXR1cm47CiAgICAgICAgICAgICB3ZWFrUHRyLT5vbkJ1ZmZlcmluZ1N0YXJ0ZWQo KTsKQEAgLTQ4MSw3ICs0ODIsNyBAQCBib29sIE1lZGlhUGxheWVyUHJpdmF0ZU1lZGlhRm91bmRh dGlvbjo6ZW5kR2V0RXZlbnQoSU1GQXN5bmNSZXN1bHQqIGFzeW5jUmVzdWx0KQogICAgIH0KIAog ICAgIGNhc2UgTUVCdWZmZXJpbmdTdG9wcGVkOiB7Ci0gICAgICAgIGNhbGxPbk1haW5UaHJlYWQo W3dlYWtQdHIgPSBtYWtlV2Vha1B0cigqdGhpcyldIHsKKyAgICAgICAgY2FsbE9uTWFpblRocmVh ZChbd2Vha1B0ciA9IG1fd2Vha1RoaXNdIHsKICAgICAgICAgICAgIGlmICghd2Vha1B0cikKICAg ICAgICAgICAgICAgICByZXR1cm47CiAgICAgICAgICAgICB3ZWFrUHRyLT5vbkJ1ZmZlcmluZ1N0 b3BwZWQoKTsKQEAgLTQ5MCw3ICs0OTEsNyBAQCBib29sIE1lZGlhUGxheWVyUHJpdmF0ZU1lZGlh Rm91bmRhdGlvbjo6ZW5kR2V0RXZlbnQoSU1GQXN5bmNSZXN1bHQqIGFzeW5jUmVzdWx0KQogICAg IH0KIAogICAgIGNhc2UgTUVTZXNzaW9uRW5kZWQ6IHsKLSAgICAgICAgY2FsbE9uTWFpblRocmVh ZChbd2Vha1B0ciA9IG1ha2VXZWFrUHRyKCp0aGlzKV0geworICAgICAgICBjYWxsT25NYWluVGhy ZWFkKFt3ZWFrUHRyID0gbV93ZWFrVGhpc10gewogICAgICAgICAgICAgaWYgKCF3ZWFrUHRyKQog ICAgICAgICAgICAgICAgIHJldHVybjsKICAgICAgICAgICAgIHdlYWtQdHItPm9uU2Vzc2lvbkVu ZGVkKCk7CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy93aW4v TWVkaWFQbGF5ZXJQcml2YXRlTWVkaWFGb3VuZGF0aW9uLmggYi9Tb3VyY2UvV2ViQ29yZS9wbGF0 Zm9ybS9ncmFwaGljcy93aW4vTWVkaWFQbGF5ZXJQcml2YXRlTWVkaWFGb3VuZGF0aW9uLmgKaW5k ZXggNDRiZTYzNjExZDEyNmFiYTk1ODc0YzE1NzdlMzZiZDk1YjVjMWY1Ny4uMmQ4M2Y5NzY5ZjU2 ZDZjMmU3MzY4MWQ4ZmY3ZDNmMzBjNTEwMWRjNCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUv cGxhdGZvcm0vZ3JhcGhpY3Mvd2luL01lZGlhUGxheWVyUHJpdmF0ZU1lZGlhRm91bmRhdGlvbi5o CisrKyBiL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNzL3dpbi9NZWRpYVBsYXllclBy aXZhdGVNZWRpYUZvdW5kYXRpb24uaApAQCAtMTAxLDYgKzEwMSw3IEBAIHB1YmxpYzoKICAgICB2 b2lkIHBhaW50KEdyYXBoaWNzQ29udGV4dCYsIGNvbnN0IEZsb2F0UmVjdCYpIG92ZXJyaWRlOwog CiBwcml2YXRlOgorICAgIFdlYWtQdHI8TWVkaWFQbGF5ZXJQcml2YXRlTWVkaWFGb3VuZGF0aW9u PiBtX3dlYWtUaGlzOwogICAgIE1lZGlhUGxheWVyKiBtX3BsYXllcjsKICAgICBJbnRTaXplIG1f c2l6ZTsKICAgICBib29sIG1fdmlzaWJsZTsK