Bug 210492 – Make use of WeakHashSet for MediaStreamTrackPrivate and RealtimeMediaSource observers

• Status: RESOLVED
• Resolution: FIXED
• Component: WebRTC

[youenn fablet]

Make use of WeakHashSet for MediaStreamTrackPrivate and RealtimeMediaSource observers

[youenn fablet]

Created attachment 396410 [details]
Patch

[Geoffrey Garen]

Comment on attachment 396410 [details]
Patch

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

> Source/WTF/wtf/WeakHashSet.h:144
> +        for (auto& item : map(m_set, [](auto& item) { return makeWeakPtr(item->template get<T>()); })) {
> +            if (item && m_set.contains(*item.m_impl))

This doesn't look right to me.

The lambda shouldn't need to call makeWeakPtr. That's a needless cost. It can just return the pointer or null, depending on whether the WeakPtrImpl is truthy. 

Also, we shouldn't need to check contains(). We just need to check whether the pointer is non-null / the WeakPtrImpl is truthy.

[youenn fablet]

(In reply to Geoffrey Garen from comment #2)
> Comment on attachment 396410 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=396410&action=review
> 
> > Source/WTF/wtf/WeakHashSet.h:144
> > +        for (auto& item : map(m_set, [](auto& item) { return makeWeakPtr(item->template get<T>()); })) {
> > +            if (item && m_set.contains(*item.m_impl))
> 
> This doesn't look right to me.
> 
> The lambda shouldn't need to call makeWeakPtr. That's a needless cost. It
> can just return the pointer or null, depending on whether the WeakPtrImpl is
> truthy. 

What is the guarantee that the vector pointers remain valid while looping over the vector and calling callback(item)?

WeakHashSet::add/remove could be made knowledgeable about the fact that a forEach call is ongoing to make sure m_set does not get modified during the forEach execution. Then we no longer need a vector at all. We would still probably need to check for any to-be-removed entry.

> Also, we shouldn't need to check contains(). We just need to check whether
> the pointer is non-null / the WeakPtrImpl is truthy.

This keeps parity with the existing code.
m_set can be modified as part of a callback(item) call and may no longer contain the next item in the vector.
We probably do not want an observer to be called after the observer removed itself from the set.

[Geoffrey Garen]

Comment on attachment 396410 [details]
Patch

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

r=me

>>> Source/WTF/wtf/WeakHashSet.h:144
>>> +            if (item && m_set.contains(*item.m_impl))
>> 
>> This doesn't look right to me.
>> 
>> The lambda shouldn't need to call makeWeakPtr. That's a needless cost. It can just return the pointer or null, depending on whether the WeakPtrImpl is truthy. 
>> 
>> Also, we shouldn't need to check contains(). We just need to check whether the pointer is non-null / the WeakPtrImpl is truthy.
> 
> What is the guarantee that the vector pointers remain valid while looping over the vector and calling callback(item)?
> 
> WeakHashSet::add/remove could be made knowledgeable about the fact that a forEach call is ongoing to make sure m_set does not get modified during the forEach execution. Then we no longer need a vector at all. We would still probably need to check for any to-be-removed entry.

I see. I guess I was imagining behavior more like other iteration and WTF::forEach, where modifying the set during iteration just isn't supported. But I don't object to supporting it.

[EWS]

Committed r260241: <https://trac.webkit.org/changeset/260241>

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

[Radar WebKit Bug Importer]

<rdar://problem/61925770>

[youenn fablet]

*** Bug 213928 has been marked as a duplicate of this bug. ***