Last Comment Bug 641393 - In test_SVGxxxList.xhtml: ###!!! ASSERTION: Axis() isn't valid: 'mElement', file SVGLengthList.h, line 225
: In test_SVGxxxList.xhtml: ###!!! ASSERTION: Axis() isn't valid: 'mElement', f...
Status: RESOLVED FIXED
: assertion
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla6
Assigned To: Daniel Holbert [:dholbert]
:
Mentors:
Depends on: 898915
Blocks:
  Show dependency treegraph
 
Reported: 2011-03-13 15:49 PDT by Daniel Holbert [:dholbert]
Modified: 2013-08-05 00:18 PDT (History)
0 users
dholbert: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase 1 (418 bytes, image/svg+xml)
2011-03-13 19:17 PDT, Daniel Holbert [:dholbert]
no flags Details
patch 1: upgrade assertions to ABORT_IF_FALSE (4.29 KB, patch)
2011-03-14 16:17 PDT, Daniel Holbert [:dholbert]
jwatt: review+
Details | Diff | Splinter Review
patch 2: non-functional tweaks for consistency & cleanup (9.98 KB, patch)
2011-03-14 16:30 PDT, Daniel Holbert [:dholbert]
jwatt: review+
Details | Diff | Splinter Review
patch 3: make SVGLengthListSMILType logic match other list types (5.37 KB, patch)
2011-03-14 16:41 PDT, Daniel Holbert [:dholbert]
no flags Details | Diff | Splinter Review
patch 3 v2: make SVGLengthListSMILType logic match other list types (5.35 KB, patch)
2011-03-17 16:58 PDT, Daniel Holbert [:dholbert]
no flags Details | Diff | Splinter Review
patch 4: make SVGPathSegListSMILType logic match other list types (4.66 KB, patch)
2011-03-17 17:00 PDT, Daniel Holbert [:dholbert]
no flags Details | Diff | Splinter Review
patch 4: tests (13.20 KB, patch)
2011-03-17 17:15 PDT, Daniel Holbert [:dholbert]
jwatt: review+
Details | Diff | Splinter Review
patch 3 v2: make SVGLengthListSMILType logic match other list types (4.68 KB, patch)
2011-04-11 12:36 PDT, Daniel Holbert [:dholbert]
jwatt: review+
Details | Diff | Splinter Review

Description Daniel Holbert [:dholbert] 2011-03-13 15:49:57 PDT
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.
Comment 1 Daniel Holbert [:dholbert] 2011-03-13 19:17:15 PDT
Created attachment 519069 [details]
testcase 1
Comment 2 Daniel Holbert [:dholbert] 2011-03-13 21:43:48 PDT
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.
Comment 3 Daniel Holbert [:dholbert] 2011-03-14 16:17:10 PDT
Created attachment 519266 [details] [diff] [review]
patch 1: upgrade assertions to ABORT_IF_FALSE

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.
Comment 4 Daniel Holbert [:dholbert] 2011-03-14 16:30:55 PDT
Created attachment 519268 [details] [diff] [review]
patch 2: non-functional tweaks for consistency & cleanup

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)
Comment 5 Daniel Holbert [:dholbert] 2011-03-14 16:41:08 PDT
Created attachment 519274 [details] [diff] [review]
patch 3: make SVGLengthListSMILType logic match other list types

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.
Comment 6 Daniel Holbert [:dholbert] 2011-03-17 16:58:48 PDT
Created attachment 520075 [details] [diff] [review]
patch 3 v2: 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.)
Comment 7 Daniel Holbert [:dholbert] 2011-03-17 17:00:21 PDT
Created attachment 520076 [details] [diff] [review]
patch 4: make SVGPathSegListSMILType logic match other list types

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.
Comment 8 Daniel Holbert [:dholbert] 2011-03-17 17:15:29 PDT
Created attachment 520081 [details] [diff] [review]
patch 4: tests

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.
Comment 9 Daniel Holbert [:dholbert] 2011-03-21 16:46:03 PDT
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.
Comment 10 Daniel Holbert [:dholbert] 2011-04-11 12:36:11 PDT
Created attachment 525135 [details] [diff] [review]
patch 3 v2: make SVGLengthListSMILType logic match other list types

(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.
Comment 11 Daniel Holbert [:dholbert] 2011-04-12 22:07:30 PDT
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.
Comment 12 Daniel Holbert [:dholbert] 2011-04-12 22:10:59 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.