882706 - [webvtt] Implement TextTrackList::GetTrackById()

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Rick Eyre (:reyre)]

We haven't implemented this yet.

Comment 1

[Rick Eyre (:reyre)]

Attachment: v1: Implement TextTrackList::GetTrackById()

Comment 2

[Ralph Giles (:rillian)]

Comment on attachment 772679 [details] [diff] [review]
v1: Implement TextTrackList::GetTrackById()

Review of attachment 772679 [details] [diff] [review]:
-----------------------------------------------------------------

The code looks fine so far. How is mId set? It's a read-only attribute, so the bindings can't set it. The spec says it should have the same value as the creating <track> element's id, so there should be an optional constructor which sets it and HTMLTrackElement should set it.

http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element.html#dom-texttrack-id

::: content/media/TextTrackList.cpp
@@ +60,5 @@
> +  nsAutoString id;
> +  for (uint32_t i = 0; i < Length(); i++) {
> +    mTextTracks[i]->GetId(id);
> +    if (aId.Equals(id)) {
> +      return mTextTracks[i].forget();

Help me understand the memory management here. Why do we forget the reference if we're returning an already_AddRefed?

Comment 3

[Ralph Giles (:rillian)]

Oh, sorry, setting mId is bug 891381. I'd just like a response from the memory question.

Comment 4

[Rick Eyre (:reyre)]

Attachment: v2: Implement TextTrackList::GetTrackById()

Thanks for the review Ralph.

Should we get anyone else to review this as well?

Comment 5

[Caitlin Potter (:caitp)]

You should just close it and give up on this ****

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Ralph Giles (:rillian) from comment #2)
> Comment on attachment 772679 [details] [diff] [review]
> v1: Implement TextTrackList::GetTrackById()

> ::: content/media/TextTrackList.cpp
> @@ +60,5 @@
> > +  nsAutoString id;
> > +  for (uint32_t i = 0; i < Length(); i++) {
> > +    mTextTracks[i]->GetId(id);
> > +    if (aId.Equals(id)) {
> > +      return mTextTracks[i].forget();
> 
> Help me understand the memory management here. Why do we forget the
> reference if we're returning an already_AddRefed?

forget() clears the smart pointers reference without calling Release() on the object pointed to, i.e. forget() returns an already_AddRefed<T>, the object already has an addref on it.

Does that answer your question, or were you looking for a higher level overview of what releases the last ref to that object? (Rick would need to give you that).

Comment 7

[Rick Eyre (:reyre)]

(In reply to Chris Pearce (:cpearce) from comment #6)

> forget() clears the smart pointers reference without calling Release() on
> the object pointed to, i.e. forget() returns an already_AddRefed<T>, the
> object already has an addref on it.

Should we be incrementing the ref count before passing it back to the JS side? In my last patch I'm just returning an already_AddRefed constructed from the nsRefPtr without incrementing the refcount. To my mind this means that when JS deletes its TextTrack then the next time we try to access that TextTrack in mTextTracks we will crash. Is that correct?

Comment 8

[Ralph Giles (:rillian)]

Comment on attachment 773327 [details] [diff] [review]
v2: Implement TextTrackList::GetTrackById()

Review of attachment 773327 [details] [diff] [review]:
-----------------------------------------------------------------

Chris answered my question. Thanks.

The return foo.forget(); pattern is cleaner, please use what you originally wrote.

Comment 9

[Ralph Giles (:rillian)]

er, sorry, the original was wrong. More tomorrow.

Comment 10

[Rick Eyre (:reyre)]

Attachment: v3: Implement TextTrackList::GetTrackById()

Switched over to returning a raw pointer instead of an already_AddRefed<TextTrack>

Comment 11

[Ralph Giles (:rillian)]

Comment on attachment 774182 [details] [diff] [review]
v3: Implement TextTrackList::GetTrackById()

Review of attachment 774182 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me. Thanks!

Comment 12

[Ralph Giles (:rillian)]

To summarize irc discussions on the memory issue, Rick is correct comment 7. If returning an already_AddRef we need to bump the reference count, The usual pattern for this is to have a local nsRefPtr which bumps the count by assignment, and then return track.forget() to block its destructor from calling Release. In this case we don't already have a local pointer, so it would have been cleaner to call mTextTracks[i]->AddRef() directly before returning.

A shorter option is what Rick does here, just returning mTextTracks[i] directly as a raw pointer. This will be implicitly converted to a TextTrack* though nsRefPtr's operator T*() method. The bindings assign that a new nsRefPtr<TextTrack> which again bumps the refcount like we want.

Either approach is fine. Returning an already_AddRefed is safer, as the type system will block compilation if the caller assigns it to a dumb pointer, which would leak. Since this will likely only be called by the bindings or local code, manual compliance seemed to me a reasonable risk for the cleaner code.

Comment 13

[Rick Eyre (:reyre)]

Thanks for the review Ralph and the explanation :).

Try: https://tbpl.mozilla.org/?tree=Try&rev=7757bd997a80

Comment 14

[Rick Eyre (:reyre)]

Try looks green so marking as checkin now.

Please note that tests for this will land in bug 891381 when ID starts getting set to something meaningful.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8a375d3c1b43

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8a375d3c1b43