Media Session: Improve the safety of playback toggling

• Component: Media
• Status: RESOLVED FIXED

[Matt Rajca]

Per Darin's feedback in Bug 145978, we should be careful about iterating elements that were deleted underneath us in MediaSession::togglePlayback.

[Radar WebKit Bug Importer]

<rdar://problem/21388739>

[Matt Rajca]

Created attachment 254891 [details]
Patch

[Alex Christensen]

Comment on attachment 254891 [details]
Patch

There's no need to make a member variable for something that's only used in MediaSsession::togglePlayback.

[Matt Rajca]

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

[Alex Christensen]

It would be much nicer to not use so many raw pointers everywhere.

[Darin Adler]

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?

[Matt Rajca]

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

[Darin Adler]

Comment on attachment 254891 [details]
Patch

Oops, just noticed this doesn’t add the needed call to remove. Why not?

[Matt Rajca]

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.

[Alex Christensen]

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

[Darin Adler]

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

[Matt Rajca]

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.

[WebKit Commit Bot]

Comment on attachment 254891 [details]
Patch

Clearing flags on attachment: 254891

Committed r185570: <http://trac.webkit.org/changeset/185570>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.