Closed
Bug 467671
Opened 16 years ago
Closed 16 years ago
Leak 6 nsGlobalWindows due to DOMAnimatedLength not participating in cycle collection
Categories
(Core :: SVG, defect)
Core
SVG
Tracking
()
VERIFIED
FIXED
mozilla1.9.1
People
(Reporter: jruderman, Assigned: peterv)
References
Details
(Keywords: memory-leak, testcase, verified1.9.1)
Attachments
(2 files, 2 obsolete files)
(deleted),
image/svg+xml
|
Details | |
(deleted),
patch
|
longsonr
:
review+
jst
:
superreview+
beltzner
:
approval1.9.1+
|
Details | Diff | Splinter Review |
This SVG testcase somehow manages to make Firefox leak _SIX_ nsGlobalWindows.
Comment 1•16 years ago
|
||
This seems to be worksforme...
Reporter | ||
Comment 2•16 years ago
|
||
The original testcase only triggers the bug if you have a Greasemonkey script that adds an event listener, such as "Check Range". This testcase triggers the bug in a fresh profile.
Attachment #351080 -
Attachment is obsolete: true
Comment 3•16 years ago
|
||
So what's really happening here is that Object.prototype.toSource ends up having a property set to a DOMAnimatedLength.
The event listener presumably entrains the whole shebang.
DOMAnimatedLength holds a strong ref to a content node and doesn't participate in cycle collection. It should do so, and I suspect that would fix this bug.
The <switch> doesn't seem to be needed to reproduce the leak with that last testcase, right? And the ({}).toSource thing can be replaced with Object.prototype.toSource.
Comment 4•16 years ago
|
||
Heck, you can assign the DOMAnimatedLength as a property of Object.prototype directly to get the leak.
Reporter | ||
Updated•16 years ago
|
Summary: Leak 6 nsGlobalWindows with <svg:switch> and some nonsense → Leak 6 nsGlobalWindows due to DOMAnimatedLength not participating in cycle collection
Assignee | ||
Comment 5•16 years ago
|
||
Yikes, there's a fair number of classes we need to hook up.
Assignee: nobody → peterv
Status: NEW → ASSIGNED
OS: Mac OS X → All
Hardware: PC → All
Target Milestone: --- → mozilla1.9.1
Assignee | ||
Comment 6•16 years ago
|
||
Comment 7•16 years ago
|
||
Peter, were you planning on asking for a review of this?
Assignee | ||
Comment 8•16 years ago
|
||
Nope, I was going to add an automated testcase and then I had to figure out why the previous patch still leaked.
Attachment #351565 -
Attachment is obsolete: true
Attachment #354447 -
Flags: superreview?(jst)
Attachment #354447 -
Flags: review?(longsonr)
Comment 9•16 years ago
|
||
Comment on attachment 354447 [details] [diff] [review]
v1.1
Adding a comment line indicating which type you are checking would be nice
// angle
>+storeSVGPropertyAsExpando("marker", "orientAngle");
// boolean
>+storeSVGPropertyAsExpando("feConvolveMatrix", "preserveAlpha");
// enum
>+storeSVGPropertyAsExpando("feConvolveMatrix", "edgeMode");
// special marker enum
>+storeSVGPropertyAsExpando("marker", "orientType");
// integer
>+storeSVGPropertyAsExpando("feConvolveMatrix", "orderX");
// length
>+storeSVGPropertyAsExpando("feConvolveMatrix", "x");
// number
>+storeSVGPropertyAsExpando("feConvolveMatrix", "divisor");
// string
>+storeSVGPropertyAsExpando("feConvolveMatrix", "in1");
Attachment #354447 -
Flags: review?(longsonr) → review+
Updated•16 years ago
|
Attachment #354447 -
Flags: superreview?(jst) → superreview+
Assignee | ||
Updated•16 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Comment 10•16 years ago
|
||
Does the new class value implentation for bug 471165 need cycle collection support too? Also what about the new style preserve aspect ratio for bug 470911?
Yeah, all the tearoff DOMAnimated* classes need to participate since they hold strong refs to elements.
Assignee | ||
Comment 12•16 years ago
|
||
Comment on attachment 354447 [details] [diff] [review]
v1.1
Should get leak fixes into 1.9.1.
Attachment #354447 -
Flags: approval1.9.1?
Comment 13•16 years ago
|
||
Comment on attachment 354447 [details] [diff] [review]
v1.1
a191=beltzner
please add a comment with both the mozilla-central and mozilla-1.9.1 hg push IDs
Attachment #354447 -
Flags: approval1.9.1? → approval1.9.1+
Assignee | ||
Comment 14•16 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/9b0d41d19445
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/ac601c3cdcbd
Keywords: fixed1.9.1
Comment 15•16 years ago
|
||
Seeing that there haven't been any duplicate sightings for this issue for 2
months, I'm going to go ahead and verify this unless someone has an issue with
that.
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.1 → verified1.9.1
You need to log in
before you can comment on or make changes to this bug.
Description
•