[SVG] OOB access in SVGListProperty::replaceItemValues()
authorfmalita@chromium.org <fmalita@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 13 Feb 2013 17:49:47 +0000 (17:49 +0000)
committerfmalita@chromium.org <fmalita@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 13 Feb 2013 17:49:47 +0000 (17:49 +0000)
https://bugs.webkit.org/show_bug.cgi?id=109293

Source/WebCore:

Replacing a list property item with itself should be a no-op. This patch updates the related
APIs and logic to detect the self-replace case and prevent removal of the item from the list.

To avoid scanning the list multiple times, removeItemFromList() is updated to operate on
indices and a findItem() method is added to resolve an item to an index.

Reviewed by Dirk Schulze.

No new tests: updated existing tests cover the change.

* svg/properties/SVGAnimatedListPropertyTearOff.h:
(WebCore::SVGAnimatedListPropertyTearOff::findItem):
(SVGAnimatedListPropertyTearOff):
(WebCore::SVGAnimatedListPropertyTearOff::removeItemFromList):
* svg/properties/SVGAnimatedPathSegListPropertyTearOff.h:
(WebCore::SVGAnimatedPathSegListPropertyTearOff::findItem):
(SVGAnimatedPathSegListPropertyTearOff):
(WebCore::SVGAnimatedPathSegListPropertyTearOff::removeItemFromList):
Add a findItem() delegating method, and update removeItemFromList() to use the new
index-based API.

* svg/properties/SVGListProperty.h:
(WebCore::SVGListProperty::insertItemBeforeValues):
(WebCore::SVGListProperty::insertItemBeforeValuesAndWrappers):
(WebCore::SVGListProperty::replaceItemValues):
(WebCore::SVGListProperty::replaceItemValuesAndWrappers):
(SVGListProperty):
Updated to handle the no-op case for insertItemBefore() & replaceItem().

* svg/properties/SVGListPropertyTearOff.h:
(WebCore::SVGListPropertyTearOff::findItem):
(WebCore::SVGListPropertyTearOff::removeItemFromList):
Index-based API updates.

(WebCore::SVGListPropertyTearOff::processIncomingListItemValue):
(WebCore::SVGListPropertyTearOff::processIncomingListItemWrapper):
* svg/properties/SVGPathSegListPropertyTearOff.cpp:
(WebCore::SVGPathSegListPropertyTearOff::processIncomingListItemValue):
Detect the self-replace case and return without removing the item from the list.

* svg/properties/SVGPathSegListPropertyTearOff.h:
(WebCore::SVGPathSegListPropertyTearOff::findItem):
(WebCore::SVGPathSegListPropertyTearOff::removeItemFromList):
(SVGPathSegListPropertyTearOff):
(WebCore::SVGPathSegListPropertyTearOff::processIncomingListItemWrapper):
* svg/properties/SVGStaticListPropertyTearOff.h:
(WebCore::SVGStaticListPropertyTearOff::processIncomingListItemValue):
(WebCore::SVGStaticListPropertyTearOff::processIncomingListItemWrapper):
Index-based API updates.

LayoutTests:

Updated tests to cover the crash and new behavior.

Reviewed by Dirk Schulze.

* svg/dom/SVGLengthList-basics-expected.txt:
* svg/dom/SVGLengthList-basics.xhtml:
* svg/dom/SVGNumberList-basics-expected.txt:
* svg/dom/SVGNumberList-basics.xhtml:
* svg/dom/SVGPointList-basics-expected.txt:
* svg/dom/SVGPointList-basics.xhtml:
* svg/dom/SVGTransformList-basics-expected.txt:
* svg/dom/SVGTransformList-basics.xhtml:

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

17 files changed:
LayoutTests/ChangeLog
LayoutTests/svg/dom/SVGLengthList-basics-expected.txt
LayoutTests/svg/dom/SVGLengthList-basics.xhtml
LayoutTests/svg/dom/SVGNumberList-basics-expected.txt
LayoutTests/svg/dom/SVGNumberList-basics.xhtml
LayoutTests/svg/dom/SVGPointList-basics-expected.txt
LayoutTests/svg/dom/SVGPointList-basics.xhtml
LayoutTests/svg/dom/SVGTransformList-basics-expected.txt
LayoutTests/svg/dom/SVGTransformList-basics.xhtml
Source/WebCore/ChangeLog
Source/WebCore/svg/properties/SVGAnimatedListPropertyTearOff.h
Source/WebCore/svg/properties/SVGAnimatedPathSegListPropertyTearOff.h
Source/WebCore/svg/properties/SVGListProperty.h
Source/WebCore/svg/properties/SVGListPropertyTearOff.h
Source/WebCore/svg/properties/SVGPathSegListPropertyTearOff.cpp
Source/WebCore/svg/properties/SVGPathSegListPropertyTearOff.h
Source/WebCore/svg/properties/SVGStaticListPropertyTearOff.h

index cd11217..6325acb 100644 (file)
@@ -1,3 +1,21 @@
+2013-02-13  Florin Malita  <fmalita@chromium.org>
+
+        [SVG] OOB access in SVGListProperty::replaceItemValues()
+        https://bugs.webkit.org/show_bug.cgi?id=109293
+
+        Updated tests to cover the crash and new behavior.
+
+        Reviewed by Dirk Schulze.
+
+        * svg/dom/SVGLengthList-basics-expected.txt:
+        * svg/dom/SVGLengthList-basics.xhtml:
+        * svg/dom/SVGNumberList-basics-expected.txt:
+        * svg/dom/SVGNumberList-basics.xhtml:
+        * svg/dom/SVGPointList-basics-expected.txt:
+        * svg/dom/SVGPointList-basics.xhtml:
+        * svg/dom/SVGTransformList-basics-expected.txt:
+        * svg/dom/SVGTransformList-basics.xhtml:
+
 2013-02-13  Takashi Sakamoto  <tasak@google.com>
 
         [Refactoring] StyleResolver::State should have methods to access its me
index 254edef..f66e8f8 100644 (file)
@@ -59,6 +59,17 @@ PASS text1.x.baseVal.insertItemBefore(null, 0) threw exception Error: SVG_WRONG_
 
 Set x='1 2 3 4' for text1
 PASS text1.setAttribute('x', '1 2 3 4') is undefined.
+
+Test edge cases for insertItemBefore()
+PASS text1.x.baseVal.insertItemBefore(text1.x.baseVal.getItem(3), 3) is text1.x.baseVal.getItem(3)
+PASS text1.getAttribute('x') is "1 2 3 4"
+PASS text1.x.baseVal.insertItemBefore(text1.x.baseVal.getItem(1), 5) is text1.x.baseVal.getItem(3)
+PASS text1.getAttribute('x') is "1 3 4 2"
+PASS text1.x.baseVal.insertItemBefore(text1.x.baseVal.getItem(1), 0) is text1.x.baseVal.getItem(0)
+PASS text1.getAttribute('x') is "3 1 4 2"
+
+Set x='1 2 3 4' for text1
+PASS text1.setAttribute('x', '1 2 3 4') is undefined.
 PASS text1.x.baseVal.numberOfItems is 4
 PASS text1.x.baseVal.getItem(0).value is 1
 PASS text1.x.baseVal.getItem(1).value is 2
@@ -76,22 +87,34 @@ PASS text1.x.baseVal.replaceItem('aString', 0) threw exception TypeError: Type e
 PASS text1.x.baseVal.replaceItem(text1, 0) threw exception TypeError: Type error.
 PASS text1.x.baseVal.replaceItem(null, 0) threw exception Error: SVG_WRONG_TYPE_ERR: DOM SVG Exception 0.
 PASS text1.x.baseVal.replaceItem(text1.x.baseVal.getItem(0), 0) is text1.x.baseVal.getItem(0)
-PASS text1.x.baseVal.numberOfItems is 3
+PASS text1.x.baseVal.numberOfItems is 4
 PASS text1.x.baseVal.getItem(0).value is 1
-PASS text1.x.baseVal.getItem(1).value is 3
-PASS text1.x.baseVal.getItem(2).value is 4
-PASS text1.getAttribute('x') is "1 3 4"
+PASS text1.x.baseVal.getItem(1).value is 2
+PASS text1.x.baseVal.getItem(2).value is 3
+PASS text1.x.baseVal.getItem(3).value is 4
+PASS text1.getAttribute('x') is "1 2 3 4"
 PASS text1.x.baseVal.replaceItem(text1.x.baseVal.getItem(0), 'aString') is text1.x.baseVal.getItem(0)
-PASS text1.x.baseVal.numberOfItems is 2
+PASS text1.x.baseVal.numberOfItems is 4
 PASS text1.x.baseVal.getItem(0).value is 1
-PASS text1.x.baseVal.getItem(1).value is 4
-PASS text1.getAttribute('x') is "1 4"
+PASS text1.x.baseVal.getItem(1).value is 2
+PASS text1.x.baseVal.getItem(2).value is 3
+PASS text1.x.baseVal.getItem(3).value is 4
+PASS text1.getAttribute('x') is "1 2 3 4"
 PASS text1.x.baseVal.replaceItem(text1.x.baseVal.getItem(0), text1) is text1.x.baseVal.getItem(0)
-PASS text1.x.baseVal.numberOfItems is 1
-PASS text1.getAttribute('x') is "1"
-PASS text1.x.baseVal.replaceItem(text1.x.baseVal.getItem(0), null) threw exception Error: IndexSizeError: DOM Exception 1.
-PASS text1.x.baseVal.numberOfItems is 0
-PASS text1.getAttribute('x') is ""
+PASS text1.x.baseVal.numberOfItems is 4
+PASS text1.getAttribute('x') is "1 2 3 4"
+
+Set x='1 2 3 4' for text1
+PASS text1.setAttribute('x', '1 2 3 4') is undefined.
+
+Test edge cases for replaceItem()
+PASS text1.x.baseVal.replaceItem(text1.x.baseVal.getItem(3), 3) is text1.x.baseVal.getItem(3)
+PASS text1.x.baseVal.numberOfItems is 4
+PASS text1.getAttribute('x') is "1 2 3 4"
+PASS text1.x.baseVal.replaceItem(text1.x.baseVal.getItem(1), 3) is text1.x.baseVal.getItem(2)
+PASS text1.x.baseVal.numberOfItems is 3
+PASS text1.getAttribute('x') is "1 3 2"
+PASS text1.x.baseVal.replaceItem(text1.x.baseVal.getItem(3), 4) threw exception Error: IndexSizeError: DOM Exception 1.
 
 Set x='1 2 3 4' for text1
 PASS text1.setAttribute('x', '1 2 3 4') is undefined.
index 4924c37..405642e 100644 (file)
@@ -14,6 +14,9 @@
 <![CDATA[
     description("This is a test of the simple SVGLengthList API parts.");
 
+    if (window.testRunner)
+        testRunner.dumpAsText();
+
     var text1 = document.getElementById("text1");
  
     // Spec: The object referenced by animVal will always be distinct from the one referenced by baseVal, even when the attribute is not animated.
     debug("");
     debug("Set x='1 2 3 4' for text1");
     shouldBeUndefined("text1.setAttribute('x', '1 2 3 4')");
+
+    debug("");
+    debug("Test edge cases for insertItemBefore()");
+    shouldBe("text1.x.baseVal.insertItemBefore(text1.x.baseVal.getItem(3), 3)", "text1.x.baseVal.getItem(3)");
+    shouldBeEqualToString("text1.getAttribute('x')", "1 2 3 4");
+    shouldBe("text1.x.baseVal.insertItemBefore(text1.x.baseVal.getItem(1), 5)", "text1.x.baseVal.getItem(3)");
+    shouldBeEqualToString("text1.getAttribute('x')", "1 3 4 2");
+    shouldBe("text1.x.baseVal.insertItemBefore(text1.x.baseVal.getItem(1), 0)", "text1.x.baseVal.getItem(0)");
+    shouldBeEqualToString("text1.getAttribute('x')", "3 1 4 2");
+
+    debug("");
+    debug("Set x='1 2 3 4' for text1");
+    shouldBeUndefined("text1.setAttribute('x', '1 2 3 4')");
     shouldBe("text1.x.baseVal.numberOfItems", "4");
     shouldBe("text1.x.baseVal.getItem(0).value", "1");
     shouldBe("text1.x.baseVal.getItem(1).value", "2");
     shouldThrow("text1.x.baseVal.replaceItem(null, 0)");
 
     shouldBe("text1.x.baseVal.replaceItem(text1.x.baseVal.getItem(0), 0)", "text1.x.baseVal.getItem(0)");
-    shouldBe("text1.x.baseVal.numberOfItems", "3");
+    shouldBe("text1.x.baseVal.numberOfItems", "4");
     shouldBe("text1.x.baseVal.getItem(0).value", "1");
-    shouldBe("text1.x.baseVal.getItem(1).value", "3");
-    shouldBe("text1.x.baseVal.getItem(2).value", "4");
-    shouldBeEqualToString("text1.getAttribute('x')", "1 3 4");
+    shouldBe("text1.x.baseVal.getItem(1).value", "2");
+    shouldBe("text1.x.baseVal.getItem(2).value", "3");
+    shouldBe("text1.x.baseVal.getItem(3).value", "4");
+    shouldBeEqualToString("text1.getAttribute('x')", "1 2 3 4");
 
     shouldBe("text1.x.baseVal.replaceItem(text1.x.baseVal.getItem(0), 'aString')", "text1.x.baseVal.getItem(0)");
-    shouldBe("text1.x.baseVal.numberOfItems", "2");
+    shouldBe("text1.x.baseVal.numberOfItems", "4");
     shouldBe("text1.x.baseVal.getItem(0).value", "1");
-    shouldBe("text1.x.baseVal.getItem(1).value", "4");
-    shouldBeEqualToString("text1.getAttribute('x')", "1 4");
+    shouldBe("text1.x.baseVal.getItem(1).value", "2");
+    shouldBe("text1.x.baseVal.getItem(2).value", "3");
+    shouldBe("text1.x.baseVal.getItem(3).value", "4");
+    shouldBeEqualToString("text1.getAttribute('x')", "1 2 3 4");
 
     shouldBe("text1.x.baseVal.replaceItem(text1.x.baseVal.getItem(0), text1)", "text1.x.baseVal.getItem(0)");
-    shouldBe("text1.x.baseVal.numberOfItems", "1");
-    shouldBeEqualToString("text1.getAttribute('x')", "1");
+    shouldBe("text1.x.baseVal.numberOfItems", "4");
+    shouldBeEqualToString("text1.getAttribute('x')", "1 2 3 4");
 
-    shouldThrow("text1.x.baseVal.replaceItem(text1.x.baseVal.getItem(0), null)");
-    shouldBe("text1.x.baseVal.numberOfItems", "0");
-    shouldBeEqualToString("text1.getAttribute('x')", "");
+    debug("");
+    debug("Set x='1 2 3 4' for text1");
+    shouldBeUndefined("text1.setAttribute('x', '1 2 3 4')");
+
+    debug("");
+    debug("Test edge cases for replaceItem()");
+    shouldBe("text1.x.baseVal.replaceItem(text1.x.baseVal.getItem(3), 3)", "text1.x.baseVal.getItem(3)");
+    shouldBe("text1.x.baseVal.numberOfItems", "4");
+    shouldBeEqualToString("text1.getAttribute('x')", "1 2 3 4");
+    shouldBe("text1.x.baseVal.replaceItem(text1.x.baseVal.getItem(1), 3)", "text1.x.baseVal.getItem(2)");
+    shouldBe("text1.x.baseVal.numberOfItems", "3");
+    shouldBeEqualToString("text1.getAttribute('x')", "1 3 2");
+    shouldThrow("text1.x.baseVal.replaceItem(text1.x.baseVal.getItem(3), 4)");
 
     debug("");
     debug("Set x='1 2 3 4' for text1");
index 40ec389..4d090be 100644 (file)
@@ -73,22 +73,25 @@ PASS text1.rotate.baseVal.replaceItem('aString', 0) threw exception TypeError: T
 PASS text1.rotate.baseVal.replaceItem(text1, 0) threw exception TypeError: Type error.
 PASS text1.rotate.baseVal.replaceItem(null, 0) threw exception Error: SVG_WRONG_TYPE_ERR: DOM SVG Exception 0.
 PASS text1.rotate.baseVal.replaceItem(text1.rotate.baseVal.getItem(0), 0) is text1.rotate.baseVal.getItem(0)
-PASS text1.rotate.baseVal.numberOfItems is 3
+PASS text1.rotate.baseVal.numberOfItems is 4
 PASS text1.rotate.baseVal.getItem(0).value is 1
-PASS text1.rotate.baseVal.getItem(1).value is 3
-PASS text1.rotate.baseVal.getItem(2).value is 4
-PASS text1.getAttribute('rotate') is "1 3 4"
+PASS text1.rotate.baseVal.getItem(1).value is 2
+PASS text1.rotate.baseVal.getItem(2).value is 3
+PASS text1.rotate.baseVal.getItem(3).value is 4
+PASS text1.getAttribute('rotate') is "1 2 3 4"
 PASS text1.rotate.baseVal.replaceItem(text1.rotate.baseVal.getItem(0), 'aString') is text1.rotate.baseVal.getItem(0)
-PASS text1.rotate.baseVal.numberOfItems is 2
+PASS text1.rotate.baseVal.numberOfItems is 4
 PASS text1.rotate.baseVal.getItem(0).value is 1
-PASS text1.rotate.baseVal.getItem(1).value is 4
-PASS text1.getAttribute('rotate') is "1 4"
+PASS text1.rotate.baseVal.getItem(1).value is 2
+PASS text1.rotate.baseVal.getItem(2).value is 3
+PASS text1.rotate.baseVal.getItem(3).value is 4
+PASS text1.getAttribute('rotate') is "1 2 3 4"
 PASS text1.rotate.baseVal.replaceItem(text1.rotate.baseVal.getItem(0), text1) is text1.rotate.baseVal.getItem(0)
-PASS text1.rotate.baseVal.numberOfItems is 1
-PASS text1.getAttribute('rotate') is "1"
-PASS text1.rotate.baseVal.replaceItem(text1.rotate.baseVal.getItem(0), null) threw exception Error: IndexSizeError: DOM Exception 1.
-PASS text1.rotate.baseVal.numberOfItems is 0
-PASS text1.getAttribute('rotate') is ""
+PASS text1.rotate.baseVal.numberOfItems is 4
+PASS text1.getAttribute('rotate') is "1 2 3 4"
+PASS text1.rotate.baseVal.replaceItem(text1.rotate.baseVal.getItem(0), null) is text1.rotate.baseVal.getItem(0)
+PASS text1.rotate.baseVal.numberOfItems is 4
+PASS text1.getAttribute('rotate') is "1 2 3 4"
 
 Set rotate='1 2 3 4' for text1
 PASS text1.setAttribute('rotate', '1 2 3 4') is undefined.
index d4d8eed..9c0d51b 100644 (file)
@@ -14,6 +14,9 @@
 <![CDATA[
     description("This is a test of the simple SVGNumberList API parts.");
 
+    if (window.testRunner)
+        testRunner.dumpAsText();
+
     var text1 = document.getElementById("text1");
  
     // Spec: The object referenced by animVal will always be distinct from the one referenced by baseVal, even when the attribute is not animated.
     shouldThrow("text1.rotate.baseVal.replaceItem(null, 0)");
 
     shouldBe("text1.rotate.baseVal.replaceItem(text1.rotate.baseVal.getItem(0), 0)", "text1.rotate.baseVal.getItem(0)");
-    shouldBe("text1.rotate.baseVal.numberOfItems", "3");
+    shouldBe("text1.rotate.baseVal.numberOfItems", "4");
     shouldBe("text1.rotate.baseVal.getItem(0).value", "1");
-    shouldBe("text1.rotate.baseVal.getItem(1).value", "3");
-    shouldBe("text1.rotate.baseVal.getItem(2).value", "4");
-    shouldBeEqualToString("text1.getAttribute('rotate')", "1 3 4");
+    shouldBe("text1.rotate.baseVal.getItem(1).value", "2");
+    shouldBe("text1.rotate.baseVal.getItem(2).value", "3");
+    shouldBe("text1.rotate.baseVal.getItem(3).value", "4");
+    shouldBeEqualToString("text1.getAttribute('rotate')", "1 2 3 4");
 
     shouldBe("text1.rotate.baseVal.replaceItem(text1.rotate.baseVal.getItem(0), 'aString')", "text1.rotate.baseVal.getItem(0)");
-    shouldBe("text1.rotate.baseVal.numberOfItems", "2");
+    shouldBe("text1.rotate.baseVal.numberOfItems", "4");
     shouldBe("text1.rotate.baseVal.getItem(0).value", "1");
-    shouldBe("text1.rotate.baseVal.getItem(1).value", "4");
-    shouldBeEqualToString("text1.getAttribute('rotate')", "1 4");
+    shouldBe("text1.rotate.baseVal.getItem(1).value", "2");
+    shouldBe("text1.rotate.baseVal.getItem(2).value", "3");
+    shouldBe("text1.rotate.baseVal.getItem(3).value", "4");
+    shouldBeEqualToString("text1.getAttribute('rotate')", "1 2 3 4");
 
     shouldBe("text1.rotate.baseVal.replaceItem(text1.rotate.baseVal.getItem(0), text1)", "text1.rotate.baseVal.getItem(0)");
-    shouldBe("text1.rotate.baseVal.numberOfItems", "1");
-    shouldBeEqualToString("text1.getAttribute('rotate')", "1");
+    shouldBe("text1.rotate.baseVal.numberOfItems", "4");
+    shouldBeEqualToString("text1.getAttribute('rotate')", "1 2 3 4");
 
-    shouldThrow("text1.rotate.baseVal.replaceItem(text1.rotate.baseVal.getItem(0), null)");
-    shouldBe("text1.rotate.baseVal.numberOfItems", "0");
-    shouldBeEqualToString("text1.getAttribute('rotate')", "");
+    shouldBe("text1.rotate.baseVal.replaceItem(text1.rotate.baseVal.getItem(0), null)", "text1.rotate.baseVal.getItem(0)");
+    shouldBe("text1.rotate.baseVal.numberOfItems", "4");
+    shouldBeEqualToString("text1.getAttribute('rotate')", "1 2 3 4");
 
     debug("");
     debug("Set rotate='1 2 3 4' for text1");
index 6b0f347..584a841 100644 (file)
@@ -93,23 +93,29 @@ PASS dumpPoint(poly1.points.getItem(2)) is "x=100 y=100"
 PASS dumpPoint(poly1.points.getItem(3)) is "x=0 y=100"
 PASS poly1.getAttribute('points').formatPointsAttribute() is "0 0 100 0 100 100 0 100"
 PASS dumpPoint(poly1.points.replaceItem(poly1.points.getItem(0), 0)) is "x=0 y=0"
-PASS poly1.points.numberOfItems is 3
+PASS poly1.points.numberOfItems is 4
 PASS dumpPoint(poly1.points.getItem(0)) is "x=0 y=0"
-PASS dumpPoint(poly1.points.getItem(1)) is "x=100 y=100"
-PASS dumpPoint(poly1.points.getItem(2)) is "x=0 y=100"
-PASS poly1.getAttribute('points').formatPointsAttribute() is "0 0 100 100 0 100"
+PASS dumpPoint(poly1.points.getItem(1)) is "x=100 y=0"
+PASS dumpPoint(poly1.points.getItem(2)) is "x=100 y=100"
+PASS dumpPoint(poly1.points.getItem(3)) is "x=0 y=100"
+PASS poly1.getAttribute('points').formatPointsAttribute() is "0 0 100 0 100 100 0 100"
 PASS dumpPoint(poly1.points.replaceItem(poly1.points.getItem(0), 'aString')) is "x=0 y=0"
-PASS poly1.points.numberOfItems is 2
+PASS poly1.points.numberOfItems is 4
 PASS dumpPoint(poly1.points.getItem(0)) is "x=0 y=0"
-PASS dumpPoint(poly1.points.getItem(1)) is "x=0 y=100"
-PASS poly1.getAttribute('points').formatPointsAttribute() is "0 0 0 100"
+PASS dumpPoint(poly1.points.getItem(1)) is "x=100 y=0"
+PASS dumpPoint(poly1.points.getItem(2)) is "x=100 y=100"
+PASS dumpPoint(poly1.points.getItem(3)) is "x=0 y=100"
+PASS poly1.getAttribute('points').formatPointsAttribute() is "0 0 100 0 100 100 0 100"
 PASS dumpPoint(poly1.points.replaceItem(poly1.points.getItem(0), poly1)) is "x=0 y=0"
-PASS poly1.points.numberOfItems is 1
+PASS poly1.points.numberOfItems is 4
 PASS dumpPoint(poly1.points.getItem(0)) is "x=0 y=0"
-PASS poly1.getAttribute('points').formatPointsAttribute() is "0 0"
-PASS poly1.points.replaceItem(poly1.points.getItem(0), null) threw exception Error: IndexSizeError: DOM Exception 1.
-PASS poly1.points.numberOfItems is 0
-PASS poly1.getAttribute('points') is ""
+PASS dumpPoint(poly1.points.getItem(1)) is "x=100 y=0"
+PASS dumpPoint(poly1.points.getItem(2)) is "x=100 y=100"
+PASS dumpPoint(poly1.points.getItem(3)) is "x=0 y=100"
+PASS poly1.getAttribute('points').formatPointsAttribute() is "0 0 100 0 100 100 0 100"
+PASS dumpPoint(poly1.points.replaceItem(poly1.points.getItem(0), null)) is "x=0 y=0"
+PASS poly1.points.numberOfItems is 4
+PASS poly1.getAttribute('points') is "0 0 100 0 100 100 0 100"
 
 Reset points attribute to 0 0 100 0 100 100 0 100
 PASS poly1.setAttribute('points', '0 0 100 0 100 100 0 100') is undefined.
index d078e64..b13a215 100644 (file)
@@ -14,6 +14,9 @@
 <![CDATA[
     description("This is a test of the simple SVGPointList API parts.");
 
+    if (window.testRunner)
+        testRunner.dumpAsText();
+
     // Extend String prototype, to offer a function, that formats the points attribute in the same way across browsers
     String.prototype.formatPointsAttribute = function() {
         return this.replace(/,/g, " "); // Remove Firefox commas
     shouldBeEqualToString("poly1.getAttribute('points').formatPointsAttribute()", "0 0 100 0 100 100 0 100");
 
     shouldBeEqualToString("dumpPoint(poly1.points.replaceItem(poly1.points.getItem(0), 0))", "x=0 y=0");
-    shouldBe("poly1.points.numberOfItems", "3");
+    shouldBe("poly1.points.numberOfItems", "4");
     shouldBeEqualToString("dumpPoint(poly1.points.getItem(0))", "x=0 y=0");
-    shouldBeEqualToString("dumpPoint(poly1.points.getItem(1))", "x=100 y=100");
-    shouldBeEqualToString("dumpPoint(poly1.points.getItem(2))", "x=0 y=100");
-    shouldBeEqualToString("poly1.getAttribute('points').formatPointsAttribute()", "0 0 100 100 0 100");
+    shouldBeEqualToString("dumpPoint(poly1.points.getItem(1))", "x=100 y=0");
+    shouldBeEqualToString("dumpPoint(poly1.points.getItem(2))", "x=100 y=100");
+    shouldBeEqualToString("dumpPoint(poly1.points.getItem(3))", "x=0 y=100");
+    shouldBeEqualToString("poly1.getAttribute('points').formatPointsAttribute()", "0 0 100 0 100 100 0 100");
 
     shouldBeEqualToString("dumpPoint(poly1.points.replaceItem(poly1.points.getItem(0), 'aString'))", "x=0 y=0");
-    shouldBe("poly1.points.numberOfItems", "2");
+    shouldBe("poly1.points.numberOfItems", "4");
     shouldBeEqualToString("dumpPoint(poly1.points.getItem(0))", "x=0 y=0");
-    shouldBeEqualToString("dumpPoint(poly1.points.getItem(1))", "x=0 y=100");
-    shouldBeEqualToString("poly1.getAttribute('points').formatPointsAttribute()", "0 0 0 100");
+    shouldBeEqualToString("dumpPoint(poly1.points.getItem(1))", "x=100 y=0");
+    shouldBeEqualToString("dumpPoint(poly1.points.getItem(2))", "x=100 y=100");
+    shouldBeEqualToString("dumpPoint(poly1.points.getItem(3))", "x=0 y=100");
+    shouldBeEqualToString("poly1.getAttribute('points').formatPointsAttribute()", "0 0 100 0 100 100 0 100");
 
     shouldBeEqualToString("dumpPoint(poly1.points.replaceItem(poly1.points.getItem(0), poly1))", "x=0 y=0");
-    shouldBe("poly1.points.numberOfItems", "1");
+    shouldBe("poly1.points.numberOfItems", "4");
     shouldBeEqualToString("dumpPoint(poly1.points.getItem(0))", "x=0 y=0");
-    shouldBeEqualToString("poly1.getAttribute('points').formatPointsAttribute()", "0 0");
+    shouldBeEqualToString("dumpPoint(poly1.points.getItem(1))", "x=100 y=0");
+    shouldBeEqualToString("dumpPoint(poly1.points.getItem(2))", "x=100 y=100");
+    shouldBeEqualToString("dumpPoint(poly1.points.getItem(3))", "x=0 y=100");
+    shouldBeEqualToString("poly1.getAttribute('points').formatPointsAttribute()", "0 0 100 0 100 100 0 100");
 
-    shouldThrow("poly1.points.replaceItem(poly1.points.getItem(0), null)");
-    shouldBe("poly1.points.numberOfItems", "0");
-    shouldBeEqualToString("poly1.getAttribute('points')", "");
+    shouldBeEqualToString("dumpPoint(poly1.points.replaceItem(poly1.points.getItem(0), null))", "x=0 y=0");
+    shouldBe("poly1.points.numberOfItems", "4");
+    shouldBeEqualToString("poly1.getAttribute('points')", "0 0 100 0 100 100 0 100");
 
     debug("");
     debug("Reset points attribute to 0 0 100 0 100 100 0 100");
index 5e7543d..2041163 100644 (file)
@@ -70,23 +70,29 @@ PASS circle1.transform.baseVal.replaceItem('aString', 0) threw exception TypeErr
 PASS circle1.transform.baseVal.replaceItem(circle1, 0) threw exception TypeError: Type error.
 PASS circle1.transform.baseVal.replaceItem(null, 0) threw exception Error: SVG_WRONG_TYPE_ERR: DOM SVG Exception 0.
 PASS circle1.transform.baseVal.replaceItem(circle1.transform.baseVal.getItem(0), 0) is circle1.transform.baseVal.getItem(0)
-PASS circle1.transform.baseVal.numberOfItems is 3
+PASS circle1.transform.baseVal.numberOfItems is 4
 PASS dumpTransform(circle1.transform.baseVal.getItem(0)) is "type=SVG_TRANSFORM_ROTATE matrix=[0.0 1.0 -1.0 0.0 0.0 0.0]"
-PASS dumpTransform(circle1.transform.baseVal.getItem(1)) is "type=SVG_TRANSFORM_TRANSLATE matrix=[1.0 0.0 0.0 1.0 10.0 10.0]"
-PASS dumpTransform(circle1.transform.baseVal.getItem(2)) is "type=SVG_TRANSFORM_SKEWX matrix=[1.0 0.0 1.0 1.0 0.0 0.0]"
-PASS circle1.getAttribute('transform') is "rotate(90) translate(10 10) skewX(45)"
+PASS dumpTransform(circle1.transform.baseVal.getItem(1)) is "type=SVG_TRANSFORM_SCALE matrix=[2.0 0.0 0.0 2.0 0.0 0.0]"
+PASS dumpTransform(circle1.transform.baseVal.getItem(2)) is "type=SVG_TRANSFORM_TRANSLATE matrix=[1.0 0.0 0.0 1.0 10.0 10.0]"
+PASS dumpTransform(circle1.transform.baseVal.getItem(3)) is "type=SVG_TRANSFORM_SKEWX matrix=[1.0 0.0 1.0 1.0 0.0 0.0]"
+PASS circle1.getAttribute('transform') is "rotate(90) scale(2 2) translate(10 10) skewX(45)"
 PASS circle1.transform.baseVal.replaceItem(circle1.transform.baseVal.getItem(0), 'aString') is circle1.transform.baseVal.getItem(0)
-PASS circle1.transform.baseVal.numberOfItems is 2
+PASS circle1.transform.baseVal.numberOfItems is 4
 PASS dumpTransform(circle1.transform.baseVal.getItem(0)) is "type=SVG_TRANSFORM_ROTATE matrix=[0.0 1.0 -1.0 0.0 0.0 0.0]"
-PASS dumpTransform(circle1.transform.baseVal.getItem(1)) is "type=SVG_TRANSFORM_SKEWX matrix=[1.0 0.0 1.0 1.0 0.0 0.0]"
-PASS circle1.getAttribute('transform') is "rotate(90) skewX(45)"
+PASS dumpTransform(circle1.transform.baseVal.getItem(1)) is "type=SVG_TRANSFORM_SCALE matrix=[2.0 0.0 0.0 2.0 0.0 0.0]"
+PASS dumpTransform(circle1.transform.baseVal.getItem(2)) is "type=SVG_TRANSFORM_TRANSLATE matrix=[1.0 0.0 0.0 1.0 10.0 10.0]"
+PASS dumpTransform(circle1.transform.baseVal.getItem(3)) is "type=SVG_TRANSFORM_SKEWX matrix=[1.0 0.0 1.0 1.0 0.0 0.0]"
+PASS circle1.getAttribute('transform') is "rotate(90) scale(2 2) translate(10 10) skewX(45)"
 PASS circle1.transform.baseVal.replaceItem(circle1.transform.baseVal.getItem(0), circle1) is circle1.transform.baseVal.getItem(0)
-PASS circle1.transform.baseVal.numberOfItems is 1
+PASS circle1.transform.baseVal.numberOfItems is 4
 PASS dumpTransform(circle1.transform.baseVal.getItem(0)) is "type=SVG_TRANSFORM_ROTATE matrix=[0.0 1.0 -1.0 0.0 0.0 0.0]"
-PASS circle1.getAttribute('transform') is "rotate(90)"
-PASS circle1.transform.baseVal.replaceItem(circle1.transform.baseVal.getItem(0), null) threw exception Error: IndexSizeError: DOM Exception 1.
-PASS circle1.transform.baseVal.numberOfItems is 0
-PASS circle1.getAttribute('transform') is ""
+PASS dumpTransform(circle1.transform.baseVal.getItem(1)) is "type=SVG_TRANSFORM_SCALE matrix=[2.0 0.0 0.0 2.0 0.0 0.0]"
+PASS dumpTransform(circle1.transform.baseVal.getItem(2)) is "type=SVG_TRANSFORM_TRANSLATE matrix=[1.0 0.0 0.0 1.0 10.0 10.0]"
+PASS dumpTransform(circle1.transform.baseVal.getItem(3)) is "type=SVG_TRANSFORM_SKEWX matrix=[1.0 0.0 1.0 1.0 0.0 0.0]"
+PASS circle1.getAttribute('transform') is "rotate(90) scale(2 2) translate(10 10) skewX(45)"
+PASS circle1.transform.baseVal.replaceItem(circle1.transform.baseVal.getItem(0), null) is circle1.transform.baseVal.getItem(0)
+PASS circle1.transform.baseVal.numberOfItems is 4
+PASS circle1.getAttribute('transform') is "rotate(90) scale(2 2) translate(10 10) skewX(45)"
 
 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.
index e4f6cf3..66bd1c8 100644 (file)
@@ -14,6 +14,9 @@
 <![CDATA[
     description("This is a test of the simple SVGTransformList API parts.");
 
+    if (window.testRunner)
+        testRunner.dumpAsText();
+
     function dumpMatrix(matrix) {
         return "[" + matrix.a.toFixed(1)
              + " " + matrix.b.toFixed(1)
     shouldThrow("circle1.transform.baseVal.replaceItem(null, 0)");
 
     shouldBe("circle1.transform.baseVal.replaceItem(circle1.transform.baseVal.getItem(0), 0)", "circle1.transform.baseVal.getItem(0)");
-    shouldBe("circle1.transform.baseVal.numberOfItems", "3");
+    shouldBe("circle1.transform.baseVal.numberOfItems", "4");
     shouldBeEqualToString("dumpTransform(circle1.transform.baseVal.getItem(0))", "type=SVG_TRANSFORM_ROTATE matrix=[0.0 1.0 -1.0 0.0 0.0 0.0]");
-    shouldBeEqualToString("dumpTransform(circle1.transform.baseVal.getItem(1))", "type=SVG_TRANSFORM_TRANSLATE matrix=[1.0 0.0 0.0 1.0 10.0 10.0]");
-    shouldBeEqualToString("dumpTransform(circle1.transform.baseVal.getItem(2))", "type=SVG_TRANSFORM_SKEWX matrix=[1.0 0.0 1.0 1.0 0.0 0.0]");
-    shouldBeEqualToString("circle1.getAttribute('transform')", "rotate(90) translate(10 10) skewX(45)");
+    shouldBeEqualToString("dumpTransform(circle1.transform.baseVal.getItem(1))", "type=SVG_TRANSFORM_SCALE matrix=[2.0 0.0 0.0 2.0 0.0 0.0]");
+    shouldBeEqualToString("dumpTransform(circle1.transform.baseVal.getItem(2))", "type=SVG_TRANSFORM_TRANSLATE matrix=[1.0 0.0 0.0 1.0 10.0 10.0]");
+    shouldBeEqualToString("dumpTransform(circle1.transform.baseVal.getItem(3))", "type=SVG_TRANSFORM_SKEWX matrix=[1.0 0.0 1.0 1.0 0.0 0.0]");
+    shouldBeEqualToString("circle1.getAttribute('transform')", "rotate(90) scale(2 2) translate(10 10) skewX(45)");
 
     shouldBe("circle1.transform.baseVal.replaceItem(circle1.transform.baseVal.getItem(0), 'aString')", "circle1.transform.baseVal.getItem(0)");
-    shouldBe("circle1.transform.baseVal.numberOfItems", "2");
+    shouldBe("circle1.transform.baseVal.numberOfItems", "4");
     shouldBeEqualToString("dumpTransform(circle1.transform.baseVal.getItem(0))", "type=SVG_TRANSFORM_ROTATE matrix=[0.0 1.0 -1.0 0.0 0.0 0.0]");
-    shouldBeEqualToString("dumpTransform(circle1.transform.baseVal.getItem(1))", "type=SVG_TRANSFORM_SKEWX matrix=[1.0 0.0 1.0 1.0 0.0 0.0]");
-    shouldBeEqualToString("circle1.getAttribute('transform')", "rotate(90) skewX(45)");
+    shouldBeEqualToString("dumpTransform(circle1.transform.baseVal.getItem(1))", "type=SVG_TRANSFORM_SCALE matrix=[2.0 0.0 0.0 2.0 0.0 0.0]");
+    shouldBeEqualToString("dumpTransform(circle1.transform.baseVal.getItem(2))", "type=SVG_TRANSFORM_TRANSLATE matrix=[1.0 0.0 0.0 1.0 10.0 10.0]");
+    shouldBeEqualToString("dumpTransform(circle1.transform.baseVal.getItem(3))", "type=SVG_TRANSFORM_SKEWX matrix=[1.0 0.0 1.0 1.0 0.0 0.0]");
+    shouldBeEqualToString("circle1.getAttribute('transform')", "rotate(90) scale(2 2) translate(10 10) skewX(45)");
 
     shouldBe("circle1.transform.baseVal.replaceItem(circle1.transform.baseVal.getItem(0), circle1)", "circle1.transform.baseVal.getItem(0)");
-    shouldBe("circle1.transform.baseVal.numberOfItems", "1");
+    shouldBe("circle1.transform.baseVal.numberOfItems", "4");
     shouldBeEqualToString("dumpTransform(circle1.transform.baseVal.getItem(0))", "type=SVG_TRANSFORM_ROTATE matrix=[0.0 1.0 -1.0 0.0 0.0 0.0]");
-    shouldBeEqualToString("circle1.getAttribute('transform')", "rotate(90)");
+    shouldBeEqualToString("dumpTransform(circle1.transform.baseVal.getItem(1))", "type=SVG_TRANSFORM_SCALE matrix=[2.0 0.0 0.0 2.0 0.0 0.0]");
+    shouldBeEqualToString("dumpTransform(circle1.transform.baseVal.getItem(2))", "type=SVG_TRANSFORM_TRANSLATE matrix=[1.0 0.0 0.0 1.0 10.0 10.0]");
+    shouldBeEqualToString("dumpTransform(circle1.transform.baseVal.getItem(3))", "type=SVG_TRANSFORM_SKEWX matrix=[1.0 0.0 1.0 1.0 0.0 0.0]");
+    shouldBeEqualToString("circle1.getAttribute('transform')", "rotate(90) scale(2 2) translate(10 10) skewX(45)");
 
-    shouldThrow("circle1.transform.baseVal.replaceItem(circle1.transform.baseVal.getItem(0), null)");
-    shouldBe("circle1.transform.baseVal.numberOfItems", "0");
-    shouldBeEqualToString("circle1.getAttribute('transform')", "");
+    shouldBe("circle1.transform.baseVal.replaceItem(circle1.transform.baseVal.getItem(0), null)", "circle1.transform.baseVal.getItem(0)");
+    shouldBe("circle1.transform.baseVal.numberOfItems", "4");
+    shouldBeEqualToString("circle1.getAttribute('transform')", "rotate(90) scale(2 2) translate(10 10) skewX(45)");
 
     debug("");
     debug("Set transform='rotate(90) scale(2 2) translate(10 10) skewX(45)' for circle1");
index 388eadf..437a0d1 100644 (file)
@@ -1,3 +1,58 @@
+2013-02-13  Florin Malita  <fmalita@chromium.org>
+
+        [SVG] OOB access in SVGListProperty::replaceItemValues()
+        https://bugs.webkit.org/show_bug.cgi?id=109293
+
+        Replacing a list property item with itself should be a no-op. This patch updates the related
+        APIs and logic to detect the self-replace case and prevent removal of the item from the list.
+
+        To avoid scanning the list multiple times, removeItemFromList() is updated to operate on
+        indices and a findItem() method is added to resolve an item to an index.
+
+        Reviewed by Dirk Schulze.
+
+        No new tests: updated existing tests cover the change.
+
+        * svg/properties/SVGAnimatedListPropertyTearOff.h:
+        (WebCore::SVGAnimatedListPropertyTearOff::findItem):
+        (SVGAnimatedListPropertyTearOff):
+        (WebCore::SVGAnimatedListPropertyTearOff::removeItemFromList):
+        * svg/properties/SVGAnimatedPathSegListPropertyTearOff.h:
+        (WebCore::SVGAnimatedPathSegListPropertyTearOff::findItem):
+        (SVGAnimatedPathSegListPropertyTearOff):
+        (WebCore::SVGAnimatedPathSegListPropertyTearOff::removeItemFromList):
+        Add a findItem() delegating method, and update removeItemFromList() to use the new
+        index-based API.
+
+        * svg/properties/SVGListProperty.h:
+        (WebCore::SVGListProperty::insertItemBeforeValues):
+        (WebCore::SVGListProperty::insertItemBeforeValuesAndWrappers):
+        (WebCore::SVGListProperty::replaceItemValues):
+        (WebCore::SVGListProperty::replaceItemValuesAndWrappers):
+        (SVGListProperty):
+        Updated to handle the no-op case for insertItemBefore() & replaceItem().
+
+        * svg/properties/SVGListPropertyTearOff.h:
+        (WebCore::SVGListPropertyTearOff::findItem):
+        (WebCore::SVGListPropertyTearOff::removeItemFromList):
+        Index-based API updates.
+
+        (WebCore::SVGListPropertyTearOff::processIncomingListItemValue):
+        (WebCore::SVGListPropertyTearOff::processIncomingListItemWrapper):
+        * svg/properties/SVGPathSegListPropertyTearOff.cpp:
+        (WebCore::SVGPathSegListPropertyTearOff::processIncomingListItemValue):
+        Detect the self-replace case and return without removing the item from the list.
+
+        * svg/properties/SVGPathSegListPropertyTearOff.h:
+        (WebCore::SVGPathSegListPropertyTearOff::findItem):
+        (WebCore::SVGPathSegListPropertyTearOff::removeItemFromList):
+        (SVGPathSegListPropertyTearOff):
+        (WebCore::SVGPathSegListPropertyTearOff::processIncomingListItemWrapper):
+        * svg/properties/SVGStaticListPropertyTearOff.h:
+        (WebCore::SVGStaticListPropertyTearOff::processIncomingListItemValue):
+        (WebCore::SVGStaticListPropertyTearOff::processIncomingListItemWrapper):
+        Index-based API updates.
+
 2013-02-13  Takashi Sakamoto  <tasak@google.com>
 
         [Refactoring] StyleResolver::State should have methods to access its member variables.
index 6be9b1c..65ee9e5 100644 (file)
@@ -56,12 +56,19 @@ public:
 
     virtual bool isAnimatedListTearOff() const { return true; }
 
-    int removeItemFromList(SVGProperty* property, bool shouldSynchronizeWrappers)
+    int findItem(SVGProperty* property) const
     {
         // This should ever be called for our baseVal, as animVal can't modify the list.
         // It's safe to cast to ListPropertyTearOff here as all classes inheriting from us supply their own removeItemFromList() method.
         typedef SVGPropertyTearOff<typename SVGPropertyTraits<PropertyType>::ListItemType> ListItemTearOff;
-        return static_cast<ListPropertyTearOff*>(m_baseVal.get())->removeItemFromList(static_cast<ListItemTearOff*>(property), shouldSynchronizeWrappers);
+        return static_cast<ListPropertyTearOff*>(m_baseVal.get())->findItem(static_cast<ListItemTearOff*>(property));
+    }
+
+    void removeItemFromList(size_t itemIndex, bool shouldSynchronizeWrappers)
+    {
+        // This should ever be called for our baseVal, as animVal can't modify the list.
+        // It's safe to cast to ListPropertyTearOff here as all classes inheriting from us supply their own removeItemFromList() method.
+        static_cast<ListPropertyTearOff*>(m_baseVal.get())->removeItemFromList(itemIndex, shouldSynchronizeWrappers);
     }
 
     void detachListWrappers(unsigned newListSize)
index db910fe..f1268f8 100644 (file)
@@ -46,11 +46,18 @@ public:
         return static_cast<SVGListProperty<SVGPathSegList>*>(m_animVal.get());
     }
 
-    int removeItemFromList(const RefPtr<SVGPathSeg>& segment, bool shouldSynchronizeWrappers)
+    int findItem(const RefPtr<SVGPathSeg>& segment) const
     {
         // This should ever be called for our baseVal, as animVal can't modify the list.
         ASSERT(m_baseVal);
-        return static_cast<SVGPathSegListPropertyTearOff*>(m_baseVal.get())->removeItemFromList(segment, shouldSynchronizeWrappers);
+        return static_cast<SVGPathSegListPropertyTearOff*>(m_baseVal.get())->findItem(segment);
+    }
+
+    void removeItemFromList(size_t itemIndex, bool shouldSynchronizeWrappers)
+    {
+        // This should ever be called for our baseVal, as animVal can't modify the list.
+        ASSERT(m_baseVal);
+        static_cast<SVGPathSegListPropertyTearOff*>(m_baseVal.get())->removeItemFromList(itemIndex, shouldSynchronizeWrappers);
     }
 
     static PassRefPtr<SVGAnimatedPathSegListPropertyTearOff> create(SVGElement* contextElement, const QualifiedName& attributeName, AnimatedPropertyType animatedPropertyType, SVGPathSegList& values)
index 9db2f4c..5a202df 100644 (file)
@@ -221,7 +221,10 @@ public:
             index = m_values->size();
 
         // Spec: If newItem is already in a list, it is removed from its previous list before it is inserted into this list.
-        processIncomingListItemValue(newItem, &index);
+        if (!processIncomingListItemValue(newItem, &index)) {
+            // Inserting the item before itself is a no-op.
+            return newItem;
+        }
 
         // Spec: Inserts a new item into the list at the specified position. The index of the item before which the new item is to be
         // inserted. The first item is number 0. If the index is equal to 0, then the new item is inserted at the front of the list.
@@ -251,7 +254,8 @@ public:
         ASSERT(m_values->size() == m_wrappers->size());
 
         // Spec: If newItem is already in a list, it is removed from its previous list before it is inserted into this list.
-        processIncomingListItemWrapper(newItem, &index);
+        if (!processIncomingListItemWrapper(newItem, &index))
+            return newItem.release();
 
         // Spec: Inserts a new item into the list at the specified position. The index of the item before which the new item is to be
         // inserted. The first item is number 0. If the index is equal to 0, then the new item is inserted at the front of the list.
@@ -285,7 +289,10 @@ public:
 
         // Spec: If newItem is already in a list, it is removed from its previous list before it is inserted into this list.
         // Spec: If the item is already in this list, note that the index of the item to replace is before the removal of the item.
-        processIncomingListItemValue(newItem, &index);
+        if (!processIncomingListItemValue(newItem, &index)) {
+            // Replacing the item with itself is a no-op.
+            return newItem;
+        }
 
         if (m_values->isEmpty()) {
             // 'newItem' already lived in our list, we removed it, and now we're empty, which means there's nothing to replace.
@@ -317,7 +324,8 @@ public:
 
         // Spec: If newItem is already in a list, it is removed from its previous list before it is inserted into this list.
         // Spec: If the item is already in this list, note that the index of the item to replace is before the removal of the item.
-        processIncomingListItemWrapper(newItem, &index);
+        if (!processIncomingListItemWrapper(newItem, &index))
+            return newItem.release();
 
         if (m_values->isEmpty()) {
             ASSERT(m_wrappers->isEmpty());
@@ -461,8 +469,8 @@ protected:
         commitChange();
     }
 
-    virtual void processIncomingListItemValue(const ListItemType& newItem, unsigned* indexToModify) = 0;
-    virtual void processIncomingListItemWrapper(RefPtr<ListItemTearOff>& newItem, unsigned* indexToModify) = 0;
+    virtual bool processIncomingListItemValue(const ListItemType& newItem, unsigned* indexToModify) = 0;
+    virtual bool processIncomingListItemWrapper(RefPtr<ListItemTearOff>& newItem, unsigned* indexToModify) = 0;
 
     SVGPropertyRole m_role;
     bool m_ownsValues;
index c3bba4b..3c65524 100644 (file)
@@ -47,30 +47,35 @@ public:
         return adoptRef(new Self(animatedProperty, role, values, wrappers));
     }
 
-    int removeItemFromList(ListItemTearOff* removeItem, bool shouldSynchronizeWrappers)
+    int findItem(ListItemTearOff* item) const
     {
         ASSERT(m_values);
         ASSERT(m_wrappers);
 
-        // Lookup item in cache and remove its corresponding wrapper.
         unsigned size = m_wrappers->size();
         ASSERT(size == m_values->size());
-        for (unsigned i = 0; i < size; ++i) {
-            RefPtr<ListItemTearOff>& item = m_wrappers->at(i);
-            if (item != removeItem)
-                continue;
+        for (size_t i = 0; i < size; ++i) {
+            if (item == m_wrappers->at(i))
+                return i;
+        }
 
-            item->detachWrapper();
-            m_wrappers->remove(i);
-            m_values->remove(i);
+        return -1;
+    }
 
-            if (shouldSynchronizeWrappers)
-                commitChange();
+    void removeItemFromList(size_t itemIndex, bool shouldSynchronizeWrappers)
+    {
+        ASSERT(m_values);
+        ASSERT(m_wrappers);
+        ASSERT(m_values->size() == m_wrappers->size());
+        ASSERT_WITH_SECURITY_IMPLICATION(itemIndex < m_wrappers->size());
 
-            return i;
-        }
+        RefPtr<ListItemTearOff>& item = m_wrappers->at(itemIndex);
+        item->detachWrapper();
+        m_wrappers->remove(itemIndex);
+        m_values->remove(itemIndex);
 
-        return -1;
+        if (shouldSynchronizeWrappers)
+            commitChange();
     }
 
     // SVGList API
@@ -144,19 +149,20 @@ protected:
         m_animatedProperty->commitChange();
     }
 
-    virtual void processIncomingListItemValue(const ListItemType&, unsigned*)
+    virtual bool processIncomingListItemValue(const ListItemType&, unsigned*)
     {
         ASSERT_NOT_REACHED();
+        return true;
     }
 
-    virtual void processIncomingListItemWrapper(RefPtr<ListItemTearOff>& newItem, unsigned* indexToModify)
+    virtual bool processIncomingListItemWrapper(RefPtr<ListItemTearOff>& newItem, unsigned* indexToModify)
     {
         SVGAnimatedProperty* animatedPropertyOfItem = newItem->animatedProperty();
 
         // newItem has been created manually, it doesn't belong to any SVGElement.
         // (for example: "textElement.x.baseVal.appendItem(svgsvgElement.createSVGLength())")
         if (!animatedPropertyOfItem)
-            return;
+            return true;
 
         // newItem belongs to a SVGElement, but its associated SVGAnimatedProperty is not an animated list tear off.
         // (for example: "textElement.x.baseVal.appendItem(rectElement.width.baseVal)")
@@ -167,25 +173,34 @@ protected:
             // that's inserted into SVGTextElements SVGAnimatedLengthList 'x'. textElement.x.baseVal.getItem(0).value += 150 would
             // mutate the rectElement width _and_ the textElement x list. That's obviously wrong, take care of that.
             newItem = ListItemTearOff::create(newItem->propertyReference());
-            return;
+            return true;
         }
 
         // Spec: If newItem is already in a list, it is removed from its previous list before it is inserted into this list.
         // 'newItem' is already living in another list. If it's not our list, synchronize the other lists wrappers after the removal.
         bool livesInOtherList = animatedPropertyOfItem != m_animatedProperty;
-        int removedIndex = static_cast<AnimatedListPropertyTearOff*>(animatedPropertyOfItem)->removeItemFromList(newItem.get(), livesInOtherList);
-        ASSERT(removedIndex != -1);
+        AnimatedListPropertyTearOff* propertyTearOff = static_cast<AnimatedListPropertyTearOff*>(animatedPropertyOfItem);
+        int indexToRemove = propertyTearOff->findItem(newItem.get());
+        ASSERT(indexToRemove != -1);
+
+        // Do not remove newItem if already in this list at the target index.
+        if (!livesInOtherList && indexToModify && static_cast<unsigned>(indexToRemove) == *indexToModify)
+            return false;
+
+        propertyTearOff->removeItemFromList(indexToRemove, livesInOtherList);
 
         if (!indexToModify)
-            return;
+            return true;
 
         // If the item lived in our list, adjust the insertion index.
         if (!livesInOtherList) {
             unsigned& index = *indexToModify;
             // Spec: If the item is already in this list, note that the index of the item to (replace|insert before) is before the removal of the item.
-            if (static_cast<unsigned>(removedIndex) < index)
+            if (static_cast<unsigned>(indexToRemove) < index)
                 --index;
         }
+
+        return true;
     }
 
     // Back pointer to the animated property that created us
index 2021301..ccef713 100644 (file)
@@ -70,7 +70,7 @@ SVGPathElement* SVGPathSegListPropertyTearOff::contextElement() const
     return static_cast<SVGPathElement*>(contextElement);
 }
 
-void SVGPathSegListPropertyTearOff::processIncomingListItemValue(const ListItemType& newItem, unsigned* indexToModify)
+bool SVGPathSegListPropertyTearOff::processIncomingListItemValue(const ListItemType& newItem, unsigned* indexToModify)
 {
     SVGPathSegWithContext* newItemWithContext = static_cast<SVGPathSegWithContext*>(newItem.get());
     SVGAnimatedProperty* animatedPropertyOfItem = newItemWithContext->animatedProperty();
@@ -79,29 +79,38 @@ void SVGPathSegListPropertyTearOff::processIncomingListItemValue(const ListItemT
     newItemWithContext->setContextAndRole(contextElement(), m_pathSegRole);
 
     if (!animatedPropertyOfItem)
-        return;
+        return true;
 
     // newItem belongs to a SVGPathElement, but its associated SVGAnimatedProperty is not an animated list tear off.
     // (for example: "pathElement.pathSegList.appendItem(pathElement.createSVGPathSegClosepath())")
     if (!animatedPropertyOfItem->isAnimatedListTearOff())
-        return;
+        return true;
 
     // Spec: If newItem is already in a list, it is removed from its previous list before it is inserted into this list.
     // 'newItem' is already living in another list. If it's not our list, synchronize the other lists wrappers after the removal.
     bool livesInOtherList = animatedPropertyOfItem != m_animatedProperty;
-    int removedIndex = static_cast<SVGAnimatedPathSegListPropertyTearOff*>(animatedPropertyOfItem)->removeItemFromList(newItem.get(), livesInOtherList);
-    ASSERT(removedIndex != -1);
+    SVGAnimatedPathSegListPropertyTearOff* propertyTearOff = static_cast<SVGAnimatedPathSegListPropertyTearOff*>(animatedPropertyOfItem);
+    int indexToRemove = propertyTearOff->findItem(newItem.get());
+    ASSERT(indexToRemove != -1);
+
+    // Do not remove newItem if already in this list at the target index.
+    if (!livesInOtherList && indexToModify && static_cast<unsigned>(indexToRemove) == *indexToModify)
+        return false;
+
+    propertyTearOff->removeItemFromList(indexToRemove, livesInOtherList);
 
     if (!indexToModify)
-        return;
+        return true;
 
     // If the item lived in our list, adjust the insertion index.
     if (!livesInOtherList) {
         unsigned& index = *indexToModify;
         // Spec: If the item is already in this list, note that the index of the item to (replace|insert before) is before the removal of the item.
-        if (static_cast<unsigned>(removedIndex) < index)
+        if (static_cast<unsigned>(indexToRemove) < index)
             --index;
     }
+
+    return true;
 }
 
 }
index bac9ee7..abde768 100644 (file)
@@ -41,24 +41,28 @@ public:
         return adoptRef(new SVGPathSegListPropertyTearOff(animatedProperty, role, pathSegRole, values, wrappers));
     }
 
-    int removeItemFromList(const ListItemType& removeItem, bool shouldSynchronizeWrappers)
+    int findItem(const ListItemType& item) const
     {
         ASSERT(m_values);
+
         unsigned size = m_values->size();
-        for (unsigned i = 0; i < size; ++i) {
-            ListItemType& item = m_values->at(i);
-            if (item != removeItem)
-                continue;
+        for (size_t i = 0; i < size; ++i) {
+            if (item == m_values->at(i))
+                return i;
+        }
 
-            m_values->remove(i);
+        return -1;
+    }
 
-            if (shouldSynchronizeWrappers)
-                commitChange();
+    void removeItemFromList(size_t itemIndex, bool shouldSynchronizeWrappers)
+    {
+        ASSERT(m_values);
+        ASSERT_WITH_SECURITY_IMPLICATION(itemIndex < m_values->size());
 
-            return i;
-        }
+        m_values->remove(itemIndex);
 
-        return -1;
+        if (shouldSynchronizeWrappers)
+            commitChange();
     }
 
     // SVGList API
@@ -149,10 +153,11 @@ private:
         m_values->commitChange(m_animatedProperty->contextElement(), listModification);
     }
 
-    virtual void processIncomingListItemValue(const ListItemType& newItem, unsigned* indexToModify);
-    virtual void processIncomingListItemWrapper(RefPtr<ListItemTearOff>&, unsigned*)
+    virtual bool processIncomingListItemValue(const ListItemType& newItem, unsigned* indexToModify) OVERRIDE;
+    virtual bool processIncomingListItemWrapper(RefPtr<ListItemTearOff>&, unsigned*)
     {
         ASSERT_NOT_REACHED();
+        return true;
     }
 
 private:
index b39aaf3..4e6f5fd 100644 (file)
@@ -96,14 +96,16 @@ private:
         m_values->commitChange(m_contextElement.get());
     }
 
-    virtual void processIncomingListItemValue(const ListItemType&, unsigned*)
+    virtual bool processIncomingListItemValue(const ListItemType&, unsigned*)
     {
         // no-op for static lists
+        return true;
     }
 
-    virtual void processIncomingListItemWrapper(RefPtr<ListItemTearOff>&, unsigned*)
+    virtual bool processIncomingListItemWrapper(RefPtr<ListItemTearOff>&, unsigned*)
     {
         ASSERT_NOT_REACHED();
+        return true;
     }
 
 private: