Closed
Bug 905320
Opened 11 years ago
Closed 11 years ago
TextTrack does not run the base class Traverse/Unlink methods
Categories
(Core :: Audio/Video, defect)
Core
Audio/Video
Tracking
()
RESOLVED
FIXED
mozilla26
People
(Reporter: khuey, Assigned: reyre)
References
Details
(Keywords: memory-leak, Whiteboard: [MemShrink])
Attachments
(2 files, 1 obsolete file)
(deleted),
patch
|
khuey
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review |
> 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.
Reporter | ||
Comment 1•11 years ago
|
||
TextTrackCue and TextTrackList have the same problem.
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → rick.eyre
Assignee | ||
Comment 2•11 years ago
|
||
This is probably what is causing bug 895305.
Assignee | ||
Comment 3•11 years ago
|
||
Attachment #790834 -
Flags: review?(khuey)
Reporter | ||
Updated•11 years ago
|
Attachment #790834 -
Flags: review?(khuey) → review+
Assignee | ||
Comment 4•11 years ago
|
||
Thanks for review Kyle.
Try push: https://tbpl.mozilla.org/?tree=Try&rev=5ad29c005d9b
Assignee | ||
Comment 5•11 years ago
|
||
Attachment #791289 -
Flags: review?(khuey)
Reporter | ||
Comment 6•11 years ago
|
||
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+
Assignee | ||
Comment 7•11 years ago
|
||
(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.
Assignee | ||
Comment 9•11 years ago
|
||
Try push with test: https://tbpl.mozilla.org/?tree=Try&rev=abfeb40e7868
Assignee | ||
Comment 10•11 years ago
|
||
Updating to use tabs instead of spaces in the Makefile.
Carrying forward r=khuey
Attachment #791289 -
Attachment is obsolete: true
Assignee | ||
Comment 11•11 years ago
|
||
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
Comment 12•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/6245b87ea6b1
https://hg.mozilla.org/integration/mozilla-inbound/rev/330b7c0fe248
Flags: in-testsuite+
Keywords: checkin-needed
Comment 13•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/6245b87ea6b1
https://hg.mozilla.org/mozilla-central/rev/330b7c0fe248
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in
before you can comment on or make changes to this bug.
Description
•