Crash when accessing an item in SVGTransformList and then removing a previous item...
authorsaid@apple.com <said@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 16 Feb 2015 02:08:39 +0000 (02:08 +0000)
committersaid@apple.com <said@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 16 Feb 2015 02:08:39 +0000 (02:08 +0000)
https://bugs.webkit.org/show_bug.cgi?id=141550.

Reviewed by Darin Adler.

Source/WebCore:

Tests: LayoutTests/svg/dom/SVGTransformList-basics.xhtml: This test is modified to
include a new test case.

* svg/properties/SVGMatrixTearOff.h: m_value of SVGMatrixTearOff will be a null
pointer. There is no point in having SVGMatrixTearOff points to the parent and
the property of the parent at the same time.

(WebCore::SVGMatrixTearOff::create): SVGMatrixTearOff will hold a pointer to
the parent SVGPropertyTearOff<SVGTransform>. But it should overrides setValue()
and propertyReference() so it can set and get the SVGMatrix from the SVGTransform
parent.

(WebCore::SVGMatrixTearOff::SVGMatrixTearOff): Pass a nullptr to the base class.
SVGMatrixTearOff will act as a proxy of the parent. It does not hold any data by
itself but it knows what property to set and get from the parent.

* svg/properties/SVGPropertyTearOff.h:
(WebCore::SVGPropertyTearOff::create): Add a create method which can take a pointer value.

(WebCore::SVGPropertyTearOff::propertyReference):
(WebCore::SVGPropertyTearOff::setValue): Make these functions virtual so concrete classes
like SVGMatrixTearOff can override them.

(WebCore::SVGPropertyTearOff::SVGPropertyTearOff): Add a new constructor.

LayoutTests:

* svg/dom/SVGTransformList-basics-expected.txt:
* svg/dom/SVGTransformList-basics.xhtml: Add a new test case to this test. Have a
reference to an SVGMatrix of the last SVGTransform of an SVGTransformList and then
remove the items of this list from the beginning till the end.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@180129 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/svg/dom/SVGTransformList-basics-expected.txt
LayoutTests/svg/dom/SVGTransformList-basics.xhtml
Source/WebCore/ChangeLog
Source/WebCore/svg/properties/SVGMatrixTearOff.h
Source/WebCore/svg/properties/SVGPropertyTearOff.h

index bb8d7cc..6cfdff6 100644 (file)
@@ -1,5 +1,17 @@
 2015-02-15  Said Abou-Hallawa  <sabouhallawa@apple.com>
 
+        Crash when accessing an item in SVGTransformList and then removing a previous item from this list.
+        https://bugs.webkit.org/show_bug.cgi?id=141550.
+
+        Reviewed by Darin Adler.
+
+        * svg/dom/SVGTransformList-basics-expected.txt:
+        * svg/dom/SVGTransformList-basics.xhtml: Add a new test case to this test. Have a
+        reference to an SVGMatrix of the last SVGTransform of an SVGTransformList and then
+        remove the items of this list from the beginning till the end.
+
+2015-02-15  Said Abou-Hallawa  <sabouhallawa@apple.com>
+
         Crash when accessing an item in SVGLengthList and then replacing it with a previous item in the list.
         https://bugs.webkit.org/show_bug.cgi?id=141552.
 
index 545cf7b..c2bdcc8 100644 (file)
@@ -51,6 +51,13 @@ PASS circle1.transform.baseVal.insertItemBefore('aString', 0) threw exception Ty
 PASS circle1.transform.baseVal.insertItemBefore(circle1, 0) threw exception TypeError: Argument 1 ('item') to SVGTransformList.insertItemBefore must be an instance of SVGTransform.
 PASS circle1.transform.baseVal.insertItemBefore(null, 0) threw exception Error: SVG_WRONG_TYPE_ERR: DOM SVG Exception 0.
 
+Test overlapping edge cases for removeItem()
+PASS circle1.setAttribute('transform', 'scale(2 2) translate(10 10)') is undefined.
+PASS dumpTransform(circle1.transform.baseVal.getItem(1)) is "type=SVG_TRANSFORM_MATRIX matrix=[1.0 0.0 0.0 1.0 20.0 20.0]"
+PASS dumpTransform(circle1.transform.baseVal.getItem(1)) is "type=SVG_TRANSFORM_MATRIX matrix=[1.0 0.0 0.0 1.0 20.0 20.0]"
+PASS dumpTransform(circle1.transform.baseVal.getItem(1)) is "type=SVG_TRANSFORM_MATRIX matrix=[1.0 0.0 0.0 1.0 40.0 40.0]"
+PASS circle1.transform.baseVal.numberOfItems is 0
+
 Set transform='rotate(90) scale(2 2) translate(10 10) skewX(45)' for circle1
 PASS circle1.setAttribute('transform', 'rotate(90) scale(2 2) translate(10 10) skewX(45)') is undefined.
 PASS circle1.transform.baseVal.numberOfItems is 4
index 66c85b6..fd2cafa 100644 (file)
     shouldThrow("circle1.transform.baseVal.insertItemBefore(null, 0)");
 
     debug("");
+    debug("Test overlapping edge cases for removeItem()");
+    shouldBeUndefined("circle1.setAttribute('transform', 'scale(2 2) translate(10 10)')");
+    var matrix = circle1.transform.baseVal.getItem(1).matrix;
+    matrix.e *= 2;
+    matrix.f *= 2;
+    shouldBeEqualToString("dumpTransform(circle1.transform.baseVal.getItem(1))", "type=SVG_TRANSFORM_MATRIX matrix=[1.0 0.0 0.0 1.0 20.0 20.0]");
+    matrix = matrix.translate(20, 20);
+    shouldBeEqualToString("dumpTransform(circle1.transform.baseVal.getItem(1))", "type=SVG_TRANSFORM_MATRIX matrix=[1.0 0.0 0.0 1.0 20.0 20.0]");
+    circle1.transform.baseVal.getItem(1).setMatrix(matrix);
+    shouldBeEqualToString("dumpTransform(circle1.transform.baseVal.getItem(1))", "type=SVG_TRANSFORM_MATRIX matrix=[1.0 0.0 0.0 1.0 40.0 40.0]");
+    circle1.transform.baseVal.removeItem(0);
+    circle1.transform.baseVal.removeItem(0);
+    shouldBe("circle1.transform.baseVal.numberOfItems", "0");
+
+    debug("");
     debug("Set transform='rotate(90) scale(2 2) translate(10 10) skewX(45)' for circle1");
     shouldBeUndefined("circle1.setAttribute('transform', 'rotate(90) scale(2 2) translate(10 10) skewX(45)')");
     shouldBe("circle1.transform.baseVal.numberOfItems", "4");
index f7f5a5b..b627e8f 100644 (file)
@@ -1,5 +1,37 @@
 2015-02-15  Said Abou-Hallawa  <sabouhallawa@apple.com>
 
+        Crash when accessing an item in SVGTransformList and then removing a previous item from this list.
+        https://bugs.webkit.org/show_bug.cgi?id=141550.
+
+        Reviewed by Darin Adler.
+
+        Tests: LayoutTests/svg/dom/SVGTransformList-basics.xhtml: This test is modified to
+        include a new test case.
+
+        * svg/properties/SVGMatrixTearOff.h: m_value of SVGMatrixTearOff will be a null
+        pointer. There is no point in having SVGMatrixTearOff points to the parent and
+        the property of the parent at the same time.
+        
+        (WebCore::SVGMatrixTearOff::create): SVGMatrixTearOff will hold a pointer to
+        the parent SVGPropertyTearOff<SVGTransform>. But it should overrides setValue()
+        and propertyReference() so it can set and get the SVGMatrix from the SVGTransform
+        parent.
+        
+        (WebCore::SVGMatrixTearOff::SVGMatrixTearOff): Pass a nullptr to the base class.
+        SVGMatrixTearOff will act as a proxy of the parent. It does not hold any data by
+        itself but it knows what property to set and get from the parent.
+        
+        * svg/properties/SVGPropertyTearOff.h:
+        (WebCore::SVGPropertyTearOff::create): Add a create method which can take a pointer value.
+        
+        (WebCore::SVGPropertyTearOff::propertyReference):
+        (WebCore::SVGPropertyTearOff::setValue): Make these functions virtual so concrete classes
+        like SVGMatrixTearOff can override them.
+        
+        (WebCore::SVGPropertyTearOff::SVGPropertyTearOff): Add a new constructor.
+
+2015-02-15  Said Abou-Hallawa  <sabouhallawa@apple.com>
+
         Crash when accessing an item in SVGLengthList and then replacing it with a previous item in the list.
         https://bugs.webkit.org/show_bug.cgi?id=141552.
 
index b5c584c..51255b4 100644 (file)
@@ -30,13 +30,18 @@ public:
     // Used for non-animated POD types that are not associated with a SVGAnimatedProperty object, nor with a XML DOM attribute
     // and that contain a parent type that's exposed to the bindings via a SVGStaticPropertyTearOff object
     // (for example: SVGTransform::matrix).
-    static PassRefPtr<SVGMatrixTearOff> create(SVGPropertyTearOff<SVGTransform>& parent, SVGMatrix& value)
+    static Ref<SVGMatrixTearOff> create(SVGPropertyTearOff<SVGTransform>& parent, SVGMatrix& value)
     {
-        RefPtr<SVGMatrixTearOff> result = adoptRef(new SVGMatrixTearOff(&parent, value));
+        ASSERT(&parent.propertyReference().svgMatrix() == &value);
+        Ref<SVGMatrixTearOff> result = adoptRef(*new SVGMatrixTearOff(&parent));
         parent.addChild(result->m_weakFactory.createWeakPtr());
-        return result.release();
+        return result;
     }
 
+    virtual SVGMatrix& propertyReference() override { return m_parent->propertyReference().svgMatrix(); }
+
+    virtual void setValue(SVGMatrix& value) override { m_parent->propertyReference().setMatrix(value); }
+
     virtual void commitChange()
     {
         m_parent->propertyReference().updateSVGMatrix();
@@ -44,8 +49,8 @@ public:
     }
 
 private:
-    SVGMatrixTearOff(SVGPropertyTearOff<SVGTransform>* parent, SVGMatrix& value)
-        : SVGPropertyTearOff<SVGMatrix>(0, UndefinedRole, value)
+    SVGMatrixTearOff(SVGPropertyTearOff<SVGTransform>* parent)
+        : SVGPropertyTearOff<SVGMatrix>(nullptr)
         , m_parent(parent)
         , m_weakFactory(this)
     {
index 644ec74..7ccd0d0 100644 (file)
@@ -39,22 +39,27 @@ public:
 
     // Used for child types (baseVal/animVal) of a SVGAnimated* property (for example: SVGAnimatedLength::baseVal()).
     // Also used for list tear offs (for example: text.x.baseVal.getItem(0)).
-    static PassRefPtr<Self> create(SVGAnimatedProperty* animatedProperty, SVGPropertyRole role, PropertyType& value)
+    static Ref<Self> create(SVGAnimatedProperty* animatedProperty, SVGPropertyRole role, PropertyType& value)
     {
         ASSERT(animatedProperty);
-        return adoptRef(new Self(animatedProperty, role, value));
+        return adoptRef(*new Self(animatedProperty, role, value));
     }
 
     // Used for non-animated POD types (for example: SVGSVGElement::createSVGLength()).
-    static PassRefPtr<Self> create(const PropertyType& initialValue)
+    static Ref<Self> create(const PropertyType& initialValue)
     {
-        return adoptRef(new Self(initialValue));
+        return adoptRef(*new Self(initialValue));
     }
 
-    PropertyType& propertyReference() { return *m_value; }
+    static Ref<Self> create(const PropertyType* initialValue)
+    {
+        return adoptRef(*new Self(initialValue));
+    }
+
+    virtual PropertyType& propertyReference() { return *m_value; }
     SVGAnimatedProperty* animatedProperty() const { return m_animatedProperty; }
 
-    void setValue(PropertyType& value)
+    virtual void setValue(PropertyType& value)
     {
         if (m_valueIsCopy) {
             detachChildren();
@@ -134,9 +139,14 @@ protected:
     }
 
     SVGPropertyTearOff(const PropertyType& initialValue)
+        : SVGPropertyTearOff(&initialValue)
+    {
+    }
+
+    SVGPropertyTearOff(const PropertyType* initialValue)
         : m_animatedProperty(0)
         , m_role(UndefinedRole)
-        , m_value(new PropertyType(initialValue))
+        , m_value(initialValue ? new PropertyType(*initialValue) : nullptr)
         , m_valueIsCopy(true)
     {
     }