Closed Bug 632143 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: SVG, defect, critical)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla2.0b12
Tracking Status
blocking2.0 --- final+
status1.9.2 --- unaffected
status1.9.1 --- unaffected

People

(Reporter: jwatt, Assigned: dholbert)

Details

(Whiteboard: [sg:critical?][hardblocker])

Attachments

(7 files, 4 obsolete files)

561 bytes, image/svg+xml
Details
571 bytes, image/svg+xml
Details
9.60 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
11.66 KB, patch
roc
: review+
Details | Diff | Splinter Review
4.36 KB, patch
roc
: review+
Details | Diff | Splinter Review
5.56 KB, patch
roc
: review+
Details | Diff | Splinter Review
10.48 KB, patch
roc
: review+
Details | Diff | Splinter Review
This only affects FF4. Members of DOMSVGxxxList that change the list don't keep the animVal in sync which is pretty bad.
Attached patch patch for DOMSVGLengthList only (obsolete) — Splinter Review
Attachment #510363 - Attachment is obsolete: true
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.
Attachment #510386 - Flags: review?(roc)
Previous version of "patch for DOMSVGLengthList and DOMSVGNumberList only" had a compile error -- it needed s/DOMSVGLength/DOMSVGNumber/ in DOMSVGNumberList.cpp.

Fixed here.
Attachment #510386 - Attachment is obsolete: true
Attachment #511472 - Flags: review+
Version: unspecified → Trunk
Keywords: testcase-wanted
Whiteboard: [softblocker] → [sg:critical?][hardblocker]
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
...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
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.)
Keywords: testcase-wanted
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.
Attachment #511472 - Attachment is obsolete: true
Attachment #511596 - Flags: review+
Attachment #511592 - Flags: review?(roc)
+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?
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...
(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.
This addresses comment 9.  (only DOMSVGPathSegList.h/cpp are different from previous version.)
Attachment #511592 - Attachment is obsolete: true
Attachment #511860 - Flags: review?(roc)
Attachment #511592 - Flags: review?(roc)
Whiteboard: [sg:critical?][hardblocker] → [sg:critical?][hardblocker] [has patch]
Whiteboard: [sg:critical?][hardblocker] [has patch] → [sg:critical?][hardblocker][has partial patch]
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.)
...and here's the patch for DOMSVGPathSegList.  (see previous comment - that all applies to this patch, too.)
Attachment #512648 - Flags: review?(roc)
(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).)
Attachment #512298 - Attachment description: patch 2 WIP (for DOMSVGPointList) → patch 2 v1 (for DOMSVGPointList)
Attachment #512298 - Flags: review?(roc)
Attachment #512300 - Attachment description: patch 3 WIP (for DOMSVGPathSegList) → patch 3 v1 (for DOMSVGPathSegList)
Attachment #512300 - Flags: review?(roc)
Whiteboard: [sg:critical?][hardblocker][has partial patch] → [sg:critical?][hardblocker][has patch]
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
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [sg:critical?][hardblocker][has patch] → [sg:critical?][hardblocker]
Target Milestone: --- → mozilla2.0b12
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.