Closed Bug 905320 Opened 6 years ago Closed 6 years ago

TextTrack does not run the base class Traverse/Unlink methods

Categories

(Core :: Audio/Video, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: khuey, Assigned: reyre)

References

Details

(Keywords: memory-leak, Whiteboard: [MemShrink])

Attachments

(2 files, 1 obsolete file)

> 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.
Keywords: mlk
Whiteboard: [MemShrink]
TextTrackCue and TextTrackList have the same problem.
Assignee: nobody → rick.eyre
This is probably what is causing bug 895305.
Attachment #790834 - Flags: review?(khuey) → review+
Thanks for review Kyle.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=5ad29c005d9b
Attachment #791289 - Flags: review?(khuey)
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?
Attachment #791289 - Flags: review?(khuey) → review+
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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.
Duplicate of this bug: 895305
Updating to use tabs instead of spaces in the Makefile.

Carrying forward r=khuey
Attachment #791289 - Attachment is obsolete: true
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.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6245b87ea6b1
https://hg.mozilla.org/mozilla-central/rev/330b7c0fe248
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.