Media Session: Active participating elements can change while being iterated

• Component: Media
• Status: RESOLVED FIXED

[Matt Rajca]

While enumerating m_activeParticipatingElements, we play/pause media elements. This in turn can cause changes to m_activeParticipatingElements while it's being enumerated.

[Radar WebKit Bug Importer]

<rdar://problem/21384140>

[Matt Rajca]

Created attachment 254881 [details]
Patch

[Alex Christensen]

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.

[Matt Rajca]

Created attachment 254883 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 254883 [details]
Patch

Clearing flags on attachment: 254883

Committed r185560: <http://trac.webkit.org/changeset/185560>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

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.

[Alex Christensen]

Created attachment 254890 [details]
followup patch

This should address Darin's concern.

[Matt Rajca]

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

[Alex Christensen]

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.