Closed
Bug 641393
Opened 14 years ago
Closed 14 years ago
In test_SVGxxxList.xhtml: ###!!! ASSERTION: Axis() isn't valid: 'mElement', file SVGLengthList.h, line 225
Categories
(Core :: SVG, defect)
Core
SVG
Tracking
()
RESOLVED
FIXED
mozilla6
People
(Reporter: dholbert, Assigned: dholbert)
References
Details
(Keywords: assertion)
Attachments
(5 files, 3 obsolete files)
(deleted),
image/svg+xml
|
Details | |
(deleted),
patch
|
jwatt
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jwatt
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jwatt
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jwatt
:
review+
|
Details | Diff | Splinter Review |
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•14 years ago
|
||
Assignee | ||
Updated•14 years ago
|
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•14 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•14 years ago
|
||
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•14 years ago
|
||
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•14 years ago
|
||
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•14 years ago
|
Attachment #519266 -
Flags: review?(jwatt) → review+
Updated•14 years ago
|
Attachment #519268 -
Flags: review?(jwatt) → review+
Assignee | ||
Updated•14 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•14 years ago
|
||
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•14 years ago
|
||
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•14 years ago
|
||
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•14 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•14 years ago
|
||
(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•14 years ago
|
Attachment #525135 -
Flags: review?(jwatt) → review+
Assignee | ||
Comment 11•14 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•14 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•14 years ago
|
Attachment #520081 -
Flags: review?(jwatt) → review+
Assignee | ||
Comment 13•14 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
Closed: 14 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Assignee | ||
Updated•14 years ago
|
Target Milestone: --- → mozilla6
You need to log in
before you can comment on or make changes to this bug.
Description
•