Closed Bug 641393 Opened 13 years ago Closed 13 years ago

In test_SVGxxxList.xhtml: ###!!! ASSERTION: Axis() isn't valid: 'mElement', file SVGLengthList.h, line 225

Categories

(Core :: SVG, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla6

People

(Reporter: dholbert, Assigned: dholbert)

References

Details

(Keywords: assertion)

Attachments

(5 files, 3 obsolete files)

STEPS TO REPRODUCE:
 In the objdir of a debug build, run:
>  TEST_PATH=content/svg/content/test/test_SVGxxxList.xhtml make mochitest-plain

ACTUAL RESULTS:
###!!! ASSERTION: Axis() isn't valid: 'mElement', file ../../../../../mozilla/content/svg/content/src/SVGLengthList.h, line 225
mozilla::SVGLengthListAndInfo::Axis() const (/scratch/work/builds/mozilla-central/mozilla-central.10-12-09.12-05/obj/content/svg/content/src/../../../../../mozilla/content/svg/content/src/SVGLengthList.h:226)
mozilla::SVGLengthListSMILType::Add(nsSMILValue&, nsSMILValue const&, unsigned int) const (/scratch/work/builds/mozilla-central/mozilla-central.10-12-09.12-05/obj/content/svg/content/src/../../../../../mozilla/content/svg/content/src/SVGLengthListSMILType.cpp:154)
nsISMILType::SandwichAdd(nsSMILValue&, nsSMILValue const&) const (/scratch/work/builds/mozilla-central/mozilla-central.10-12-09.12-05/obj/content/svg/content/src/../../../../dist/include/nsISMILType.h:200)
nsSMILValue::SandwichAdd(nsSMILValue const&) (/scratch/work/builds/mozilla-central/mozilla-central.10-12-09.12-05/obj/content/smil/../../../mozilla/content/smil/nsSMILValue.cpp:121)
nsSMILAnimationFunction::ComposeResult(nsISMILAttr const&, nsSMILValue&) (/scratch/work/builds/mozilla-central/mozilla-central.10-12-09.12-05/obj/content/smil/../../../mozilla/content/smil/nsSMILAnimationFunction.cpp:306)
nsSMILCompositor::ComposeAttribute() (/scratch/work/builds/mozilla-central/mozilla-central.10-12-09.12-05/obj/content/smil/../../../mozilla/content/smil/nsSMILCompositor.cpp:123)
DoComposeAttribute(nsSMILCompositor*, void*) (/scratch/work/builds/mozilla-central/mozilla-central.10-12-09.12-05/obj/content/smil/../../../mozilla/content/smil/nsSMILAnimationController.cpp:391)
nsTHashtable<nsSMILCompositor>::s_EnumStub(PLDHashTable*, PLDHashEntryHdr*, unsigned int, void*) (/scratch/work/builds/mozilla-central/mozilla-central.10-12-09.12-05/obj/content/smil/../../dist/include/nsTHashtable.h:421)
PL_DHashTableEnumerate (/scratch/work/builds/mozilla-central/mozilla-central.10-12-09.12-05/obj/xpcom/build/pldhash.c:754)
nsTHashtable<nsSMILCompositor>::EnumerateEntries(PLDHashOperator (*)(nsSMILCompositor*, void*), void*) (/scratch/work/builds/mozilla-central/mozilla-central.10-12-09.12-05/obj/content/smil/../../dist/include/nsTHashtable.h:242)
nsSMILAnimationController::DoSample(int) (/scratch/work/builds/mozilla-central/mozilla-central.10-12-09.12-05/obj/content/smil/../../../mozilla/content/smil/nsSMILAnimationController.cpp:481)
nsSMILAnimationController::Resample() (/scratch/work/builds/mozilla-central/mozilla-central.10-12-09.12-05/obj/content/svg/content/src/../../../../dist/include/nsSMILAnimationController.h:96)
nsSMILAnimationController::FlushResampleRequests() (/scratch/work/builds/mozilla-central/mozilla-central.10-12-09.12-05/obj/content/svg/content/src/../../../../dist/include/nsSMILAnimationController.h:108)
nsSVGElement::FlushAnimations() (/scratch/work/builds/mozilla-central/mozilla-central.10-12-09.12-05/obj/content/svg/content/src/../../../../../mozilla/content/svg/content/src/nsSVGElement.cpp:2502)
nsSVGSVGElement::SetCurrentTime(float) (/scratch/work/builds/mozilla-central/mozilla-central.10-12-09.12-05/obj/content/svg/content/src/../../../../../mozilla/content/svg/content/src/nsSVGSVGElement.cpp:559)
NS_InvokeByIndex_P (/scratch/work/builds/mozilla-central/mozilla-central.10-12-09.12-05/obj/xpcom/reflect/xptcall/src/md/unix/../../../../../../../mozilla/xpcom/reflect/xptcall/src/md/unix/xptcinvoke_x86_64_unix.cpp:195)

Reduced testcase coming up soon.
Attached image testcase 1
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
As shown by this mxr search...
http://mxr.mozilla.org/mozilla-central/search?string=propagate&find=content%2Fsvg%2Fcontent%2Fsrc&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central
> /content/svg/content/src/SVGNumberListSMILType.cpp
>     line 142 -- dest.SetInfo(valueToAdd.Element()); // propagate target element info!
>     line 155 -- dest.SetInfo(valueToAdd.Element()); // propagate target element info!
>     line 215 -- NS_ABORT_IF_FALSE(end.Element(), "Can't propagate target element");
>     line 229 -- result.SetInfo(end.Element()); // propagate target element info!
>
> /content/svg/content/src/SVGLengthListSMILType.cpp
>     line 160 -- // propagate flag:
>     line 302 -- // propagate flag:
>
> /content/svg/content/src/SVGPointListSMILType.cpp
>     line 126 -- dest.SetInfo(valueToAdd.Element()); // propagate target element info!
>     line 139 -- dest.SetInfo(valueToAdd.Element()); // propagate target element info!
>     line 200 -- NS_ABORT_IF_FALSE(end.Element(), "Can't propagate target element");
>     line 214 -- result.SetInfo(end.Element()); // propagate target element info!

... we're propagating the "Element()" pointer in Point lists & Number lists, but not in Length lists.  We need to fix that.
This first patch just upgrades the failing assertion (and a few other scary-looking assertions in SVG list code) to be NS_ABORT_IF_FALSE, to make us more likely to catch them failing in the future.
Attachment #519266 - Flags: review?(jwatt)
Per discussion with jwatt yesterday, this bug's real fix (coming up next) largely consists of "sync up code between other SVGxxxListSMILType classes and SVGLengthListSMILType", including some code style for sanity / consistency / easy-side-by-side-comparability-of-similar-classes.

The first thing I noticed when diffing the various files was that "namespace mozilla {" is there in SVGLengthListSMILType, but we use the 'using' syntax instead in the other SVG*List classes.  Per bug 633315 comment 4, I think we're standardizing on this syntax, so this patch cleans that up across all SVGxxxList.cpp and SVGxxxListSMILType.cpp files.

This patch also changes some "Length() == 0" comparisons to "IsEmpty()" calls in SVGLengthListSMILType.cpp, since I think the latter is generally preferred over comparisons-to-zero.  (and makes for easier readability, IMHO)
Attachment #519268 - Flags: review?(jwatt)
This patch transfers the logic from the number-list and point-list SMIL type classes into the length list class.  (Number-list and point-list have identical logic, which is nice.  Almost makes me wonder if we should be using a single templated class... but that's beyond the scope of this bug.)

On top of fixing the assertion from comment 0, this patch also makes us use |aCount| in SVGLengthListSMILType::Add(), because we previously were ignoring it.  I still need to write a test for this (with a repeating, cumulative animation).

I've just noticed that SVGPathSegListSMILType.cpp also suffers from the same |aCount|-ignoring issue, so it could probably do with a similar logic-sync-up.  I'll post a separate patch for that.
Attachment #519266 - Flags: review?(jwatt) → review+
Attachment #519268 - Flags: review?(jwatt) → review+
Attachment #519274 - Attachment description: patch 3: make SVGPathSegListSMILType logic match other list types → patch 3: make SVGLengthListSMILType logic match other list types
Here's a slightly-revised version of Patch 3.  Ready for review.  (I just tweaked it to use IsEmpty() instead of Element() to check for the identity value.  I made that tweak for consistency with the equivalent code in SVGPathSegListSMILType, whose patch is coming next.)
Attachment #519274 - Attachment is obsolete: true
Attachment #520075 - Flags: review?(jwatt)
BTW, for doing these reviews, I recommend using a merge editor to compare the modified class against e.g. SVGNumberListSMILType.cpp -- that's how I started when writing the patch, at least.
Attachment #520076 - Flags: review?(jwatt)
Attached patch patch 4: testsSplinter Review
This patch extends the test_SVGxxxList.xhtml mochitest as follows:
 - extends "item_is" for SVGPathSegList to be more thorough and actually check numeric values
 - extends the repeated animation in the test to have repeatCount=3 instead of repeatCount="2".  (This change is to test aCount-related bugs, as noted in comment 5.  We use Add() to generate the *base value* for repeated animations, and we don't start showing errors from aCount-disregarding-bugs until the third repeat, because that's when our base value goes from {1 * to-val} to {2 * to-val}.)
 - adds a function "attr_val_5b_firstItem_x3_constructor" to each test-bundle, to generate the expected first list-item after the repeatCount="3" animation completes & freezes.

The new test that uses "attr_val_5b_firstItem_x3_constructor" fails for length-list and path-seg-list without this bug's other patches.  It passes with them applied, though.
Attachment #520081 - Flags: review?(jwatt)
Comment on attachment 520076 [details] [diff] [review]
patch 4: make SVGPathSegListSMILType logic match other list types

Canceling review request on Patch 4, as it'll need to be rebased to apply on top of Cameron's patch in Bug 619498.
Attachment #520076 - Flags: review?(jwatt)
(In reply to comment #6)
> patch 3 v2: make SVGLengthListSMILType logic match other list types
> Here's a slightly-revised version of Patch 3. [...]  (I just
> tweaked it to use IsEmpty() instead of Element() [...] for consistency with
> SVGPathSegListSMILType

I undid comment 6's IsEmpty <--> Element change in this version, per jwatt's request & to preserve consistency with SVGNumberListSMILType, and I also shifted the added "SetInfo()" call in Interpolate down to the end of the method, just for consistency.  (This change doesn't affect functionality at all, as there are no return statements or relevant function-calls between the old & new patch's SetInfo placement.)

SIDE NOTE: Per IRL discussion with jwatt, we should probably add an "IsIdentity()" method on the various SVGXXXListAndInfo classes, but that might be better to do in a followup bug, across all the relevant classes.
Attachment #520075 - Attachment is obsolete: true
Attachment #520075 - Flags: review?(jwatt)
Attachment #525135 - Flags: review?(jwatt)
Attachment #525135 - Flags: review?(jwatt) → review+
Comment on attachment 520076 [details] [diff] [review]
patch 4: make SVGPathSegListSMILType logic match other list types

In un-bitrotting & extending this SVGPathSegListSMILType patch, I ended up spinning it off into a separate bug -- bug 649568.

Obsoleting the version here.
Attachment #520076 - Attachment is obsolete: true
Comment on attachment 520081 [details] [diff] [review]
patch 4: tests

/me renumbers the final tests-patch here from 'patch 5' to 'patch 4', per previous comment.

Once this gets review, I think this bug is done!  See comment 8 for a summary of what this patch does.
Attachment #520081 - Attachment description: patch 5: tests → patch 4: tests
Attachment #520081 - Flags: review?(jwatt) → review+
Target Milestone: --- → mozilla6
Depends on: 898915
You need to log in before you can comment on or make changes to this bug.