Bug 630760 - DOMSVGLengthList ctor synchronizes animVal instances to the baseVal internal list, plus sync some divergence between comments and code in list types. r=roc, a=blocking.

content/svg/content/src/DOMSVGAnimatedLengthList.cpp

@@ -67,7 +67,7 @@ NS_IMETHODIMP
 DOMSVGAnimatedLengthList::GetBaseVal(nsIDOMSVGLengthList **_retval)
 {
   if (!mBaseVal) {
-    mBaseVal = new DOMSVGLengthList(this);
+    mBaseVal = new DOMSVGLengthList(this, InternalAList().GetBaseValue());
   }
   NS_ADDREF(*_retval = mBaseVal);
   return NS_OK;
@@ -77,7 +77,7 @@ NS_IMETHODIMP
 DOMSVGAnimatedLengthList::GetAnimVal(nsIDOMSVGLengthList **_retval)
 {
   if (!mAnimVal) {
-    mAnimVal = new DOMSVGLengthList(this);
+    mAnimVal = new DOMSVGLengthList(this, InternalAList().GetAnimValue());
   }
   NS_ADDREF(*_retval = mAnimVal);
   return NS_OK;

content/svg/content/src/DOMSVGLengthList.cpp

@@ -85,7 +85,9 @@ DOMSVGLengthList::InternalListLengthWillChange(PRUint32 aNewLength)
     }
   }
 
-  if (!mItems.SetLength(aNewLength)) { // OOM
+  if (!mItems.SetLength(aNewLength)) {
+    // We silently ignore SetLength OOM failure since being out of sync is safe
+    // so long as we have *fewer* items than our internal list.
     mItems.Clear();
     return;
   }
@@ -204,26 +206,31 @@ DOMSVGLengthList::InsertItemBefore(nsIDOMSVGLength *newItem,
   if (!domItem) {
     return NS_ERROR_DOM_SVG_WRONG_TYPE_ERR;
   }
-  index = NS_MIN(index, Length());
-  SVGLength length = domItem->ToSVGLength(); // get before setting domItem
   if (domItem->HasOwner()) {
-    domItem = new DOMSVGLength();
+    domItem = domItem->Copy(); // must do this before changing anything!
   }
-  PRBool ok = !!InternalList().InsertItem(index, length);
-  if (!ok) {
+  index = NS_MIN(index, Length());
+
+  // Ensure we have enough memory so we can avoid complex error handling below:
+  if (!mItems.SetCapacity(mItems.Length() + 1) ||
+      !InternalList().SetCapacity(InternalList().Length() + 1)) {
     return NS_ERROR_OUT_OF_MEMORY;
   }
+
+  InternalList().InsertItem(index, domItem->ToSVGLength());
+  mItems.InsertElementAt(index, domItem.get());
+
+  // This MUST come after the insertion into InternalList(), or else under the
+  // insertion into InternalList() the values read from domItem would be bad
+  // data from InternalList() itself!:
   domItem->InsertingIntoList(this, AttrEnum(), index, IsAnimValList());
-  ok = !!mItems.InsertElementAt(index, domItem.get());
-  if (!ok) {
-    InternalList().RemoveItem(index);
-    return NS_ERROR_OUT_OF_MEMORY;
-  }
+
   for (PRUint32 i = index + 1; i < Length(); ++i) {
     if (mItems[i]) {
       mItems[i]->UpdateListIndex(i);
     }
   }
+
   Element()->DidChangeLengthList(AttrEnum(), PR_TRUE);
 #ifdef MOZ_SMIL
   if (mAList->IsAnimating()) {
@@ -251,19 +258,23 @@ DOMSVGLengthList::ReplaceItem(nsIDOMSVGLength *newItem,
   if (index >= Length()) {
     return NS_ERROR_DOM_INDEX_SIZE_ERR;
   }
-  SVGLength length = domItem->ToSVGLength(); // get before setting domItem
   if (domItem->HasOwner()) {
-    domItem = new DOMSVGLength();
+    domItem = domItem->Copy(); // must do this before changing anything!
   }
+
   if (mItems[index]) {
     // Notify any existing DOM item of removal *before* modifying the lists so
     // that the DOM item can copy the *old* value at its index:
     mItems[index]->RemovingFromList();
   }
-  InternalList()[index] = length;
-  domItem->InsertingIntoList(this, AttrEnum(), index, IsAnimValList());
+
+  InternalList()[index] = domItem->ToSVGLength();
   mItems[index] = domItem;
 
+  // This MUST come after the ToSVGPoint() call, otherwise that call
+  // would end up reading bad data from InternalList()!
+  domItem->InsertingIntoList(this, AttrEnum(), index, IsAnimValList());
+
   Element()->DidChangeLengthList(AttrEnum(), PR_TRUE);
 #ifdef MOZ_SMIL
   if (mAList->IsAnimating()) {
@@ -292,16 +303,17 @@ DOMSVGLengthList::RemoveItem(PRUint32 index,
   // Notify the DOM item of removal *before* modifying the lists so that the
   // DOM item can copy its *old* value:
   mItems[index]->RemovingFromList();
+  NS_ADDREF(*_retval = mItems[index]);
 
   InternalList().RemoveItem(index);
-
-  NS_ADDREF(*_retval = mItems[index]);
   mItems.RemoveElementAt(index);
+
   for (PRUint32 i = index; i < Length(); ++i) {
     if (mItems[i]) {
       mItems[i]->UpdateListIndex(i);
     }
   }
+
   Element()->DidChangeLengthList(AttrEnum(), PR_TRUE);
 #ifdef MOZ_SMIL
   if (mAList->IsAnimating()) {

content/svg/content/src/DOMSVGLengthList.h

@@ -76,17 +76,16 @@ public:
   NS_DECL_CYCLE_COLLECTION_CLASS(DOMSVGLengthList)
   NS_DECL_NSIDOMSVGLENGTHLIST
 
-  DOMSVGLengthList(DOMSVGAnimatedLengthList *aAList)
+  DOMSVGLengthList(DOMSVGAnimatedLengthList *aAList,
+                   const SVGLengthList &aInternalList)
     : mAList(aAList)
   {
-    // We silently ignore SetLength OOM failure since being out of sync is safe
-    // so long as we have *fewer* items than our internal list.
-
-    mItems.SetLength(InternalList().Length());
-    for (PRUint32 i = 0; i < Length(); ++i) {
-      // null out all the pointers - items are created on-demand
-      mItems[i] = nsnull;
-    }
+    // aInternalList must be passed in explicitly because we can't use
+    // InternalList() here. (Because it depends on IsAnimValList, which depends
+    // on this object having been assigned to aAList's mBaseVal or mAnimVal,
+    // which hasn't happend yet.)
+    
+    InternalListLengthWillChange(aInternalList.Length()); // Sync mItems
   }
 
   ~DOMSVGLengthList() {
@@ -129,6 +128,8 @@ private:
 
   /// Used to determine if this list is the baseVal or animVal list.
   PRBool IsAnimValList() const {
+    NS_ABORT_IF_FALSE(this == mAList->mBaseVal || this == mAList->mAnimVal,
+                      "Calling IsAnimValList() too early?!");
     return this == mAList->mAnimVal;
   }
 

content/svg/content/src/DOMSVGNumberList.cpp

@@ -204,26 +204,31 @@ DOMSVGNumberList::InsertItemBefore(nsIDOMSVGNumber *newItem,
   if (!domItem) {
     return NS_ERROR_DOM_SVG_WRONG_TYPE_ERR;
   }
-  index = NS_MIN(index, Length());
-  float value = domItem->ToSVGNumber(); // get before setting domItem
   if (domItem->HasOwner()) {
-    domItem = new DOMSVGNumber();
+    domItem = domItem->Clone(); // must do this before changing anything!
   }
-  PRBool ok = !!InternalList().InsertItem(index, value);
-  if (!ok) {
+  index = NS_MIN(index, Length());
+
+  // Ensure we have enough memory so we can avoid complex error handling below:
+  if (!mItems.SetCapacity(mItems.Length() + 1) ||
+      !InternalList().SetCapacity(InternalList().Length() + 1)) {
     return NS_ERROR_OUT_OF_MEMORY;
   }
+
+  InternalList().InsertItem(index, domItem->ToSVGNumber());
+  mItems.InsertElementAt(index, domItem.get());
+
+  // This MUST come after the insertion into InternalList(), or else under the
+  // insertion into InternalList() the values read from domItem would be bad
+  // data from InternalList() itself!:
   domItem->InsertingIntoList(this, AttrEnum(), index, IsAnimValList());
-  ok = !!mItems.InsertElementAt(index, domItem.get());
-  if (!ok) {
-    InternalList().RemoveItem(index);
-    return NS_ERROR_OUT_OF_MEMORY;
-  }
+
   for (PRUint32 i = index + 1; i < Length(); ++i) {
     if (mItems[i]) {
       mItems[i]->UpdateListIndex(i);
     }
   }
+
   Element()->DidChangeNumberList(AttrEnum(), PR_TRUE);
 #ifdef MOZ_SMIL
   if (mAList->IsAnimating()) {
@@ -251,19 +256,23 @@ DOMSVGNumberList::ReplaceItem(nsIDOMSVGNumber *newItem,
   if (index >= Length()) {
     return NS_ERROR_DOM_INDEX_SIZE_ERR;
   }
-  float length = domItem->ToSVGNumber(); // get before setting domItem
   if (domItem->HasOwner()) {
-    domItem = new DOMSVGNumber();
+    domItem = domItem->Clone(); // must do this before changing anything!
   }
+
   if (mItems[index]) {
     // Notify any existing DOM item of removal *before* modifying the lists so
     // that the DOM item can copy the *old* value at its index:
     mItems[index]->RemovingFromList();
   }
-  InternalList()[index] = length;
-  domItem->InsertingIntoList(this, AttrEnum(), index, IsAnimValList());
+
+  InternalList()[index] = domItem->ToSVGNumber();
   mItems[index] = domItem;
 
+  // This MUST come after the ToSVGPoint() call, otherwise that call
+  // would end up reading bad data from InternalList()!
+  domItem->InsertingIntoList(this, AttrEnum(), index, IsAnimValList());
+
   Element()->DidChangeNumberList(AttrEnum(), PR_TRUE);
 #ifdef MOZ_SMIL
   if (mAList->IsAnimating()) {
@@ -292,16 +301,17 @@ DOMSVGNumberList::RemoveItem(PRUint32 index,
   // Notify the DOM item of removal *before* modifying the lists so that the
   // DOM item can copy its *old* value:
   mItems[index]->RemovingFromList();
+  NS_ADDREF(*_retval = mItems[index]);
 
   InternalList().RemoveItem(index);
-
-  NS_ADDREF(*_retval = mItems[index]);
   mItems.RemoveElementAt(index);
+
   for (PRUint32 i = index; i < Length(); ++i) {
     if (mItems[i]) {
       mItems[i]->UpdateListIndex(i);
     }
   }
+
   Element()->DidChangeNumberList(AttrEnum(), PR_TRUE);
 #ifdef MOZ_SMIL
   if (mAList->IsAnimating()) {

content/svg/content/src/DOMSVGNumberList.h

@@ -79,12 +79,12 @@ public:
                    const SVGNumberList &aInternalList)
     : mAList(aAList)
   {
-    // We can NOT use InternalList() here - it depends on IsAnimValList, which
-    // in turn depends on this object having been assigned to either aAList's
-    // mBaseVal or mAnimVal. At this point we haven't been assigned to either!
-    // This is why our caller must pass in aInternalList.
+    // aInternalList must be passed in explicitly because we can't use
+    // InternalList() here. (Because it depends on IsAnimValList, which depends
+    // on this object having been assigned to aAList's mBaseVal or mAnimVal,
+    // which hasn't happend yet.)
 
-    InternalListLengthWillChange(aInternalList.Length()); // Initialize mItems
+    InternalListLengthWillChange(aInternalList.Length()); // Sync mItems
   }
 
   ~DOMSVGNumberList() {

content/svg/content/src/DOMSVGPathSegList.cpp

@@ -86,10 +86,8 @@ DOMSVGPathSegList::GetDOMWrapperIfExists(void *aList)
 
 DOMSVGPathSegList::~DOMSVGPathSegList()
 {
-  // We no longer have any list items, and there are no script references to
-  // us.
-  //
-  // Do NOT use InternalList() here! That's different!
+  // There are now no longer any references to us held by script or list items.
+  // Note we must use GetAnimValKey/GetBaseValKey here, NOT InternalList()!
   void *key = mIsAnimValList ?
     InternalAList().GetAnimValKey() :
     InternalAList().GetBaseValKey();
@@ -345,8 +343,8 @@ DOMSVGPathSegList::InsertItemBefore(nsIDOMSVGPathSeg *aNewItem,
   mItems.InsertElementAt(aIndex, ItemProxy(domItem.get(), internalIndex));
 
   // This MUST come after the insertion into InternalList(), or else under the
-  // insertion into InternalList() the data read from domItem would be bad data
-  // from InternalList() itself!:
+  // insertion into InternalList() the values read from domItem would be bad
+  // data from InternalList() itself!:
   domItem->InsertingIntoList(this, aIndex, IsAnimValList());
 
   for (PRUint32 i = aIndex + 1; i < Length(); ++i) {
@@ -411,7 +409,7 @@ DOMSVGPathSegList::ReplaceItem(nsIDOMSVGPathSeg *aNewItem,
   }
   ItemAt(aIndex) = domItem;
 
-  // This MUST come after the ToSVGPathSegEncodedData call otherwise that call
+  // This MUST come after the ToSVGPathSegEncodedData call, otherwise that call
   // would end up reading bad data from InternalList()!
   domItem->InsertingIntoList(this, aIndex, IsAnimValList());
 

content/svg/content/src/DOMSVGPathSegList.h

@@ -160,12 +160,7 @@ private:
     : mElement(aElement)
     , mIsAnimValList(aIsAnimValList)
   {
-    // This call populates mItems with the same number of items as there are
-    // segments contained in the internal list. We ignore OOM failure since
-    // being out of sync is safe so long as we have *fewer* items than our
-    // internal list.
-
-    InternalListWillChangeTo(InternalList());
+    InternalListWillChangeTo(InternalList()); // Sync mItems
   }
 
   ~DOMSVGPathSegList();

content/svg/content/src/DOMSVGPointList.cpp

@@ -86,10 +86,8 @@ DOMSVGPointList::GetDOMWrapperIfExists(void *aList)
 
 DOMSVGPointList::~DOMSVGPointList()
 {
-  // We no longer have any list items, and there are no script references to
-  // us.
-  //
-  // Do NOT use InternalList() as the key here! That's different!
+  // There are now no longer any references to us held by script or list items.
+  // Note we must use GetAnimValKey/GetBaseValKey here, NOT InternalList()!
   void *key = mIsAnimValList ?
     InternalAList().GetAnimValKey() :
     InternalAList().GetBaseValKey();
@@ -270,8 +268,9 @@ DOMSVGPointList::InsertItemBefore(nsIDOMSVGPoint *aNewItem,
   InternalList().InsertItem(aIndex, domItem->ToSVGPoint());
   mItems.InsertElementAt(aIndex, domItem.get());
 
-  // This MUST come after the insertion into InternalList(), or else the data
-  // read from domItem would be bad data from InternalList() itself!
+  // This MUST come after the insertion into InternalList(), or else under the
+  // insertion into InternalList() the values read from domItem would be bad
+  // data from InternalList() itself!:
   domItem->InsertingIntoList(this, aIndex, IsAnimValList());
 
   for (PRUint32 i = aIndex + 1; i < Length(); ++i) {
@@ -320,7 +319,7 @@ DOMSVGPointList::ReplaceItem(nsIDOMSVGPoint *aNewItem,
   InternalList()[aIndex] = domItem->ToSVGPoint();
   mItems[aIndex] = domItem;
 
-  // This MUST come after the assignment to InternalList, otherwise that call
+  // This MUST come after the ToSVGPoint() call, otherwise that call
   // would end up reading bad data from InternalList()!
   domItem->InsertingIntoList(this, aIndex, IsAnimValList());
 

content/svg/content/src/DOMSVGPointList.h

@@ -160,11 +160,7 @@ private:
     : mElement(aElement)
     , mIsAnimValList(aIsAnimValList)
   {
-    // This call populates mItems with the same number of items as there are
-    // points in the internal list. We ignore OOM failure since being out of
-    // sync is safe so long as we have *fewer* items than our internal list.
-
-    InternalListWillChangeTo(InternalList());
+    InternalListWillChangeTo(InternalList()); // Sync mItems
   }
 
   ~DOMSVGPointList();
@@ -190,8 +186,7 @@ private:
 
   SVGAnimatedPointList& InternalAList();
 
-  /// Creates an instance of the appropriate DOMSVGPoint sub-class for
-  // aIndex, if it doesn't already exist.
+  /// Creates a DOMSVGPoint for aIndex, if it doesn't already exist.
   void EnsureItemAt(PRUint32 aIndex);
 
   // Weak refs to our DOMSVGPoint items. The items are friends and take care

content/svg/content/test/Makefile.in

@@ -73,6 +73,7 @@ _TEST_FILES = \
 		animated-svg-image-helper.html \
 		animated-svg-image-helper.svg \
 		test_SVGLengthList.xhtml \
+		test_SVGLengthList-2.xhtml \
 		test_SVGPathSegList.xhtml \
 		test_SVGStyleElement.xhtml \
 		test_SVGxxxList.xhtml \

content/svg/content/test/test_SVGLengthList-2.xhtml

@@ -0,0 +1,50 @@
+<html xmlns="http://www.w3.org/1999/xhtml">
+<!--
+https://bugzilla.mozilla.org/show_bug.cgi?id=630760
+-->
+<head>
+  <title>Tests specific to SVGLengthList</title>
+  <script type="text/javascript" src="/MochiKit/packed.js"></script>
+  <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
+</head>
+<body>
+<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=515116">Mozilla Bug 630760</a>
+<p id="display"></p>
+<div id="content" style="display:none;">
+<svg id="svg" xmlns="http://www.w3.org/2000/svg" width="100" height="100">
+  <text id="text">
+    <set attributeName="x" to="10 20 30 40" begin="0" dur="indefinite"/>
+  </text>
+</svg>
+</div>
+<pre id="test">
+<script class="testbody" type="text/javascript">
+<![CDATA[
+
+SimpleTest.waitForExplicitFinish();
+
+/*
+Check that the animVal list for 'x' on <text> gives the correct number of
+items when examined for the FIRST time DURING animation.
+*/
+
+function run_tests()
+{
+  document.getElementById('svg').pauseAnimations();
+
+  var text = document.getElementById("text");
+  var list = text.x.animVal;
+
+  is(list.numberOfItems, 4, 'Checking numberOfItems');
+
+  SimpleTest.finish();
+}
+
+window.addEventListener("load", run_tests, false);
+
+]]>
+</script>
+</pre>
+</body>
+</html>