While enumerating m_activeParticipatingElements, we play/pause media elements. This in turn can cause changes to m_activeParticipatingElements while it's being enumerated.
<rdar://problem/21384140>
Created attachment 254881 [details] Patch
Comment on attachment 254881 [details] 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.
Created attachment 254883 [details] Patch
Comment on attachment 254883 [details] Patch Clearing flags on attachment: 254883 Committed r185560: <http://trac.webkit.org/changeset/185560>
All reviewed patches have been landed. Closing bug.
Comment on attachment 254883 [details] 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.
Created attachment 254890 [details] followup patch This should address Darin's concern.
(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.
Comment on attachment 254890 [details] 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.