Bug 187407 – Make HTMLMediaElement::remove*Track take a Ref<>&&

• Component: Media
• Status: RESOLVED FIXED

[Ryosuke Niwa]

Improve the pointer safety by using Ref<>&&.

[Ryosuke Niwa]

Created attachment 344448 [details]
Cleanup

[Chris Dumez]

Comment on attachment 344448 [details]
Cleanup

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

Why is the current code unsafe? Unless we can prove it is unsafe, I would avoid ref counting churn.

> Source/WebCore/html/HTMLMediaElement.cpp:4052
> +    m_audioTracks->remove(track.get());

Looks to me that this is the call here that may destroy the track since removing it from m_audioTracks and it derefs the track. However, since we're not using the track after, this code looks completely safe to me.

> Source/WebCore/html/HTMLMediaElement.cpp:4062
>          m_textTracks->remove(track, scheduleEvent);

ditto here.

[Ryosuke Niwa]

(In reply to Chris Dumez from comment #2)
> Comment on attachment 344448 [details]
> Cleanup
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=344448&action=review
> 
> Why is the current code unsafe? Unless we can prove it is unsafe, I would
> avoid ref counting churn.
> 
> > Source/WebCore/html/HTMLMediaElement.cpp:4052
> > +    m_audioTracks->remove(track.get());
> 
> Looks to me that this is the call here that may destroy the track since
> removing it from m_audioTracks and it derefs the track. However, since we're
> not using the track after, this code looks completely safe to me.

Sure, I think the concern here is that someone can add unsafe code there. Throughout WebCore, we have a bunch of code like this where someone can add one extra line of code and make it unsafe. I don't think we should be doing that going forward.

[Ryosuke Niwa]

Committed r233596: <https://trac.webkit.org/changeset/233596>

[Radar WebKit Bug Importer]

<rdar://problem/41910945>

[Said Abou-Hallawa]

Comment on attachment 344448 [details]
Cleanup

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

>>> Source/WebCore/html/HTMLMediaElement.cpp:4052
>>> +    m_audioTracks->remove(track.get());
>> 
>> Looks to me that this is the call here that may destroy the track since removing it from m_audioTracks and it derefs the track. However, since we're not using the track after, this code looks completely safe to me.
> 
> Sure, I think the concern here is that someone can add unsafe code there. Throughout WebCore, we have a bunch of code like this where someone can add one extra line of code and make it unsafe. I don't think we should be doing that going forward.

Alternatively, if the signature of the TrackListBase::remove() changes such that it takes Ref<TrackBase>&& and the above call can be changed to be

    m_audioTracks->remove(WTFMove(track));

In this case, it is clear that track becomes invalid after it is removed from m_audioTracks and should not be accessed.

> Source/WebCore/html/HTMLMediaElement.cpp:4083
> +            auto track = makeRef(*m_textTracks->item(i));
> +            if (track->trackType() == TextTrack::InBand)
> +                removeTextTrack(WTFMove(track), false);

Now we switch between RefPtr, Ref,raw pointer, raw reference in these two statements multiple times:

1. In TextTrakcList::item(), Vector::operator[]() returns a RefPtr
2. But TextTrakcList::item() returns a raw pointer.
3  Here we get a raw reference from this raw pointer above.
3. And then we call makeRef() to create a Ref from the raw reference.
4. HTMLMediaElement::removeTextTrack() gets a raw reference from the Ref object and sends it to TextTrackList::remove()
5. TextTrackList::remove() gets a raw pointer from the raw reference.
6. TextTrackList::remove() creates a RefPtr from the raw pointer when calling Vector::find().

I still do not get it, what is the point in having all kinds of pointers and references in just two statements? Why do we have to convert from Ref/RefPtr to raw reference/pointer and immediately do the opposite?