187407 2018-07-06 13:10:16 -0700 Make HTMLMediaElement::remove*Track take a Ref<>&& 2018-07-06 18:26:18 -0700 1 1 1 Unclassified WebKit Media WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 rniwa rniwa cdumez eric.carlson jer.noble sabouhallawa webkit-bug-importer zalan oldest_to_newest 1439894 0 rniwa 2018-07-06 13:10:16 -0700 Improve the pointer safety by using Ref<>&&. 1439895 1 344448 rniwa 2018-07-06 13:12:33 -0700 Created attachment 344448 Cleanup 1439899 2 344448 cdumez 2018-07-06 13:18:52 -0700 Comment on attachment 344448 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. 1439909 3 rniwa 2018-07-06 14:02:50 -0700 (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. 1439940 4 rniwa 2018-07-06 15:19:21 -0700 Committed r233596: <https://trac.webkit.org/changeset/233596> 1439943 5 webkit-bug-importer 2018-07-06 15:20:18 -0700 <rdar://problem/41910945> 1440041 6 344448 sabouhallawa 2018-07-06 18:26:18 -0700 Comment on attachment 344448 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? 344448 2018-07-06 13:12:33 -0700 2018-07-06 14:06:27 -0700 Cleanup bug-187407-20180706131232.patch text/plain 3725 rniwa SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDIzMzU4OCkKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE5IEBACisyMDE4LTA3LTA2ICBSeW9zdWtl IE5pd2EgIDxybml3YUB3ZWJraXQub3JnPgorCisgICAgICAgIE1ha2UgSFRNTE1lZGlhRWxlbWVu dDo6cmVtb3ZlKlRyYWNrIHRha2UgYSBSZWY8PiYmCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJr aXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xODc0MDcKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JP RFkgKE9PUFMhKS4KKworICAgICAgICBNYWtlIHRoZXNlIGZ1bmN0aW9ucyB0YWtlIFJlZjw+JiYg c2luY2UgdGhleSBjYW4gZGVsZXRlIHRyYWNrIG9iamVjdHMuCisKKyAgICAgICAgKiBodG1sL0hU TUxNZWRpYUVsZW1lbnQuY3BwOgorICAgICAgICAoV2ViQ29yZTo6SFRNTE1lZGlhRWxlbWVudDo6 cmVtb3ZlQXVkaW9UcmFjayk6CisgICAgICAgIChXZWJDb3JlOjpIVE1MTWVkaWFFbGVtZW50Ojpy ZW1vdmVUZXh0VHJhY2spOgorICAgICAgICAoV2ViQ29yZTo6SFRNTE1lZGlhRWxlbWVudDo6cmVt b3ZlVmlkZW9UcmFjayk6CisgICAgICAgIChXZWJDb3JlOjpIVE1MTWVkaWFFbGVtZW50Ojpmb3Jn ZXRSZXNvdXJjZVNwZWNpZmljVHJhY2tzKToKKyAgICAgICAgKiBodG1sL0hUTUxNZWRpYUVsZW1l bnQuaDoKKwogMjAxOC0wNy0wNiAgQW50b2luZSBRdWludCAgPGdyYW91dHNAYXBwbGUuY29tPgog CiAgICAgICAgIFtXZWIgQW5pbWF0aW9uc10gTWFrZSBXUFQgdGVzdCBhdCBpbnRlcmZhY2VzL0tl eWZyYW1lRWZmZWN0L3Byb2Nlc3NpbmctYS1rZXlmcmFtZXMtYXJndW1lbnQtMDAyLmh0bWwgcGFz cyByZWxpYWJseQpJbmRleDogU291cmNlL1dlYkNvcmUvaHRtbC9IVE1MTWVkaWFFbGVtZW50LmNw cAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2ViQ29yZS9odG1sL0hUTUxNZWRpYUVsZW1lbnQuY3Bw CShyZXZpc2lvbiAyMzM1NzApCisrKyBTb3VyY2UvV2ViQ29yZS9odG1sL0hUTUxNZWRpYUVsZW1l bnQuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC00MDQ2LDI3ICs0MDQ2LDI3IEBAIHZvaWQgSFRNTE1l ZGlhRWxlbWVudDo6YWRkVmlkZW9UcmFjayhSZWYKICAgICB2aWRlb1RyYWNrcygpLmFwcGVuZChX VEZNb3ZlKHRyYWNrKSk7CiB9CiAKLXZvaWQgSFRNTE1lZGlhRWxlbWVudDo6cmVtb3ZlQXVkaW9U cmFjayhBdWRpb1RyYWNrJiB0cmFjaykKK3ZvaWQgSFRNTE1lZGlhRWxlbWVudDo6cmVtb3ZlQXVk aW9UcmFjayhSZWY8QXVkaW9UcmFjaz4mJiB0cmFjaykKIHsKLSAgICB0cmFjay5jbGVhckNsaWVu dCgpOwotICAgIG1fYXVkaW9UcmFja3MtPnJlbW92ZSh0cmFjayk7CisgICAgdHJhY2stPmNsZWFy Q2xpZW50KCk7CisgICAgbV9hdWRpb1RyYWNrcy0+cmVtb3ZlKHRyYWNrLmdldCgpKTsKIH0KIAot dm9pZCBIVE1MTWVkaWFFbGVtZW50OjpyZW1vdmVUZXh0VHJhY2soVGV4dFRyYWNrJiB0cmFjaywg Ym9vbCBzY2hlZHVsZUV2ZW50KQordm9pZCBIVE1MTWVkaWFFbGVtZW50OjpyZW1vdmVUZXh0VHJh Y2soUmVmPFRleHRUcmFjaz4mJiB0cmFjaywgYm9vbCBzY2hlZHVsZUV2ZW50KQogewogICAgIFRy YWNrRGlzcGxheVVwZGF0ZVNjb3BlIHNjb3BlIHsgKnRoaXMgfTsKLSAgICBpZiAoYXV0byBjdWVz ID0gbWFrZVJlZlB0cih0cmFjay5jdWVzKCkpKQorICAgIGlmIChhdXRvIGN1ZXMgPSBtYWtlUmVm UHRyKHRyYWNrLT5jdWVzKCkpKQogICAgICAgICB0ZXh0VHJhY2tSZW1vdmVDdWVzKHRyYWNrLCAq Y3Vlcyk7Ci0gICAgdHJhY2suY2xlYXJDbGllbnQoKTsKKyAgICB0cmFjay0+Y2xlYXJDbGllbnQo KTsKICAgICBpZiAobV90ZXh0VHJhY2tzKQogICAgICAgICBtX3RleHRUcmFja3MtPnJlbW92ZSh0 cmFjaywgc2NoZWR1bGVFdmVudCk7CiAKICAgICBjbG9zZUNhcHRpb25UcmFja3NDaGFuZ2VkKCk7 CiB9CiAKLXZvaWQgSFRNTE1lZGlhRWxlbWVudDo6cmVtb3ZlVmlkZW9UcmFjayhWaWRlb1RyYWNr JiB0cmFjaykKK3ZvaWQgSFRNTE1lZGlhRWxlbWVudDo6cmVtb3ZlVmlkZW9UcmFjayhSZWY8Vmlk ZW9UcmFjaz4mJiB0cmFjaykKIHsKLSAgICB0cmFjay5jbGVhckNsaWVudCgpOworICAgIHRyYWNr LT5jbGVhckNsaWVudCgpOwogICAgIG1fdmlkZW9UcmFja3MtPnJlbW92ZSh0cmFjayk7CiB9CiAK QEAgLTQwNzgsOSArNDA3OCw5IEBAIHZvaWQgSFRNTE1lZGlhRWxlbWVudDo6Zm9yZ2V0UmVzb3Vy Y2VTcGUKICAgICBpZiAobV90ZXh0VHJhY2tzKSB7CiAgICAgICAgIFRyYWNrRGlzcGxheVVwZGF0 ZVNjb3BlIHNjb3BlIHsgKnRoaXMgfTsKICAgICAgICAgZm9yIChpbnQgaSA9IG1fdGV4dFRyYWNr cy0+bGVuZ3RoKCkgLSAxOyBpID49IDA7IC0taSkgewotICAgICAgICAgICAgYXV0byYgdHJhY2sg PSAqbV90ZXh0VHJhY2tzLT5pdGVtKGkpOwotICAgICAgICAgICAgaWYgKHRyYWNrLnRyYWNrVHlw ZSgpID09IFRleHRUcmFjazo6SW5CYW5kKQotICAgICAgICAgICAgICAgIHJlbW92ZVRleHRUcmFj ayh0cmFjaywgZmFsc2UpOworICAgICAgICAgICAgYXV0byB0cmFjayA9IG1ha2VSZWYoKm1fdGV4 dFRyYWNrcy0+aXRlbShpKSk7CisgICAgICAgICAgICBpZiAodHJhY2stPnRyYWNrVHlwZSgpID09 IFRleHRUcmFjazo6SW5CYW5kKQorICAgICAgICAgICAgICAgIHJlbW92ZVRleHRUcmFjayhXVEZN b3ZlKHRyYWNrKSwgZmFsc2UpOwogICAgICAgICB9CiAgICAgfQogCkluZGV4OiBTb3VyY2UvV2Vi Q29yZS9odG1sL0hUTUxNZWRpYUVsZW1lbnQuaAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2ViQ29y ZS9odG1sL0hUTUxNZWRpYUVsZW1lbnQuaAkocmV2aXNpb24gMjMzNTcwKQorKysgU291cmNlL1dl YkNvcmUvaHRtbC9IVE1MTWVkaWFFbGVtZW50LmgJKHdvcmtpbmcgY29weSkKQEAgLTMzOSw5ICsz MzksOSBAQCBwdWJsaWM6CiAgICAgdm9pZCBhZGRBdWRpb1RyYWNrKFJlZjxBdWRpb1RyYWNrPiYm KTsKICAgICB2b2lkIGFkZFRleHRUcmFjayhSZWY8VGV4dFRyYWNrPiYmKTsKICAgICB2b2lkIGFk ZFZpZGVvVHJhY2soUmVmPFZpZGVvVHJhY2s+JiYpOwotICAgIHZvaWQgcmVtb3ZlQXVkaW9UcmFj ayhBdWRpb1RyYWNrJik7Ci0gICAgdm9pZCByZW1vdmVUZXh0VHJhY2soVGV4dFRyYWNrJiwgYm9v bCBzY2hlZHVsZUV2ZW50ID0gdHJ1ZSk7Ci0gICAgdm9pZCByZW1vdmVWaWRlb1RyYWNrKFZpZGVv VHJhY2smKTsKKyAgICB2b2lkIHJlbW92ZUF1ZGlvVHJhY2soUmVmPEF1ZGlvVHJhY2s+JiYpOwor ICAgIHZvaWQgcmVtb3ZlVGV4dFRyYWNrKFJlZjxUZXh0VHJhY2s+JiYsIGJvb2wgc2NoZWR1bGVF dmVudCA9IHRydWUpOworICAgIHZvaWQgcmVtb3ZlVmlkZW9UcmFjayhSZWY8VmlkZW9UcmFjaz4m Jik7CiAgICAgdm9pZCBmb3JnZXRSZXNvdXJjZVNwZWNpZmljVHJhY2tzKCk7CiAgICAgdm9pZCBj bG9zZUNhcHRpb25UcmFja3NDaGFuZ2VkKCk7CiAgICAgdm9pZCBub3RpZnlNZWRpYVBsYXllck9m VGV4dFRyYWNrQ2hhbmdlcygpOwo=