Bug 145978

Summary: Media Session: Active participating elements can change while being iterated
Product: WebKit Reporter: Matt Rajca <mrajca>
Component: MediaAssignee: Nobody <webkit-unassigned>
Severity: Normal CC: commit-queue, conrad_shultz, eric.carlson, jer.noble, mrajca, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 145411    
Description Flags
followup patch achristensen: review-

Description Matt Rajca 2015-06-15 10:08:57 PDT
While enumerating m_activeParticipatingElements, we play/pause media elements. This in turn can cause changes to m_activeParticipatingElements while it's being enumerated.
Comment 1 Radar WebKit Bug Importer 2015-06-15 10:10:06 PDT
Comment 2 Matt Rajca 2015-06-15 10:13:23 PDT
Created attachment 254881 [details]
Comment 3 Alex Christensen 2015-06-15 10:39:46 PDT
Comment on attachment 254881 [details]

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.
Comment 4 Matt Rajca 2015-06-15 10:46:41 PDT
Created attachment 254883 [details]
Comment 5 WebKit Commit Bot 2015-06-15 11:56:19 PDT
Comment on attachment 254883 [details]

Clearing flags on attachment: 254883

Committed r185560: <http://trac.webkit.org/changeset/185560>
Comment 6 WebKit Commit Bot 2015-06-15 11:56:23 PDT
All reviewed patches have been landed.  Closing bug.
Comment 7 Darin Adler 2015-06-15 12:29:38 PDT
Comment on attachment 254883 [details]

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.
Comment 8 Alex Christensen 2015-06-15 13:25:30 PDT
Created attachment 254890 [details]
followup patch

This should address Darin's concern.
Comment 9 Matt Rajca 2015-06-15 13:39:50 PDT
(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 10 Alex Christensen 2015-06-15 14:07:35 PDT
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.