Per Darin's feedback in Bug 145978, we should be careful about iterating elements that were deleted underneath us in MediaSession::togglePlayback.
<rdar://problem/21388739>
Created attachment 254891 [details] Patch
Comment on attachment 254891 [details] Patch There's no need to make a member variable for something that's only used in MediaSsession::togglePlayback.
(In reply to comment #3) > Comment on attachment 254891 [details] > Patch > > There's no need to make a member variable for something that's only used in > MediaSsession::togglePlayback. We will have to remove elements from that set when elements from the underlying m_activeParticipatingElements set are removed.
It would be much nicer to not use so many raw pointers everywhere.
Comment on attachment 254891 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=254891&action=review > Source/WebCore/Modules/mediasession/MediaSession.cpp:97 > + ASSERT(!m_iteratedActiveParticipatingElements); What guarantees this won’t happen? I’d think that if we make something play that some events might fire and in response we might run some code and end up back here. Maybe we need to handle this case, perhaps by doing nothing?
(In reply to comment #6) > Comment on attachment 254891 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=254891&action=review > > > Source/WebCore/Modules/mediasession/MediaSession.cpp:97 > > + ASSERT(!m_iteratedActiveParticipatingElements); > > What guarantees this won’t happen? I’d think that if we make something play > that some events might fire and in response we might run some code and end > up back here. > > Maybe we need to handle this case, perhaps by doing nothing? This assertion is in the MediaSession::togglePlayback method which is currently only called in response to play/pause media keys. This method will never call itself.
Comment on attachment 254891 [details] Patch Oops, just noticed this doesn’t add the needed call to remove. Why not?
We cur(In reply to comment #8) > Comment on attachment 254891 [details] > Patch > > Oops, just noticed this doesn’t add the needed call to remove. Why not? We currently don't remove elements from m_activeParticipatingElements (and thus m_iteratedActiveParticipatingElements). That's part of the algorithm for "interrupting a media session from a media element" which I'll implement in the next couple days.
(In reply to comment #9) > We currently don't remove elements from m_activeParticipatingElements (and > thus m_iteratedActiveParticipatingElements). That's part of > the algorithm for "interrupting a media session from a media element" which > I'll implement in the next couple days. This is why I was so confused. This will need to be done, but 185560 doesn't need it yet.
(In reply to comment #9) > We cur(In reply to comment #8) > > Comment on attachment 254891 [details] > > Patch > > > > Oops, just noticed this doesn’t add the needed call to remove. Why not? > > We currently don't remove elements from m_activeParticipatingElements (and > thus m_iteratedActiveParticipatingElements). That's part of > the algorithm for "interrupting a media session from a media element" which > I'll implement in the next couple days. Before that algorithm comes into play: What guarantees that m_activeParticipatingElements doesn’t point to deleted objects when media elements go away?
Good catch. Section 6.5 of the Media Session spec describes what should happen when an element is removed from the document (which includes removing it from the media session's set of active participating elements). I'll get to that soon as well.
Comment on attachment 254891 [details] Patch Clearing flags on attachment: 254891 Committed r185570: <http://trac.webkit.org/changeset/185570>
All reviewed patches have been landed. Closing bug.