Improve the pointer safety by using Ref<>&&.
Created attachment 344448 [details] Cleanup
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.
(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.
Committed r233596: <https://trac.webkit.org/changeset/233596>
<rdar://problem/41910945>
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?