632143 - Members of DOMSVGxxxList that change the list don't keep the animVal in sync

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jonathan Watt [:jwatt]]

This only affects FF4. Members of DOMSVGxxxList that change the list don't keep the animVal in sync which is pretty bad.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: patch for DOMSVGLengthList only

Comment 2

[Jonathan Watt [:jwatt]]

Attachment: patch for DOMSVGLengthList and DOMSVGNumberList only

Comment 3

[Jonathan Watt [:jwatt]]

We'll need a patch for DOMSVGPointList and DOMSVGPathSegList too.

Note that the code for those classes is quite different to DOMSVGLengthList and DOMSVGNumberList because they don't have DOMSVGAnimatedXxxList parents. DOMSVGPathSegList also has the added complication that we have encoded segments internally. Also the patch for DOMSVGPointList and DOMSVGPathSegList will involve changes to other DOM methods on those classes. For example, Clear() will need more changes due to InternalListWillChangeTo not taking care of syncing the animated list when necessary.

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: patch 1 (for DOMSVGLengthList and DOMSVGNumberList only) [p=jwatt r=roc]

Previous version of "patch for DOMSVGLengthList and DOMSVGNumberList only" had a compile error -- it needed s/DOMSVGLength/DOMSVGNumber/ in DOMSVGNumberList.cpp.

Fixed here.

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: testcase 1 (length list)

This testcase kills a mozilla-central debug build (on load) with:
###!!! ABORT: DOM wrapper's list length is out of sync: 'mItems.Length() == 0 || mItems.Length() == const_cast<DOMSVGLengthList*>(this)->InternalList().Length()', file content/svg/content/src/DOMSVGLengthList.h, line 108

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: testcase 2 (number list)

...and this testcase triggers the equivalent abort for number lists:
###!!! ABORT: DOM wrapper's list length is out of sync: 'mItems.Length() == 0 || mItems.Length() == const_cast<DOMSVGNumberList*>(this)->InternalList().Length()', file content/svg/content/src/DOMSVGNumberList.h, line 107

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: patch 0: Add helper-method UpdateListIndicesFromIndex

The original version of patch 1 gives us 4 copies of the logic to do...
  "Iterate across some range of |mItems| and call UpdateListIndex() on
   all non-null entries."
...per .cpp file.

This helper-patch decomposes that logic out into a helper function (in the null namespace, to make it local to each compiled file) in all of the relevant files.

I'll post an updated version of patch 1 that makes use of the function in this patch.

(Ideally, all versions of this helper-method (except for the specialized one in DOMSVGPathSegList) could be merged into a single templatized function -- but I don't think there's a great centralized place to put it right now, and it seems silly to add a new file *just* for this tiny templated method.)

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: patch 1 v3 (for DOMSVGLengthList and DOMSVGNumberList only) [p=jwatt r=roc]

Here's an updated version of patch 1, to make use of the now-available methods in patch 0.

This also fixes a bug in patch 1 that (until this version) caused it to still abort on both attached testcases.

The original version of patch 1 called "animVal->Length();" after resizing animVal's array but before resizing InternalList(), so it ran afoul of length-matching-sanity-checks inside the DOMSVGxxxList::Length() methods.

This new version, in contrast, uses animVal->mItems.Length() instead of animVal->Length().  This returns the same thing, but just skips the sanity-checks.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+void
+DOMSVGPathSegList::UpdateListIndicesFromIndex(PRUint32 aStartingIndex,
+                                              ListMutationType aMutationType,
+                                              PRUint32 aArgCount)
+{
+  PRUint32 length = Length();
+
+  for (PRUint32 i = aStartingIndex; i < length; ++i) {
+    if (aMutationType == eInsertion) {
+      mItems[i].mInternalDataIndex += 1 + aArgCount;
+    } else { // aMutationType == eRemoval
+      mItems[i].mInternalDataIndex -= 1 + aArgCount;
+    }
+    if (ItemAt(i)) {
+      ItemAt(i)->UpdateListIndex(i);
+    }

Why not just take a signed parameter "aIndexDelta" which you always add to the index?

Comment 10

[Daniel Holbert [:dholbert]]

I initially opted for the enum over that to avoid signed/unsigned conversion issues -- but that was silly of me, given that |aArgCount| is restricted to the range [0, 6], so it's nowhere near the point where unsigned-->signed conversion would matter.

Making the suggested change...

Comment 11

[Daniel Holbert [:dholbert]]

(In reply to comment #3)
> Also the patch for DOMSVGPointList and DOMSVGPathSegList will
> involve changes to other DOM methods on those classes. For example, Clear()
> will need more changes due to InternalListWillChangeTo not taking care of
> syncing the animated list when necessary.

FWIW, Clear() is actually already fine on both DOMSVGPointList and DOMSVGPathSegList.  In both of those classes, Clear() calls
  " animList->InternalListWillChangeTo(SVGPathData());"
after calling InternalListWillChangeTo on itself.

Comment 12

[Daniel Holbert [:dholbert]]

Attachment: patch 0 v2: Add helper-method UpdateListIndicesFromIndex

This addresses comment 9.  (only DOMSVGPathSegList.h/cpp are different from previous version.)

Comment 13

[Daniel Holbert [:dholbert]]

Attachment: patch 2 v1 (for DOMSVGPointList)

Here's a patch to basically apply the same logic from patch 1 to DOMSVGPointList.  This patch fixes this bug's issues on a point-list version of my attached testcases.

I'm intending to write a mochitest for this bug, fix any problems that fall out of that, and then request review.  I also need to check whether other DOM methods need tweaking, per comment 3. (As I noted in Comment 11, "Clear()" is already fine. I'll include calls to it in my mochitest for sureness' sake.)

Comment 14

[Daniel Holbert [:dholbert]]

Attachment: patch 3 v1 (for DOMSVGPathSegList)

...and here's the patch for DOMSVGPathSegList.  (see previous comment - that all applies to this patch, too.)

Comment 15

[Daniel Holbert [:dholbert]]

Attachment: patch 4: mochitest

Comment 16

[Daniel Holbert [:dholbert]]

(In reply to comment #13)
> I'm intending to write a mochitest for this bug, fix any problems that fall out
> of that, and then request review.

Mochitest didn't uncover any new problems, so I think the patches attached are ready for review.

> I also need to check whether other DOM
> methods need tweaking, per comment 3.

(From glancing through the affected files, it looks like Clear & InsertItemBefore & RemoveItem are the only methods that modify the length of |mItems|, and Clear() is already fine as noted in Comment 11 (and as tested in the attached mochitest).)

Comment 17

[Daniel Holbert [:dholbert]]

Landed:
http://hg.mozilla.org/mozilla-central/rev/58cb9a41c8b4
http://hg.mozilla.org/mozilla-central/rev/78f8b9ee9ea8
http://hg.mozilla.org/mozilla-central/rev/2cf3e2ce1808
http://hg.mozilla.org/mozilla-central/rev/26b045c6f347
http://hg.mozilla.org/mozilla-central/rev/ab0dc35174fb