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

RESOLVED FIXED in mozilla6

Status

()

Core
SVG
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

({assertion})

Trunk
mozilla6
assertion
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 3 obsolete attachments)

(Assignee)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
Created attachment 519069 [details]
testcase 1
(Assignee)

Updated

6 years ago
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
(Assignee)

Comment 2

6 years ago
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.
(Assignee)

Comment 3

6 years ago
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.
Attachment #519266 - Flags: review?(jwatt)
(Assignee)

Comment 4

6 years ago
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)
Attachment #519268 - Flags: review?(jwatt)
(Assignee)

Comment 5

6 years ago
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.

Updated

6 years ago
Attachment #519266 - Flags: review?(jwatt) → review+

Updated

6 years ago
Attachment #519268 - Flags: review?(jwatt) → review+
(Assignee)

Updated

6 years ago
Attachment #519274 - Attachment description: patch 3: make SVGPathSegListSMILType logic match other list types → patch 3: make SVGLengthListSMILType logic match other list types
(Assignee)

Comment 6

6 years ago
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.)
Attachment #519274 - Attachment is obsolete: true
Attachment #520075 - Flags: review?(jwatt)
(Assignee)

Comment 7

6 years ago
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.
Attachment #520076 - Flags: review?(jwatt)
(Assignee)

Comment 8

6 years ago
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.
Attachment #520081 - Flags: review?(jwatt)
(Assignee)

Comment 9

6 years ago
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)
(Assignee)

Comment 10

6 years ago
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.
Attachment #520075 - Attachment is obsolete: true
Attachment #520075 - Flags: review?(jwatt)
Attachment #525135 - Flags: review?(jwatt)

Updated

6 years ago
Attachment #525135 - Flags: review?(jwatt) → review+
(Assignee)

Comment 11

6 years ago
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
(Assignee)

Comment 12

6 years ago
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

Updated

6 years ago
Attachment #520081 - Flags: review?(jwatt) → review+
(Assignee)

Comment 13

6 years ago
Landed:
http://hg.mozilla.org/mozilla-central/rev/2676e6e841a8
http://hg.mozilla.org/mozilla-central/rev/40c0848eb1d6
http://hg.mozilla.org/mozilla-central/rev/43c4e9097654
http://hg.mozilla.org/mozilla-central/rev/2b204b01f4c3
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
(Assignee)

Updated

6 years ago
Target Milestone: --- → mozilla6

Updated

4 years ago
Depends on: 898915
You need to log in before you can comment on or make changes to this bug.