Closed Bug 1269990 Opened 8 years ago Closed 8 years ago

mochitests for clip-path basic shapes trigger fatal "Assertion failure: aArray1->Item(1).GetIntValue() == aArray2->Item(1).GetIntValue() (expected matching geometry-box values)"

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla51
Tracking Flags:
Tracking Status
firefox49 --- disabled
firefox50 --- disabled
firefox51 --- fixed

People

(Reporter: dholbert, Assigned: u459114)

References

(Blocks 2 open bugs)

Details

(Keywords: assertion)

Attachments

(2 files, 1 obsolete file)

patch to enable the test and trip the assertion
16.71 KB, patch
Details | Diff | Splinter Review
Bug 1269990 -Refuse to interpolate when animating between clip-path shape values that have different reference boxes.
58 bytes, text/x-review-board-request
dholbert
: review+
Details 
As it turns out, the tests for transitions of "clip-path" basic shapes are mostly-disabled (even if you enable the pref), because its testing function throws an exception early on, which halts the test.

If I fix the test to avoid the exception, we trip a fatal assertion on one of the later testcases. :-/
This patch:
 * Enables the basic-shapes pref for testsuites.
 * Tweaks test_transitions_per_property.html to remove tests for all other properties (except for "clip-path"), so that it's faster to run.
 * Avoids the exception in what I think might be the correct way (avoid an I-think-incorrect pop() call on our "expected" list).  (The exception is mentioned in bug 1266868 comment 0, btw.)


STR for this bug (the assertion failure)
 1. Apply this patch to a debug build.
 2. ./mach mochitest layout/style/test/test_transitions_per_property.html

ACTUAL RESULTS:
Abort partway through the testrun. The test output ends with this:
{
69 INFO TEST-PASS | layout/style/test/test_transitions_per_property.html | Function should have close-paren 
70 INFO TEST-PASS | layout/style/test/test_transitions_per_property.html | Clip-path property is ellipse(500px 500px at 50% 50%) expected values of ellipse,500px,500px,at,50%,50% 
71 INFO TEST-PASS | layout/style/test/test_transitions_per_property.html | Function should have close-paren 
72 INFO TEST-PASS | layout/style/test/test_transitions_per_property.html | Clip-path property is ellipse(farthest-side farthest-side at 50% 50%) expected values of ellipse,farthest-side,farthest-side,at,50%,50% 
73 INFO TEST-PASS | layout/style/test/test_transitions_per_property.html | Function should have close-paren 
74 INFO TEST-PASS | layout/style/test/test_transitions_per_property.html | Clip-path property is ellipse(closest-side closest-side at 50% 50%) expected values of ellipse,closest-side,closest-side,at,50%,50% 
Assertion failure: aArray1->Item(1).GetIntValue() == aArray2->Item(1).GetIntValue() (expected matching geometry-box values), at /scratch/work/builds/mozilla-inbound/mozilla/layout/style/StyleAnimationValue.cpp:1942
#01: AddShapeFunction(nsCSSProperty, double, nsCSSValue::Array const*, double, nsCSSValue::Array const*) (/scratch/work/builds/mozilla-inbound/mozilla/layout/style/StyleAnimationValue.cpp:1941 (discriminator 2))
#02: mozilla::StyleAnimationValue::AddWeighted(nsCSSProperty, double, mozilla::StyleAnimationValue const&, double, mozilla::StyleAnimationValue const&, mozilla::StyleAnimationValue&) (/scratch/work/builds/mozilla-inbound/mozilla/layout/style/StyleAnimationValue.cpp:2607)
#03: mozilla::StyleAnimationValue::Interpolate(nsCSSProperty, mozilla::StyleAnimationValue const&, mozilla::StyleAnimationValue const&, double, mozilla::StyleAnimationValue&) (/scratch/work/builds/mozilla-inbound/obj/gfx/layers/../../dist/include/mozilla/StyleAnimationValue.h:105)
#04: nsTransitionManager::ConsiderStartingTransition(nsCSSProperty, mozilla::StyleTransition const&, mozilla::dom::Element*, mozilla::AnimationCollection<mozilla::dom::CSSTransition>*&, nsStyleContext*, nsStyleContext*, bool*, nsCSSPropertySet*) (/scratch/work/builds/mozilla-inbound/mozilla/layout/style/nsTransitionManager.cpp:566)
}
Flags: needinfo?(jwatt)
Blocks: basic-shape-ship
(Note: I've filed bug 1269992 on fixing the exception that gets thrown by the mochitest, mentioned in comment 0 here.)
Blocks: 1266570
Keywords: assertion
(Rebasing patch that triggers/demonstrates this bug.)
Attachment #8748488 - Attachment is obsolete: true
Take
Assignee: nobody → cku
Test cases with different start-shape-box and end-shape-box violate this assertion
{ start: "ellipse(100px 100px at 100px 100px) border-box",
    end: "ellipse(500px 500px at 500px 500px) content-box",
    expected: ["ellipse", ["500px", "500px", "at", "500px", "500px"], "content-box"]
  },

MOZ_ASSERT(aArray1->Item(1).GetIntValue() == aArray2->Item(1).GetIntValue(),
           "expected matching geometry-box values");
aArray1->Item(1).GetIntValue() is equal to border-box 
aArray2->Item(1).GetIntValue() is equal to content-box
According to the spec[1]
> Both shapes must use the same reference box. 
Different reference box is not valid.

[1] https://drafts.csswg.org/css-shapes/#basic-shape-interpolation
Attachment #8778809 - Flags: review?(dholbert)
Blocks: 1293590
Blocks: 1293810
Comment on attachment 8778809 [details]
Bug 1269990 -Refuse to interpolate when animating between clip-path shape values that have different reference boxes.

https://reviewboard.mozilla.org/r/69966/#review67872

Thanks for fixing this! The code looks good, but the commit message needs some fixing.

Current commit message:
> Bug 1269990 - Handle clip-path animation with different start/end geomerty-box.

(1) typo: "geomerty"
(2) BUT, don't bother fixing (1), because that's the wrong term -- as you probably noticed in the spec (and the code-comment you're adding), the spec uses the term "reference box". We should be using that term here too.
(3) The phrase "Handle...animation" is a bit misleading here, since we don't actually do any useful animation in this case.  "Refuse to interpolate" is a bit more specific / accurate about describing the change.

So, please replace commit message with something like the following:
> Bug 1269990 - Refuse to interpolate when animating between clip-path shape values that have different reference boxes. r=dholbert
Attachment #8778809 - Flags: review?(dholbert) → review+
BTW: I filed bug 1293810 on using "reference box" consistently.  (There are a few more usages of the wrong term, "geometry box", outside of the code that this patch is touching.)

Also, I'll clear the needinfo=jwatt since CJ took this.
Flags: needinfo?(jwatt)
Pushed by cku@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ecd7d4abe30d
Refuse to interpolate when animating between clip-path shape values that have different reference boxes. r=dholbert
https://hg.mozilla.org/mozilla-central/rev/ecd7d4abe30d
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
CJ, is it something you are going to uplift to aurora and beta? thanks
status-firefox50: --- → affected
Flags: needinfo?(cku)
We do not pref-on clip-path by default and this bug is just assertion failure, so probably not.
Flags: needinfo?(cku)
Actually, disabled is probably a better keyword
status-firefox49: wontfix → disabled
status-firefox50: wontfix → disabled
Blocks: basic-shape
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: