905320 - TextTrack does not run the base class Traverse/Unlink methods

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

> NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_3(TextTrack,
>                                         mParent,
>                                         mCueList,
>                                         mActiveCueList)

But TextTrack inherits from nsDOMEventTargetHelper.  Since this has wrapper cache in it we don't have a security issue, but we're not traversing/unlinking any of the C++ members of TextTrack.  You can see the results if you add an event listener that closes over the TextTrack object.

This code wants to use NS_IMPL_CYCLE_COLLECTION_INHERITED_3 instead.  Also it can drop the script holder bit from the macro in the header.

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

TextTrackCue and TextTrackList have the same problem.

Comment 2

[Rick Eyre (:reyre)]

This is probably what is causing bug 895305.

Comment 3

[Rick Eyre (:reyre)]

Attachment: v1: Fix TextTrack classes to utilize cycle collection correctly

Comment 4

[Rick Eyre (:reyre)]

Thanks for review Kyle.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=5ad29c005d9b

Comment 5

[Rick Eyre (:reyre)]

Attachment: Part 2 v1: Add regression tests for TextTrack* leaks

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 791289 [details] [diff] [review]
Part 2 v1: Add regression tests for TextTrack* leaks

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

You tested that this does in fact leak without the fix, right?

Comment 7

[Rick Eyre (:reyre)]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #6)
> Comment on attachment 791289 [details] [diff] [review]
> Part 2 v1: Add regression tests for TextTrack* leaks
> 
> Review of attachment 791289 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> You tested that this does in fact leak without the fix, right?

Good question! No, I hadn't, but just did now and it does. Leaks when there is no fix and doesn't leak when there is. I'll keep that in mind for next time.

Comment 9

[Rick Eyre (:reyre)]

Try push with test: https://tbpl.mozilla.org/?tree=Try&rev=abfeb40e7868

Comment 10

[Rick Eyre (:reyre)]

Attachment: Part 2 v2: Add regression tests for TextTrack* leaks

Updating to use tabs instead of spaces in the Makefile.

Carrying forward r=khuey

Comment 11

[Rick Eyre (:reyre)]

Try looks green except for opt Android 4.0 R5 build. However, looking through the other trys I see that this is a common problem so it's not related to this specific code.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6245b87ea6b1
https://hg.mozilla.org/integration/mozilla-inbound/rev/330b7c0fe248

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6245b87ea6b1
https://hg.mozilla.org/mozilla-central/rev/330b7c0fe248