Open Bug 839711 Opened 7 years ago Updated 7 years ago

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

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

ASSIGNED

People

(Reporter: dzbarsky, Assigned: dzbarsky)

Details

Attachments

(1 file)

Attached patch PatchSplinter Review
No description provided.
Attachment #712043 - Flags: review?(jwatt)
Seems like this should include an automated test. (probably a mochitest?)
Flags: in-testsuite?
Version: unspecified → Trunk
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.
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.
(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.
(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.
(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 on attachment 712043 [details] [diff] [review]
Patch

r-'ing given DOMSVGAnimatedPreserveAspectRatio needs the same treatment to fix this bug.
Attachment #712043 - Flags: review?(jwatt) → review-
You need to log in before you can comment on or make changes to this bug.