Closed Bug 467671 Opened 12 years ago Closed 12 years ago

Leak 6 nsGlobalWindows due to DOMAnimatedLength not participating in cycle collection

Categories

(Core :: SVG, defect)

defect
Not set
major

Tracking

()

VERIFIED FIXED
mozilla1.9.1

People

(Reporter: jruderman, Assigned: peterv)

References

(Blocks 1 open bug)

Details

(Keywords: memory-leak, testcase, verified1.9.1)

Attachments

(2 files, 2 obsolete files)

Attached image testcase (obsolete) —
This SVG testcase somehow manages to make Firefox leak _SIX_ nsGlobalWindows.
This seems to be worksforme...
Attached image better testcase
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
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.
Heck, you can assign the DOMAnimatedLength as a property of Object.prototype directly to get the leak.
Summary: Leak 6 nsGlobalWindows with <svg:switch> and some nonsense → Leak 6 nsGlobalWindows due to DOMAnimatedLength not participating in cycle collection
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
Attached patch v1 (obsolete) — Splinter Review
Peter, were you planning on asking for a review of this?
Attached patch v1.1Splinter Review
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 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+
Attachment #354447 - Flags: superreview?(jst) → superreview+
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
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.
Comment on attachment 354447 [details] [diff] [review]
v1.1

Should get leak fixes into 1.9.1.
Attachment #354447 - Flags: approval1.9.1?
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+
Blocks: 471165
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
You need to log in before you can comment on or make changes to this bug.