Bug 38602 – Page::setCanStartMedia does not properly handle the case where a media listener is removed

• Component: Media
• Status: RESOLVED FIXED

[Darin Adler]

Page::setCanStartMedia does not properly handle the case where a media listener is removed

[Darin Adler]

Created attachment 55144 [details]
Patch

[Darin Adler]

Comment on attachment 55144 [details]
Patch

Now the tricky part. I see this bug by code inspection, but I'm not sure how to make a test case.

I did not mean to put this up for review. webkit-patch set the review? flag for me.

[Adam Roben (:aroben)]

Comment on attachment 55144 [details]
Patch

> @@ -422,8 +422,11 @@ void Page::setCanStartMedia(bool canStar
>      m_mediaCanStartListeners.clear();
>  
>      size_t size = listeners.size();
> -    for (size_t i = 0; i < size; ++i)
> -        listeners[i]->mediaCanStart();
> +    for (size_t i = 0; i < size; ++i) {
> +        MediaCanStartListener* listener = listeners[i];
> +        if (m_mediaCanStartListeners.contains(listener))'

Won't this test always fail, since line 422 cleared out m_mediaCanStartListeners?

[Darin Adler]

(In reply to comment #3)
> (From update of attachment 55144 [details])
> > @@ -422,8 +422,11 @@ void Page::setCanStartMedia(bool canStar
> >      m_mediaCanStartListeners.clear();
> >  
> >      size_t size = listeners.size();
> > -    for (size_t i = 0; i < size; ++i)
> > -        listeners[i]->mediaCanStart();
> > +    for (size_t i = 0; i < size; ++i) {
> > +        MediaCanStartListener* listener = listeners[i];
> > +        if (m_mediaCanStartListeners.contains(listener))'
> 
> Won't this test always fail, since line 422 cleared out
> m_mediaCanStartListeners?

Yes, that's right. Need a bigger change.

[Darin Adler]

Created attachment 55147 [details]
Patch

[Darin Adler]

Created attachment 55168 [details]
Improved version.

[Darin Adler]

Committed r58894: <http://trac.webkit.org/changeset/58894>