WebCore::HTMLMediaElement::mediaCanStart crashes

[Peng Liu]

WebCore::HTMLMediaElement::mediaCanStart crashes

[Peng Liu]

<rdar://problem/49677331>

[Peng Liu]

Created attachment 395343 [details]
Patch

[Jer Noble]

Comment on attachment 395343 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=395343&action=review

r=me, with nits.

> Source/WebCore/dom/Document.cpp:6426
>  void Document::addMediaCanStartListener(MediaCanStartListener& listener)
>  {
> -    ASSERT(!m_mediaCanStartListeners.contains(&listener));
> -    m_mediaCanStartListeners.add(&listener);
> +    ASSERT(!m_mediaCanStartListeners.contains(listener));
> +    m_mediaCanStartListeners.add(listener);

The way I'd do this would be:

void Document::addMediaCanStartListener(WeakPtr<MediaCanStartListener>&& listener)
{
    ASSERT(!m_mediaCanStartListeners.contains(listener));
    m_mediaCanStartListeners.add(WTFMove(listener));
}

This way, MediaCanStartListener doesn't need to directly inherit from CanMakeWeakPtr.

> Source/WebCore/dom/Document.cpp:6437
> +    if (!m_mediaCanStartListeners.computeSize())

I think this could be: `if (m_mediaCanStartListeners.computeEmpty())`.

> Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.h:37
> -class UserMediaPermissionRequestManager : public CanMakeWeakPtr<UserMediaPermissionRequestManager>, private WebCore::MediaCanStartListener {
> +class UserMediaPermissionRequestManager : private WebCore::MediaCanStartListener {

Then this could get reverted. 

It would mean all the call sites to addMediaCanStartListener() would have to change, though.

[Peng Liu]

Comment on attachment 395343 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=395343&action=review

>> Source/WebCore/dom/Document.cpp:6426
>> +    m_mediaCanStartListeners.add(listener);
> 
> The way I'd do this would be:
> 
> void Document::addMediaCanStartListener(WeakPtr<MediaCanStartListener>&& listener)
> {
>     ASSERT(!m_mediaCanStartListeners.contains(listener));
>     m_mediaCanStartListeners.add(WTFMove(listener));
> }
> 
> This way, MediaCanStartListener doesn't need to directly inherit from CanMakeWeakPtr.

That's a good idea! Unfortunately it requires us to add WeakHashSet::add(WeakPtr<U>&& value) to the current implementation.

>> Source/WebCore/dom/Document.cpp:6437
>> +    if (!m_mediaCanStartListeners.computeSize())
> 
> I think this could be: `if (m_mediaCanStartListeners.computeEmpty())`.

Yes, that will work. I chose to use computeSize() because it will clean up nullptrs.

>> Source/WebKit/WebProcess/MediaStream/UserMediaPermissionRequestManager.h:37
>> +class UserMediaPermissionRequestManager : private WebCore::MediaCanStartListener {
> 
> Then this could get reverted. 
> 
> It would mean all the call sites to addMediaCanStartListener() would have to change, though.

Maybe we can do it later after extending WeakHashSet.

[Peng Liu]

Created attachment 395349 [details]
Patch for landing (replace computeSize() with computesEmpty())

[EWS]

Committed r259447: <https://trac.webkit.org/changeset/259447>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 395349 [details].