Bug 17736 – JS (SVG) POD Type bindings bug

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Nikolas Zimmermann]

As the summary says I found a pretty harsh JS POD type bindings bug, while writing tests for SVGLinearGradientElement & updating of the gradientTransform attribute.

Try in trunk:
var transform = mySVGSVGElement.createSVGTransform();
alert(transform.matrix.a); returns '1.0' as expected
transform.matrix.a = 2;
alert(transform.matrix.a); returns '1.0' :(

Another way to trigger the bug without having to create a new SVGTransform object:
myLinearGradientElement.gradientTransform.baseVal.getItem(0).matrix.a = 2;
alert(myLinearGradientElement.gradientTransform.baseVal.getItem(0).matrix.a); returns '1.0'

The problem is that we forgot to handle the case where a JSSVGPODTypeWrapper is able to create JSSVGPODTypeWrappers itself. In our case 'SVGTransform' contains 'readonly attribute SVGMatrix matrix;". As SVGTransform & SVGMatrix are both POD types our updating logic fails. It only works for SVG elements containing POD types (ie. a SVGRectElement contains a SVG(Animated)Length POD type => when modifying the POD Type the SVGRectElement gets updated correctly.)

Not yet sure about possible solutions, still investigating.

[Jonathan Haas]

Created attachment 19676 [details]
Layout test demonstrating this bug

Repro code from the bug repackaged as a layout test

[Jonathan Haas]

Created attachment 19746 [details]
Layout test for this bug

eseidel introduced me to the wonders of the JavaScript layout test framework, so I reworked this test to use it. Added expected results, too, and formatted as a patch.

[Nikolas Zimmermann]

Eek, fixed this bug a while ago. Forgot to submit the actual patch. Going to attach it soon.

[Nikolas Zimmermann]

Created attachment 22626 [details]
Initial patch

Fixes all problem with POD types containing POD types.

[Oliver Hunt]

I think antti should review this.

[Antti Koivisto]

Comment on attachment 22626 [details]
Initial patch

Please add a test for the Immutable marker, otherwise r=me

[Nikolas Zimmermann]

Landed in r35674, with a new test 'immutable-properties' which tests all cases of the new 'Immutable' IDL flag.