467671 - Leak 6 nsGlobalWindows due to DOMAnimatedLength not participating in cycle collection

Status: VERIFIED FIXED

(Core :: SVG, defect)

Description

[Jesse Ruderman]

Attachment: testcase

This SVG testcase somehow manages to make Firefox leak _SIX_ nsGlobalWindows.

Comment 1

[Boris Zbarsky [:bzbarsky]]

This seems to be worksforme...

Comment 2

[Jesse Ruderman]

Attachment: 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.

Comment 3

[Boris Zbarsky [:bzbarsky]]

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

[Boris Zbarsky [:bzbarsky]]

Heck, you can assign the DOMAnimatedLength as a property of Object.prototype directly to get the leak.

Comment 5

[Peter Van der Beken [:peterv]]

Yikes, there's a fair number of classes we need to hook up.

Comment 6

[Peter Van der Beken [:peterv]]

Attachment: v1

Comment 7

[Robert Longson [:longsonr]]

Peter, were you planning on asking for a review of this?

Comment 8

[Peter Van der Beken [:peterv]]

Attachment: v1.1

Nope, I was going to add an automated testcase and then I had to figure out why the previous patch still leaked.

Comment 9

[Robert Longson [: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");

Comment 10

[Craig Topper]

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?

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yeah, all the tearoff DOMAnimated* classes need to participate since they hold strong refs to elements.

Comment 12

[Peter Van der Beken [:peterv]]

Comment on attachment 354447 [details] [diff] [review]
v1.1

Should get leak fixes into 1.9.1.

Comment 13

[Mike Beltzner [:beltzner, not reading bugmail]]

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

Comment 14

[Peter Van der Beken [:peterv]]

http://hg.mozilla.org/mozilla-central/rev/9b0d41d19445
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/ac601c3cdcbd

Comment 15

[Aakash Desai [:aakashd]]

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.