839711 - SVGAnimatedPreserveAspectRatio should return the same object every time for baseVal and animVal

Status: NEW

(Core :: SVG, defect)

Description

[David Zbarsky (:dzbarsky)]

Attachment: Patch

Comment 1

[Daniel Holbert [:dholbert]]

Seems like this should include an automated test. (probably a mochitest?)

Comment 2

[Jonathan Watt [:jwatt]]

David, what are you trying to achieve here? The idea of the current code is that we return the same object if content authors will be able to tell whether the object is different, but at the same time we allow the object to die to free memory (and subsequently return a different object) if content can't tell. We achieve this by only removing the object from from the hash table if its refcount drops to zero.

You seem to be changing from "callers get an addrefed object and have a strong reference" to "the DOMSVGAnimatedPreserveAspectRatio maintains an addref, but (?) callers don't and so depend on the DOMSVGAnimatedPreserveAspectRatio"? Or something? I don't understand how that could work.

Comment 3

[David Zbarsky (:dzbarsky)]

That's not quite correct.  A test like this would fail:

svg = document.createElementNS("http://www.w3.org/2000/svg", "svg");
svg.preserveAspectRatio.baseVal.x = 1;
//other things that will end up GCing
is(svg.preserveAspectRatio.baseVal.x, 1, "oh no!")

because script is not holding a ref to the object.  Therefore DOMSVGAnimatedPreserveAspectRatio should hold a ref to the values.  I think it's safe to hand out a weak ref because the table keeps DOMSVGPreserveAspectRatio alive until DOMSVGAnimatedPreserveAspectRatio is destroyed, at which point there's no way to reach the DOMSVGPreserveAspectRatio anymore.

Comment 4

[Jonathan Watt [:jwatt]]

(In reply to David Zbarsky (:dzbarsky) from comment #3)
> That's not quite correct.  A test like this would fail:
> 
> svg = document.createElementNS("http://www.w3.org/2000/svg", "svg");
> svg.preserveAspectRatio.baseVal.x = 1;
> //other things that will end up GCing
> is(svg.preserveAspectRatio.baseVal.x, 1, "oh no!")

Good point. But what prevents the DOMSVGAnimatedPreserveAspectRatio from being GC'ed here and causing this test to fail?

> I think
> it's safe to hand out a weak ref because the table keeps
> DOMSVGPreserveAspectRatio alive until DOMSVGAnimatedPreserveAspectRatio is
> destroyed, at which point there's no way to reach the
> DOMSVGPreserveAspectRatio anymore.

What if I do:

var svg = document.createElementNS("http://www.w3.org/2000/svg", "svg");
var par = svg.preserveAspectRatio.baseVal;
//other things that will end up GCing
par.foo = 1;

I probably don't understand how this stuff works, but it seems if 'par' isn't a strong ref then we could end up using bad memory here.

Comment 5

[David Zbarsky (:dzbarsky)]

(In reply to Jonathan Watt [:jwatt] from comment #4)
> 
> Good point. But what prevents the DOMSVGAnimatedPreserveAspectRatio from
> being GC'ed here and causing this test to fail?
> 

I don't think anything does.  I'll need to convert DOMSVGAnimatedPreserveAspectRatio too.

> What if I do:
> 
> var svg = document.createElementNS("http://www.w3.org/2000/svg", "svg");
> var par = svg.preserveAspectRatio.baseVal;
> //other things that will end up GCing
> par.foo = 1;
> 
> I probably don't understand how this stuff works, but it seems if 'par'
> isn't a strong ref then we could end up using bad memory here.

I believe that if script is holding it in a variable it will be strong, it's only weak if it's not stored anywhere.

Comment 6

[Jonathan Watt [:jwatt]]

(In reply to David Zbarsky (:dzbarsky) from comment #5)
> I don't think anything does.  I'll need to convert
> DOMSVGAnimatedPreserveAspectRatio too.

Okay. Please do that in the same patch.

> I believe that if script is holding it in a variable it will be strong, it's
> only weak if it's not stored anywhere.

Makes sense.

Comment 7

[Jonathan Watt [:jwatt]]

Comment on attachment 712043 [details] [diff] [review]
Patch

r-'ing given DOMSVGAnimatedPreserveAspectRatio needs the same treatment to fix this bug.

Comment 8

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.