200924 2019-08-20 09:59:54 -0700 Fix unsafe usage of MediaStreamTrackPrivate from background thread in MediaStreamTrackPrivate::audioSamplesAvailable() 2019-08-22 02:43:20 -0700 1 1 1 Unclassified WebKit WebKit2 WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED https://bugs.webkit.org/show_bug.cgi?id=200936 https://bugs.webkit.org/show_bug.cgi?id=200975 InRadar P2 Normal --- 1 cdumez youennf achristensen beidson commit-queue eric.carlson ggaren jer.noble webkit-bug-importer youennf oldest_to_newest 1563110 0 cdumez 2019-08-20 09:59:54 -0700 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. 1563113 1 376778 cdumez 2019-08-20 10:10:40 -0700 Created attachment 376778 Patch 1563122 2 376778 youennf 2019-08-20 10:19:18 -0700 Comment on attachment 376778 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. 1563126 3 cdumez 2019-08-20 10:25:58 -0700 (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)]() { 1563135 4 youennf 2019-08-20 10:43:06 -0700 (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)]() { 1563171 5 youennf 2019-08-20 11:57:24 -0700 > > 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. 1563997 6 youennf 2019-08-22 02:11:19 -0700 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). 1563998 7 youennf 2019-08-22 02:12:07 -0700 I'll also do a follow-up to remove m_weakThis. 1564008 8 376778 commit-queue 2019-08-22 02:42:04 -0700 Comment on attachment 376778 Patch Clearing flags on attachment: 376778 Committed r249000: <https://trac.webkit.org/changeset/249000> 1564009 9 commit-queue 2019-08-22 02:42:06 -0700 All reviewed patches have been landed. Closing bug. 1564010 10 webkit-bug-importer 2019-08-22 02:43:20 -0700 <rdar://problem/54591668> 376778 2019-08-20 10:10:40 -0700 2019-08-22 02:42:04 -0700 Patch bug-200924-20190820101039.patch text/plain 4505 cdumez U3VidmVyc2lvbiBSZXZpc2lvbjogMjQ4OTAyCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggMjA1ZjAzMTY4YjY2ODRh NTFjNDEyOTMwZWY0ZjE3ZjRkYmIzNjU3Mi4uYzNlY2MwMzZiOGNjNzRlODQ5ZmE4MDk1Y2Q1Nzdl ZGU2NTc4Yjc1NSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDIyIEBACisyMDE5LTA4LTIwICBDaHJp cyBEdW1leiAgPGNkdW1lekBhcHBsZS5jb20+CisKKyAgICAgICAgRml4IHVuc2FmZSB1c2FnZSBv ZiBNZWRpYVN0cmVhbVRyYWNrUHJpdmF0ZSBmcm9tIGJhY2tncm91bmQgdGhyZWFkIGluIE1lZGlh U3RyZWFtVHJhY2tQcml2YXRlOjphdWRpb1NhbXBsZXNBdmFpbGFibGUoKQorICAgICAgICBodHRw czovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MjAwOTI0CisKKyAgICAgICAgUmV2 aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgTWVkaWFTdHJlYW1UcmFja1ByaXZh dGUgaXMgY29uc3RydWN0ZWQgLyBkZXN0cnVjdGVkIG9uIHRoZSBtYWluIHRocmVhZCBidXQgaXRz IE1lZGlhU3RyZWFtVHJhY2tQcml2YXRlOjphdWRpb1NhbXBsZXNBdmFpbGFibGUoKQorICAgICAg ICBnZXRzIGNhbGxlZCBvbiBhIGJhY2tncm91bmQgdGhyZWFkLiBUaGUgYXVkaW9TYW1wbGVzQXZh aWxhYmxlKCkgbWV0aG9kIG1heSBnZXQgY2FsbGVkIHVudGlsIHRoZSBNZWRpYVN0cmVhbVRyYWNr UHJpdmF0ZQorICAgICAgICBkZXN0cnVjdG9yIHVucmVnaXN0ZXJzIHx0aGlzfCBhcyBhbiBvYnNl cnZlciBmcm9tIG1fc291cmNlLiBFdmVudCB0aG91Z2ggTWVkaWFTdHJlYW1UcmFja1ByaXZhdGUg c3ViY2xhc3NlcyBUaHJlYWRTYWZlUmVmQ291bnRlZCwKKyAgICAgICAgcmVmJ2luZyB8dGhpc3wg b24gdGhlIGJhY2tncm91bmQgdGhyZWFkIGluc2lkZSBhdWRpb1NhbXBsZXNBdmFpbGFibGUoKSBp cyBzdGlsbCB1bnNhZmUgYXMgdGhlIGRlc3RydWN0b3IgbWF5IGFscmVhZHkgYmUgcnVubmluZwor ICAgICAgICBvbiB0aGUgbWFpbiB0aHJlYWQuCisKKyAgICAgICAgKiBwbGF0Zm9ybS9tZWRpYXN0 cmVhbS9NZWRpYVN0cmVhbVRyYWNrUHJpdmF0ZS5jcHA6CisgICAgICAgIChXZWJDb3JlOjpNZWRp YVN0cmVhbVRyYWNrUHJpdmF0ZTo6TWVkaWFTdHJlYW1UcmFja1ByaXZhdGUpOgorICAgICAgICAo V2ViQ29yZTo6TWVkaWFTdHJlYW1UcmFja1ByaXZhdGU6On5NZWRpYVN0cmVhbVRyYWNrUHJpdmF0 ZSk6CisgICAgICAgIChXZWJDb3JlOjpNZWRpYVN0cmVhbVRyYWNrUHJpdmF0ZTo6YXVkaW9TYW1w bGVzQXZhaWxhYmxlKToKKyAgICAgICAgKiBwbGF0Zm9ybS9tZWRpYXN0cmVhbS9NZWRpYVN0cmVh bVRyYWNrUHJpdmF0ZS5oOgorCiAyMDE5LTA4LTIwICBDaHJpcyBEdW1leiAgPGNkdW1lekBhcHBs ZS5jb20+CiAKICAgICAgICAgVW5zYWZlIHVzYWdlIG9mIENvb2tpZVN0b3JhZ2VPYnNlcnZlciBm cm9tIGEgYmFja2dyb3VuZCB0aHJlYWQKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL3BsYXRm b3JtL21lZGlhc3RyZWFtL01lZGlhU3RyZWFtVHJhY2tQcml2YXRlLmNwcCBiL1NvdXJjZS9XZWJD b3JlL3BsYXRmb3JtL21lZGlhc3RyZWFtL01lZGlhU3RyZWFtVHJhY2tQcml2YXRlLmNwcAppbmRl eCA3MGNhZmMwMzFlYmE4ZDBlMWJmZTMyZDNiMGY0MTQzYzMzNzc0Y2Q3Li41NTc0ZTNmNmFkYThi NzQ3OWViOTA3MjYyOGY1YTU4NDkyZmZlYTcwIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9w bGF0Zm9ybS9tZWRpYXN0cmVhbS9NZWRpYVN0cmVhbVRyYWNrUHJpdmF0ZS5jcHAKKysrIGIvU291 cmNlL1dlYkNvcmUvcGxhdGZvcm0vbWVkaWFzdHJlYW0vTWVkaWFTdHJlYW1UcmFja1ByaXZhdGUu Y3BwCkBAIC01NiwxMyArNTYsMTUgQEAgUmVmPE1lZGlhU3RyZWFtVHJhY2tQcml2YXRlPiBNZWRp YVN0cmVhbVRyYWNrUHJpdmF0ZTo6Y3JlYXRlKFJlZjxjb25zdCBMb2dnZXI+JiYKIH0KIAogTWVk aWFTdHJlYW1UcmFja1ByaXZhdGU6Ok1lZGlhU3RyZWFtVHJhY2tQcml2YXRlKFJlZjxjb25zdCBM b2dnZXI+JiYgbG9nZ2VyLCBSZWY8UmVhbHRpbWVNZWRpYVNvdXJjZT4mJiBzb3VyY2UsIFN0cmlu ZyYmIGlkKQotICAgIDogbV9zb3VyY2UoV1RGTW92ZShzb3VyY2UpKQorICAgIDogbV93ZWFrVGhp cyhtYWtlV2Vha1B0cigqdGhpcykpCisgICAgLCBtX3NvdXJjZShXVEZNb3ZlKHNvdXJjZSkpCiAg ICAgLCBtX2lkKFdURk1vdmUoaWQpKQogICAgICwgbV9sb2dnZXIoV1RGTW92ZShsb2dnZXIpKQog I2lmICFSRUxFQVNFX0xPR19ESVNBQkxFRAogICAgICwgbV9sb2dJZGVudGlmaWVyKHVuaXF1ZUxv Z0lkZW50aWZpZXIoKSkKICNlbmRpZgogeworICAgIEFTU0VSVChpc01haW5UaHJlYWQoKSk7CiAg ICAgVU5VU0VEX1BBUkFNKGxvZ2dlcik7CiAjaWYgIVJFTEVBU0VfTE9HX0RJU0FCTEVECiAgICAg bV9zb3VyY2UtPnNldExvZ2dlcihtX2xvZ2dlci5jb3B5UmVmKCksIG1fbG9nSWRlbnRpZmllcik7 CkBAIC03Miw2ICs3NCw3IEBAIE1lZGlhU3RyZWFtVHJhY2tQcml2YXRlOjpNZWRpYVN0cmVhbVRy YWNrUHJpdmF0ZShSZWY8Y29uc3QgTG9nZ2VyPiYmIGxvZ2dlciwgUmVmCiAKIE1lZGlhU3RyZWFt VHJhY2tQcml2YXRlOjp+TWVkaWFTdHJlYW1UcmFja1ByaXZhdGUoKQogeworICAgIEFTU0VSVChp c01haW5UaHJlYWQoKSk7CiAgICAgbV9zb3VyY2UtPnJlbW92ZU9ic2VydmVyKCp0aGlzKTsKIH0K IApAQCAtMjYyLDcgKzI2NSwxMCBAQCB2b2lkIE1lZGlhU3RyZWFtVHJhY2tQcml2YXRlOjp2aWRl b1NhbXBsZUF2YWlsYWJsZShNZWRpYVNhbXBsZSYgbWVkaWFTYW1wbGUpCiB2b2lkIE1lZGlhU3Ry ZWFtVHJhY2tQcml2YXRlOjphdWRpb1NhbXBsZXNBdmFpbGFibGUoY29uc3QgTWVkaWFUaW1lJiBt ZWRpYVRpbWUsIGNvbnN0IFBsYXRmb3JtQXVkaW9EYXRhJiBkYXRhLCBjb25zdCBBdWRpb1N0cmVh bURlc2NyaXB0aW9uJiBkZXNjcmlwdGlvbiwgc2l6ZV90IHNhbXBsZUNvdW50KQogewogICAgIGlm ICghbV9oYXNTZW50U3RhcnRQcm9kdWNlZERhdGEpIHsKLSAgICAgICAgY2FsbE9uTWFpblRocmVh ZChbdGhpcywgcHJvdGVjdGVkVGhpcyA9IG1ha2VSZWYoKnRoaXMpXSB7CisgICAgICAgIGNhbGxP bk1haW5UaHJlYWQoW3RoaXMsIHdlYWtUaGlzID0gbV93ZWFrVGhpc10geworICAgICAgICAgICAg aWYgKCF3ZWFrVGhpcykKKyAgICAgICAgICAgICAgICByZXR1cm47CisKICAgICAgICAgICAgIGlm ICghbV9oYXZlUHJvZHVjZWREYXRhKSB7CiAgICAgICAgICAgICAgICAgbV9oYXZlUHJvZHVjZWRE YXRhID0gdHJ1ZTsKICAgICAgICAgICAgICAgICB1cGRhdGVSZWFkeVN0YXRlKCk7CmRpZmYgLS1n aXQgYS9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9tZWRpYXN0cmVhbS9NZWRpYVN0cmVhbVRyYWNr UHJpdmF0ZS5oIGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vbWVkaWFzdHJlYW0vTWVkaWFTdHJl YW1UcmFja1ByaXZhdGUuaAppbmRleCAzNzU5MDU4NjJkODNlZjllZDQyYzRkN2RlMjM2NTY0NzZm ZTZmNzBmLi5iM2M3NmI4ODExM2MxYjhhNTU1NzY4NDhiODUzZWUzOGVmYjUxMGZkIDEwMDY0NAot LS0gYS9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9tZWRpYXN0cmVhbS9NZWRpYVN0cmVhbVRyYWNr UHJpdmF0ZS5oCisrKyBiL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL21lZGlhc3RyZWFtL01lZGlh U3RyZWFtVHJhY2tQcml2YXRlLmgKQEAgLTMxLDYgKzMxLDcgQEAKIAogI2luY2x1ZGUgIlJlYWx0 aW1lTWVkaWFTb3VyY2UuaCIKICNpbmNsdWRlIDx3dGYvTG9nZ2VySGVscGVyLmg+CisjaW5jbHVk ZSA8d3RmL1dlYWtQdHIuaD4KIAogbmFtZXNwYWNlIFdlYkNvcmUgewogCkBAIC00Miw2ICs0Myw3 IEBAIGNsYXNzIFdlYkF1ZGlvU291cmNlUHJvdmlkZXI7CiAKIGNsYXNzIE1lZGlhU3RyZWFtVHJh Y2tQcml2YXRlIGZpbmFsCiAgICAgOiBwdWJsaWMgVGhyZWFkU2FmZVJlZkNvdW50ZWQ8TWVkaWFT dHJlYW1UcmFja1ByaXZhdGUsIFdURjo6RGVzdHJ1Y3Rpb25UaHJlYWQ6Ok1haW4+CisgICAgLCBw dWJsaWMgQ2FuTWFrZVdlYWtQdHI8TWVkaWFTdHJlYW1UcmFja1ByaXZhdGU+CiAgICAgLCBwdWJs aWMgUmVhbHRpbWVNZWRpYVNvdXJjZTo6T2JzZXJ2ZXIKICNpZiAhUkVMRUFTRV9MT0dfRElTQUJM RUQKICAgICAsIHByaXZhdGUgTG9nZ2VySGVscGVyCkBAIC0xNDIsNiArMTQ0LDcgQEAgcHJpdmF0 ZToKICAgICBXVEZMb2dDaGFubmVsJiBsb2dDaGFubmVsKCkgY29uc3QgZmluYWw7CiAjZW5kaWYK IAorICAgIFdlYWtQdHI8TWVkaWFTdHJlYW1UcmFja1ByaXZhdGU+IG1fd2Vha1RoaXM7CiAgICAg bXV0YWJsZSBSZWN1cnNpdmVMb2NrIG1fb2JzZXJ2ZXJzTG9jazsKICAgICBIYXNoU2V0PE9ic2Vy dmVyKj4gbV9vYnNlcnZlcnM7CiAgICAgUmVmPFJlYWx0aW1lTWVkaWFTb3VyY2U+IG1fc291cmNl Owo=