145978 2015-06-15 10:08:57 -0700 Media Session: Active participating elements can change while being iterated 2015-06-15 14:07:35 -0700 1 1 1 Unclassified WebKit Media 528+ (Nightly build) All All RESOLVED FIXED InRadar P2 Normal --- 145411 1 mrajca webkit-unassigned commit-queue conrad_shultz eric.carlson jer.noble mrajca webkit-bug-importer oldest_to_newest 1101950 0 mrajca 2015-06-15 10:08:57 -0700 While enumerating m_activeParticipatingElements, we play/pause media elements. This in turn can cause changes to m_activeParticipatingElements while it's being enumerated. 1101951 1 webkit-bug-importer 2015-06-15 10:10:06 -0700 <rdar://problem/21384140> 1101953 2 254881 mrajca 2015-06-15 10:13:23 -0700 Created attachment 254881 Patch 1101961 3 254881 achristensen 2015-06-15 10:39:46 -0700 Comment on attachment 254881 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=254881&action=review Needs rebasing and some minor changes, but looks good to me. > Source/WebCore/ChangeLog:3 > + Media Session: Active participating elements can change while being enumerated enumerated -> iterated > Source/WebCore/Modules/mediasession/MediaSession.cpp:97 > + HashSet<HTMLMediaElement*> activeParticipatingElements = m_activeParticipatingElements; activeParticipatingElementsCopy to make it clear what is going on here. 1101963 4 254883 mrajca 2015-06-15 10:46:41 -0700 Created attachment 254883 Patch 1101988 5 254883 commit-queue 2015-06-15 11:56:19 -0700 Comment on attachment 254883 Patch Clearing flags on attachment: 254883 Committed r185560: <http://trac.webkit.org/changeset/185560> 1101989 6 commit-queue 2015-06-15 11:56:23 -0700 All reviewed patches have been landed. Closing bug. 1102000 7 254883 darin 2015-06-15 12:29:38 -0700 Comment on attachment 254883 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=254883&action=review > Source/WebCore/Modules/mediasession/MediaSession.cpp:99 > + HashSet<HTMLMediaElement*> activeParticipatingElementsCopy = m_activeParticipatingElements; > + > + for (auto* element : activeParticipatingElementsCopy) { This pattern almost always leads to serious bugs. Once you have copied the HTMLMediaElement set, elements from the set could be deleted as a side effect of the operations below, and then you could use an element pointer of a deleted object. One technique is removing each element from the set as we iterate instead of using a for loop, using the HashSet::takeAny function. Then also making sure that when an element is removed from the “real” set it’s also removed from the set currently being iterated. That pattern is used in DisplayRefreshMonitor::displayDidRefresh. Another technique is to use Vector<RefPtr<HTMLMediaElement>> instead of HashSet<MediaElement*> for the elements we are iterating. That guarantees the elements are not deallocated, but we might not want to toggle the state of an element that has been removed from the document tree, for example. I know we have run into the same problem elsewhere and solved it multiple ways. But this code is not safe. 1102021 8 254890 achristensen 2015-06-15 13:25:30 -0700 Created attachment 254890 followup patch This should address Darin's concern. 1102025 9 mrajca 2015-06-15 13:39:50 -0700 (In reply to comment #8) > Created attachment 254890 [details] > followup patch > > This should address Darin's concern. I already filed a follow-up Bug 145986. 1102030 10 254890 achristensen 2015-06-15 14:07:35 -0700 Comment on attachment 254890 followup patch My followup patch doesn't actually change anything. Matt's patch adds a pointer to the set being iterated, but I think that's also unnecessary until it is actually used. 254881 2015-06-15 10:13:23 -0700 2015-06-15 10:46:37 -0700 Patch bug-145978-20150615101248.patch text/plain 1544 mrajca U3VidmVyc2lvbiBSZXZpc2lvbjogMTg1NDc4CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggMjY2ZWVmMDkxNjZlN2Y5 NjQyZjBjNzcyZmVmODQwMmNkYmY0ZjE0ZS4uMWIzMWE3YzQ1NDNkNzRmYzA1OWE0NjcyNjk2MmVm MGFlNGNkMzRmMiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE0IEBACisyMDE1LTA2LTE1ICBNYXR0 IFJhamNhICA8bXJhamNhQGFwcGxlLmNvbT4KKworICAgICAgICBNZWRpYSBTZXNzaW9uOiBBY3Rp dmUgcGFydGljaXBhdGluZyBlbGVtZW50cyBjYW4gY2hhbmdlIHdoaWxlIGJlaW5nIGVudW1lcmF0 ZWQgCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xNDU5 NzgKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICAqIE1v ZHVsZXMvbWVkaWFzZXNzaW9uL01lZGlhU2Vzc2lvbi5jcHA6CisgICAgICAgIChXZWJDb3JlOjpN ZWRpYVNlc3Npb246OnRvZ2dsZVBsYXliYWNrKTogRW51bWVyYXRlIGEgY29weSBvZiBtX2FjdGl2 ZVBhcnRpY2lwYXRpbmdFbGVtZW50cyBzaW5jZSBpdHMgY29udGVudHMKKyAgICAgICAgICBjYW4g YmUgbW9kaWZpZWQgaW4gdGhlIGxvb3AuCisKIDIwMTUtMDYtMTAgIE1hdHQgUmFqY2EgIDxtcmFq Y2FAYXBwbGUuY29tPgogCiAgICAgICAgIEFkZCBiYXJlYm9uZXMgaW1wbGVtZW50YXRpb24gb2Yg bWVkaWEgc2Vzc2lvbiBpbnZvY2F0aW9uIGFsZ29yaXRobS4KZGlmZiAtLWdpdCBhL1NvdXJjZS9X ZWJDb3JlL01vZHVsZXMvbWVkaWFzZXNzaW9uL01lZGlhU2Vzc2lvbi5jcHAgYi9Tb3VyY2UvV2Vi Q29yZS9Nb2R1bGVzL21lZGlhc2Vzc2lvbi9NZWRpYVNlc3Npb24uY3BwCmluZGV4IGZmMzA1ZjQ1 N2ZlMjU0YjkyZThmNGM2Y2RjZmJjNDQ0NjFkY2FkMDUuLjU4NTY0NmZjMmZkOWI4Y2EzNGRmZjI1 NjE4MTcwNzQ2ZDlkYmZmMTcgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL01vZHVsZXMvbWVk aWFzZXNzaW9uL01lZGlhU2Vzc2lvbi5jcHAKKysrIGIvU291cmNlL1dlYkNvcmUvTW9kdWxlcy9t ZWRpYXNlc3Npb24vTWVkaWFTZXNzaW9uLmNwcApAQCAtOTQsNyArOTQsOSBAQCBib29sIE1lZGlh U2Vzc2lvbjo6aW52b2tlKCkKIAogdm9pZCBNZWRpYVNlc3Npb246OnRvZ2dsZVBsYXliYWNrKCkK IHsKLSAgICBmb3IgKGF1dG8qIGVsZW1lbnQgOiBtX2FjdGl2ZVBhcnRpY2lwYXRpbmdFbGVtZW50 cykgeworICAgIEhhc2hTZXQ8SFRNTE1lZGlhRWxlbWVudCo+IGFjdGl2ZVBhcnRpY2lwYXRpbmdF bGVtZW50cyA9IG1fYWN0aXZlUGFydGljaXBhdGluZ0VsZW1lbnRzOworCisgICAgZm9yIChhdXRv KiBlbGVtZW50IDogYWN0aXZlUGFydGljaXBhdGluZ0VsZW1lbnRzKSB7CiAgICAgICAgIGlmIChl bGVtZW50LT5wYXVzZWQoKSkKICAgICAgICAgICAgIGVsZW1lbnQtPnBsYXkoKTsKICAgICAgICAg ZWxzZQo= 254883 2015-06-15 10:46:41 -0700 2015-06-15 11:56:19 -0700 Patch bug-145978-20150615104606.patch text/plain 1545 mrajca U3VidmVyc2lvbiBSZXZpc2lvbjogMTg1NTU1CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggZGM1NDhhNGQwOTNjMGRj ZWE3N2U2NGU4NmJjODY5YzEwNjI4MWQzMi4uZGY1NTU1ZDEzYTU2NWZlZWQxZjdmZmY0ZWU0OTBi ZDg3ZDJlMzU0OCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE0IEBACisyMDE1LTA2LTE1ICBNYXR0 IFJhamNhICA8bXJhamNhQGFwcGxlLmNvbT4KKworICAgICAgICBNZWRpYSBTZXNzaW9uOiBBY3Rp dmUgcGFydGljaXBhdGluZyBlbGVtZW50cyBjYW4gY2hhbmdlIHdoaWxlIGJlaW5nIGl0ZXJhdGVk IAorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTQ1OTc4 CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgKiBNb2R1 bGVzL21lZGlhc2Vzc2lvbi9NZWRpYVNlc3Npb24uY3BwOgorICAgICAgICAoV2ViQ29yZTo6TWVk aWFTZXNzaW9uOjp0b2dnbGVQbGF5YmFjayk6IEl0ZXJhdGUgdGhyb3VnaCBhIGNvcHkgb2YgbV9h Y3RpdmVQYXJ0aWNpcGF0aW5nRWxlbWVudHMgc2luY2UgaXRzIGNvbnRlbnRzCisgICAgICAgICAg Y2FuIGJlIG1vZGlmaWVkIGluIHRoZSBsb29wLgorCiAyMDE1LTA2LTE1ICBBbGV4IENocmlzdGVu c2VuICA8YWNocmlzdGVuc2VuQHdlYmtpdC5vcmc+CiAKICAgICAgICAgW0NvbnRlbnQgRXh0ZW5z aW9uc10gTGltaXQgbnVtYmVyIG9mIHJ1bGVzLgpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUv TW9kdWxlcy9tZWRpYXNlc3Npb24vTWVkaWFTZXNzaW9uLmNwcCBiL1NvdXJjZS9XZWJDb3JlL01v ZHVsZXMvbWVkaWFzZXNzaW9uL01lZGlhU2Vzc2lvbi5jcHAKaW5kZXggZmYzMDVmNDU3ZmUyNTRi OTJlOGY0YzZjZGNmYmM0NDQ2MWRjYWQwNS4uNDI5OGIyNjhiODI1NzU2ZTkyNjhiZjk5MzhlNzhk ZDE1MDRiZWQ2ZiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvTW9kdWxlcy9tZWRpYXNlc3Np b24vTWVkaWFTZXNzaW9uLmNwcAorKysgYi9Tb3VyY2UvV2ViQ29yZS9Nb2R1bGVzL21lZGlhc2Vz c2lvbi9NZWRpYVNlc3Npb24uY3BwCkBAIC05NCw3ICs5NCw5IEBAIGJvb2wgTWVkaWFTZXNzaW9u OjppbnZva2UoKQogCiB2b2lkIE1lZGlhU2Vzc2lvbjo6dG9nZ2xlUGxheWJhY2soKQogewotICAg IGZvciAoYXV0byogZWxlbWVudCA6IG1fYWN0aXZlUGFydGljaXBhdGluZ0VsZW1lbnRzKSB7Cisg ICAgSGFzaFNldDxIVE1MTWVkaWFFbGVtZW50Kj4gYWN0aXZlUGFydGljaXBhdGluZ0VsZW1lbnRz Q29weSA9IG1fYWN0aXZlUGFydGljaXBhdGluZ0VsZW1lbnRzOworCisgICAgZm9yIChhdXRvKiBl bGVtZW50IDogYWN0aXZlUGFydGljaXBhdGluZ0VsZW1lbnRzQ29weSkgewogICAgICAgICBpZiAo ZWxlbWVudC0+cGF1c2VkKCkpCiAgICAgICAgICAgICBlbGVtZW50LT5wbGF5KCk7CiAgICAgICAg IGVsc2UK 254890 2015-06-15 13:25:30 -0700 2015-06-15 14:07:35 -0700 followup patch followup_patch text/plain 1508 achristensen SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDE4NTU2NSkKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE0IEBACisyMDE1LTA2LTE1ICBBbGV4IENo cmlzdGVuc2VuICA8YWNocmlzdGVuc2VuQHdlYmtpdC5vcmc+CisKKyAgICAgICAgW01lZGlhIFNl c3Npb25dIE1ha2UgY29kZSBzYWZlciBhZnRlciByMTg1NTYwLgorICAgICAgICBodHRwczovL2J1 Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTQ1OTc4CisKKyAgICAgICAgUmV2aWV3ZWQg YnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgKiBNb2R1bGVzL21lZGlhc2Vzc2lvbi9NZWRp YVNlc3Npb24uY3BwOgorICAgICAgICAoV2ViQ29yZTo6TWVkaWFTZXNzaW9uOjp0b2dnbGVQbGF5 YmFjayk6CisgICAgICAgIFVzZSBIYXNoU2V0Ojp0YWtlQW55IHdoaWxlIHRoZSBzZXQgaXMgbm90 IGVtcHR5IGluc3RlYWQgb2YgYSBmb3IgbG9vcCB0byBwcmV2ZW50IGZ1dHVyZSBidWdzLgorCiAy MDE1LTA2LTE1ICBNYXR0IFJhamNhICA8bXJhamNhQGFwcGxlLmNvbT4KIAogICAgICAgICBNZWRp YSBTZXNzaW9uOiBBY3RpdmUgcGFydGljaXBhdGluZyBlbGVtZW50cyBjYW4gY2hhbmdlIHdoaWxl IGJlaW5nIGl0ZXJhdGVkIApJbmRleDogU291cmNlL1dlYkNvcmUvTW9kdWxlcy9tZWRpYXNlc3Np b24vTWVkaWFTZXNzaW9uLmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2ViQ29yZS9Nb2R1bGVz L21lZGlhc2Vzc2lvbi9NZWRpYVNlc3Npb24uY3BwCShyZXZpc2lvbiAxODU1NjUpCisrKyBTb3Vy Y2UvV2ViQ29yZS9Nb2R1bGVzL21lZGlhc2Vzc2lvbi9NZWRpYVNlc3Npb24uY3BwCSh3b3JraW5n IGNvcHkpCkBAIC05NCw5ICs5NCwxMCBAQAogCiB2b2lkIE1lZGlhU2Vzc2lvbjo6dG9nZ2xlUGxh eWJhY2soKQogewotICAgIEhhc2hTZXQ8SFRNTE1lZGlhRWxlbWVudCo+IGFjdGl2ZVBhcnRpY2lw YXRpbmdFbGVtZW50c0NvcHkgPSBtX2FjdGl2ZVBhcnRpY2lwYXRpbmdFbGVtZW50czsKKyAgICBI YXNoU2V0PEhUTUxNZWRpYUVsZW1lbnQqPiBlbGVtZW50c1RvVG9nZ2xlID0gbV9hY3RpdmVQYXJ0 aWNpcGF0aW5nRWxlbWVudHM7CiAKLSAgICBmb3IgKGF1dG8qIGVsZW1lbnQgOiBhY3RpdmVQYXJ0 aWNpcGF0aW5nRWxlbWVudHNDb3B5KSB7CisgICAgd2hpbGUgKCFlbGVtZW50c1RvVG9nZ2xlLmlz RW1wdHkoKSkgeworICAgICAgICBhdXRvKiBlbGVtZW50ID0gZWxlbWVudHNUb1RvZ2dsZS50YWtl QW55KCk7CiAgICAgICAgIGlmIChlbGVtZW50LT5wYXVzZWQoKSkKICAgICAgICAgICAgIGVsZW1l bnQtPnBsYXkoKTsKICAgICAgICAgZWxzZQo=