stylo: Enable test_transitions_per_property.html

ASSIGNED
Assigned to

Status

()

Core
CSS Parsing and Computation
P1
normal
ASSIGNED
a year ago
a month ago

People

(Reporter: cpeterson, Assigned: boris)

Tracking

(Depends on: 2 bugs, Blocks: 2 bugs, {assertion})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [stylo:m2])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

a year ago
To reproduce, run `./mach reftest --disable-e10s <filename>` or replace <filename> with `layout/reftest/reftest.list`.

TEST-UNEXPECTED-FAIL | file:///home/shinglyu/workspace/stylo/gecko-dev/layout/reftests/svg/smil/container/deferred-anim-1.xhtml | application terminated with exit code 11
REFTEST PROCESS-CRASH | file:///home/shinglyu/workspace/stylo/gecko-dev/layout/reftests/svg/smil/container/deferred-anim-1.xhtml | application crashed [None]

Assertion failure: aStyleContext->PresContext()->StyleSet()->IsGecko() (ServoStyleSet should not use StyleAnimationValue for animations), at /files/mozilla/stylo/bb/gecko/layout/style/StyleAnimationValue.cpp:2831
Blocks: 1292284
This might be fixed by bug 1302949 (and should certainly be fixed by bug 1302945)
Blocks: 1302945
(Reporter)

Updated

10 months ago
Priority: -- → P1
Duplicate of this bug: 1292284
layout/style/test/test_transitions_per_property.html
Blocks: 1321197
(Assignee)

Updated

6 months ago
Assignee: nobody → boris.chiou
(Assignee)

Comment 4

6 months ago
I didn't see the assertions now on the tip of bookmarks/stylo today, so this might be fixed by our other changes.
(Assignee)

Updated

6 months ago
Status: NEW → RESOLVED
Last Resolved: 6 months ago
Resolution: --- → FIXED
Blocks: 1337599
No longer blocks: 1321197
No, it wasn't fixed. I can still reproduce this issue with test_transition_per_property.html.

To reproduce, you need to remove stylo from skip-if of test_transitions_per_property.html in layout/style/test/mochitest.ini, and execute |./mach mochitest --disable-e10s layout/style/test/test_transitions_per_property.html|.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
The stack here:
Assertion failure: aStyleContext->PresContext()->StyleSet()->IsGecko() (ServoStyleSet should not use StyleAnimationValue for animations), at c:/mozilla-source/stylo/layout/style/StyleAnimationValue.cpp:3488
#01: mozilla::StyleAnimationValue::ComputeValue (c:\mozilla-source\stylo\layout\style\styleanimationvalue.cpp:3579)
#02: ComputeAnimationValue (c:\mozilla-source\stylo\dom\base\nsdomwindowutils.cpp:2447)
#03: nsDOMWindowUtils::ComputeAnimationDistance (c:\mozilla-source\stylo\dom\base\nsdomwindowutils.cpp:2706)
#04: XPTC__InvokebyIndex (c:\mozilla-source\stylo\xpcom\reflect\xptcall\md\win32\xptcinvoke_asm_x86_64.asm:99)
#05: CallMethodHelper::Call (c:\mozilla-source\stylo\js\xpconnect\src\xpcwrappednative.cpp:1331)
#06: XPCWrappedNative::CallMethod (c:\mozilla-source\stylo\js\xpconnect\src\xpcwrappednative.cpp:1296)
#07: XPC_WN_CallMethod (c:\mozilla-source\stylo\js\xpconnect\src\xpcwrappednativejsops.cpp:983)
#08: js::CallJSNative (c:\mozilla-source\stylo\js\src\jscntxtinlines.h:281)
#09: js::InternalCallOrConstruct (c:\mozilla-source\stylo\js\src\vm\interpreter.cpp:463)
#10: js::Call (c:\mozilla-source\stylo\js\src\vm\interpreter.cpp:527)
#11: js::fun_apply (c:\mozilla-source\stylo\js\src\jsfun.cpp:1243)
#12: js::CallJSNative (c:\mozilla-source\stylo\js\src\jscntxtinlines.h:281)
#13: js::InternalCallOrConstruct (c:\mozilla-source\stylo\js\src\vm\interpreter.cpp:463)
#14: Interpret (c:\mozilla-source\stylo\js\src\vm\interpreter.cpp:2960)
#15: js::RunScript (c:\mozilla-source\stylo\js\src\vm\interpreter.cpp:409)
#16: js::InternalCallOrConstruct (c:\mozilla-source\stylo\js\src\vm\interpreter.cpp:481)
#17: Interpret (c:\mozilla-source\stylo\js\src\vm\interpreter.cpp:2960)
#18: js::RunScript (c:\mozilla-source\stylo\js\src\vm\interpreter.cpp:409)
#19: js::InternalCallOrConstruct (c:\mozilla-source\stylo\js\src\vm\interpreter.cpp:481)
#20: js::Call (c:\mozilla-source\stylo\js\src\vm\interpreter.cpp:527)
#21: js::ScriptedProxyHandler::call (c:\mozilla-source\stylo\js\src\proxy\scriptedproxyhandler.cpp:1155)
#22: js::Proxy::call (c:\mozilla-source\stylo\js\src\proxy\proxy.cpp:421)
#23: js::proxy_Call (c:\mozilla-source\stylo\js\src\proxy\proxy.cpp:662)
#24: js::CallJSNative (c:\mozilla-source\stylo\js\src\jscntxtinlines.h:281)
#25: js::InternalCallOrConstruct (c:\mozilla-source\stylo\js\src\vm\interpreter.cpp:451)
#26: js::Call (c:\mozilla-source\stylo\js\src\vm\interpreter.cpp:527)
#27: js::Wrapper::call (c:\mozilla-source\stylo\js\src\proxy\wrapper.cpp:165)
#28: js::CrossCompartmentWrapper::call (c:\mozilla-source\stylo\js\src\proxy\crosscompartmentwrapper.cpp:351)
#29: js::Proxy::call (c:\mozilla-source\stylo\js\src\proxy\proxy.cpp:421)
#30: js::proxy_Call (c:\mozilla-source\stylo\js\src\proxy\proxy.cpp:662)
#31: js::CallJSNative (c:\mozilla-source\stylo\js\src\jscntxtinlines.h:281)
#32: js::InternalCallOrConstruct (c:\mozilla-source\stylo\js\src\vm\interpreter.cpp:451)
#33: Interpret (c:\mozilla-source\stylo\js\src\vm\interpreter.cpp:2960)
#34: js::RunScript (c:\mozilla-source\stylo\js\src\vm\interpreter.cpp:409)
#35: js::InternalCallOrConstruct (c:\mozilla-source\stylo\js\src\vm\interpreter.cpp:481)
#36: js::jit::DoCallFallback (c:\mozilla-source\stylo\js\src\jit\baselineic.cpp:2332)
#37: ??? (???:???)
#38: ??? (???:???)
Priority: P1 → P3

Updated

5 months ago
Depends on: 1346052
(Assignee)

Comment 7

3 months ago
test_transitions_per_property.html still got some assertions after fixing bug 1346052, so I will keep looking at those problems.
(Assignee)

Updated

3 months ago
Status: REOPENED → ASSIGNED
Component: Layout → CSS Parsing and Computation
Priority: P3 → P2
Summary: Stylo Assertion failure: aStyleContext->PresContext()->StyleSet()->IsGecko() (ServoStyleSet should not use StyleAnimationValue for animations), at layout/style/StyleAnimationValue.cpp:2831 → stylo: Enable test_transitions_per_property.html
Boris, I think we should enable this test since no assertion happens now.  Once we enabled this, we can easily see what remaining problem are there, i.e. serialization, unsupported animatable properties, bug in interpolation, etc.
Flags: needinfo?(boris.chiou)
(Assignee)

Comment 9

2 months ago
(In reply to Hiroyuki Ikezoe (:hiro) from comment #8)
> Boris, I think we should enable this test since no assertion happens now. 
> Once we enabled this, we can easily see what remaining problem are there,
> i.e. serialization, unsupported animatable properties, bug in interpolation,
> etc.

Good news. I will update this soon.
Flags: needinfo?(boris.chiou)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 months ago
Attachment #8867970 - Flags: review?(hikezoe)
(Assignee)

Updated

2 months ago
Attachment #8867970 - Flags: review?(hikezoe)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 13

2 months ago
mozreview-review
Comment on attachment 8867970 [details]
Bug 1292283 - Enable test_transitions_per_property.html.

https://reviewboard.mozilla.org/r/139502/#review142872

Thanks for doing this1

::: layout/style/test/stylo-failures.md:71
(Diff revision 3)
>  * console support bug 1352669
>    * test_bug413958.html `monitorConsole` [3]
>    * test_parser_diagnostics_unprintables.html [550]
>  * Transition support:
>    * test_transitions.html: pseudo elements [12]
> +  * test_transitions_per_property.html [4051]

Is this number based on mozilla-central?
I am guessing/hoping the number has been reduced on autoland thanks to stylo team's recent works.
Attachment #8867970 - Flags: review?(hikezoe) → review+
(In reply to Hiroyuki Ikezoe (:hiro) from comment #13)
> Comment on attachment 8867970 [details]
> Bug 1292283 - Enable test_transitions_per_property.html.
> 
> https://reviewboard.mozilla.org/r/139502/#review142872
> 
> Thanks for doing this1

A shift key on my keyboard is something broken, or my little finger is broken.:-)

Comment 15

2 months ago
mozreview-review
Comment on attachment 8867970 [details]
Bug 1292283 - Enable test_transitions_per_property.html.

https://reviewboard.mozilla.org/r/139502/#review142876

::: layout/style/test/stylo-failures.md:71
(Diff revision 3)
>  * console support bug 1352669
>    * test_bug413958.html `monitorConsole` [3]
>    * test_parser_diagnostics_unprintables.html [550]
>  * Transition support:
>    * test_transitions.html: pseudo elements [12]
> +  * test_transitions_per_property.html [4051]

That's a lot of failures...

I'm not sure what the current status is, but it may worth spliting the number a bit based on the cause, so that the work can be parallelized better I guess.
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #15)
> Comment on attachment 8867970 [details]
> Bug 1292283 - Enable test_transitions_per_property.html.
> 
> https://reviewboard.mozilla.org/r/139502/#review142876
> 
> ::: layout/style/test/stylo-failures.md:71
> (Diff revision 3)
> >  * console support bug 1352669
> >    * test_bug413958.html `monitorConsole` [3]
> >    * test_parser_diagnostics_unprintables.html [550]
> >  * Transition support:
> >    * test_transitions.html: pseudo elements [12]
> > +  * test_transitions_per_property.html [4051]
> 
> That's a lot of failures...
> 
> I'm not sure what the current status is, but it may worth spliting the
> number a bit based on the cause, so that the work can be parallelized better
> I guess.

Yes, sounds good. Boris, can you please split them into smaller groups?
(Assignee)

Comment 17

2 months ago
mozreview-review-reply
Comment on attachment 8867970 [details]
Bug 1292283 - Enable test_transitions_per_property.html.

https://reviewboard.mozilla.org/r/139502/#review142872

> Is this number based on mozilla-central?
> I am guessing/hoping the number has been reduced on autoland thanks to stylo team's recent works.

Yes, by the mc yesterday. However, I will split it by properties. Thanks for reviewing.
(Assignee)

Comment 18

2 months ago
mozreview-review-reply
Comment on attachment 8867970 [details]
Bug 1292283 - Enable test_transitions_per_property.html.

https://reviewboard.mozilla.org/r/139502/#review142876

> That's a lot of failures...
> 
> I'm not sure what the current status is, but it may worth spliting the number a bit based on the cause, so that the work can be parallelized better I guess.

OK, I will try to split it by different properties. Thanks for suggestion.
Comment hidden (obsolete)
(Assignee)

Comment 20

2 months ago
Before we enable this test file, it's better to make sure these properties are animable, or we will get an uncatched exception, NS_ERROR_ILLEGAL_VALUE [1], because they are not animatable:
1. -moz-box-flex
2. -moz-image-region
3. -moz-outline-radius-{*}
4. fill
5. fill-opacity
6. flood-color
...etc.

[1] http://searchfox.org/mozilla-central/rev/24c443a440104dabd9447608bd41b8766e8fc2f5/dom/base/nsDOMWindowUtils.cpp#2690
(Assignee)

Updated

2 months ago
Depends on: 1353918
(Assignee)

Comment 21

2 months ago
(In reply to Boris Chiou [:boris] (away 6/9~6/12) from comment #20)
> Before we enable this test file, it's better to make sure these properties
> are animable, or we will get an uncatched exception, NS_ERROR_ILLEGAL_VALUE
> [1], because they are not animatable:
> 1. -moz-box-flex
> 2. -moz-image-region
> 3. -moz-outline-radius-{*}
> 4. fill
> 5. fill-opacity
> 6. flood-color
> ...etc.
> 
> [1]
> http://searchfox.org/mozilla-central/rev/
> 24c443a440104dabd9447608bd41b8766e8fc2f5/dom/base/nsDOMWindowUtils.cpp#2690

Updated:
1. -moz-box-flex (discrete, no bug)
2. -moz-image-region (discrete, no bug)
3. -moz-outline-radius-{*} (normal, no bug)
4. fill-opacity (Bug 1369624)
5. flood-color (Bug 1369625)
6. flood-opacity (Bug 1360133)
7. lighting-color (Bug 1369625)
8. stop-color (Bug 1369625)
9. stop-opacity (Bug 1360133)
10. -moz-tab-size (normal no bug)
11. text-declaration (null exception)
(Assignee)

Updated

2 months ago
Depends on: 1370803
(Assignee)

Updated

2 months ago
Depends on: 1370808
(Assignee)

Updated

2 months ago
Depends on: 1369624, 1369625, 1360133
No longer depends on: 1353918
(Assignee)

Updated

2 months ago
Priority: P2 → P1
(Assignee)

Comment 22

2 months ago
And there are some assertions for compositor animations:

GECKO(47564) | [Child 47565] ###!!! ASSERTION: Unexpected UpdateTransformLayer hint: '!(aChange & nsChangeHint_UpdateTransformLayer) || aFrame->IsTransformed() || aFrame->StyleDisplay()->HasTransformStyle()', file /Users/boris/projects/firefox/gecko/layout/base/RestyleManager.cpp, line 1186
GECKO(47564) | #01: mozilla::ServoRestyleManager::DoProcessPendingRestyles(mozilla::TraversalRestyleBehavior) (nsTArray.h:398, in XUL)
GECKO(47564) | #02: mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush) (ServoRestyleManager.cpp:655, in XUL)
GECKO(47564) | #03: nsDocument::FlushPendingNotifications(mozilla::FlushType) (nsDocument.h:1290, in XUL)
GECKO(47564) | #04: nsComputedDOMStyle::UpdateCurrentStyleSources(bool) (nsComputedDOMStyle.cpp:813, in XUL)
GECKO(47564) | #05: nsComputedDOMStyle::GetPropertyCSSValue(nsAString const&, mozilla::ErrorResult&) (nsComputedDOMStyle.cpp:1000, in XUL)
GECKO(47564) | #06: nsComputedDOMStyle::GetPropertyValue(nsAString const&, nsAString&) (AlreadyAddRefed.h:121, in XUL)
GECKO(47564) | #07: mozilla::dom::CSSStyleDeclarationBinding::getPropertyValue(JSContext*, JS::Handle<JSObject*>, nsICSSDeclaration*, JSJitMethodCallArgs const&) (nsICSSDeclaration.h:129, in XUL)
GECKO(47564) | #08: mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) (BindingUtils.cpp:2956, in XUL)
(Assignee)

Updated

2 months ago
Depends on: 1370845
(Assignee)

Updated

2 months ago
Depends on: 1370846
(Assignee)

Updated

2 months ago
Depends on: 1362897
(Reporter)

Updated

a month ago
No longer blocks: 1243581
(Assignee)

Updated

a month ago
Depends on: 1374233
(Assignee)

Comment 23

a month ago
Updated:

* SVG properties
  * test_transitions_per_property.html `stroke-dasharray` [12]
  * test_transitions_per_property.html `stroke-dashoffset` [1]
  * test_transitions_per_property.html `stroke-width` [3]
* Compositor aniamtions
  * test_transitions_per_property.html `compositor transform transition` [39]
* Interpolate with auto
  * test_transitions_per_property.html `clip`: interpolation on auto [1]
  * test_transitions_per_property.html `moz-image-region`: interpolation on rect and auto [2]
* Clamping negative values (Bug 1374233)
  * test_transitions_per_property.html `clamping of negatives` [42]
* Transforms
  * test_transitions_per_property.html `interpolation of transition`: transform [23]
* Grid
  * test_transitions_per_property.html `grid-template-columns` [245]
  * test_transitions_per_property.html `grid-template-rows` [245]
* Unimplemented?
  * test_transitions_per_property.html `font-variant-alternates` [6]
* Visited coloring?
  * test_transitions_per_property.html `caret-color` [1]
* Others
  * test_transitions_per_property.html `background-image` [561]
  * test_transitions_per_property.html `background-position` [24]
  * test_transitions_per_property.html `background-size` [8]
  * test_transitions_per_property.html `border-image-source` [561]
  * test_transitions_per_property.html `Clip-path` [22]
  * test_transitions_per_property.html `function parameters mismatch`: for clip-patch [22]
  * test_transitions_per_property.html `Filter property` [2]
  * test_transitions_per_property.html `column-count` [3]
  * test_transitions_per_property.html `scroll-snap-coordinate` [169]
  * test_transitions_per_property.html `scroll-snap-destination` [289]
  * test_transitions_per_property.html `box-shadow` [10]
  * test_transitions_per_property.html `text-shadow` [8]
  * test_transitions_per_property.html `mask-image` [561]
  * test_transitions_per_property.html `mask-position` [24]
  * test_transitions_per_property.html `mask-size` [8]
  * test_transitions_per_property.html `order` [1]
  * test_transitions_per_property.html `vertical-align` [1]
  * test_transitions_per_property.html `z-index` [4]

* Assertions
  * test_transitions_per_property.html asserts: Unexpected UpdateTransformLayer, ComputeLineHeight screwed up [28]
(Assignee)

Updated

a month ago
Depends on: 1374564, 1374513
(Assignee)

Updated

a month ago
Depends on: 1374966
You need to log in before you can comment on or make changes to this bug.