stylo: Enable test_transitions_per_property.html

ASSIGNED
Assigned to

Status

()

Core
CSS Parsing and Computation
P2
normal
ASSIGNED
10 months ago
9 days ago

People

(Reporter: cpeterson, Assigned: boris)

Tracking

(Blocks: 4 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

10 months 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

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

Updated

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

Comment 4

4 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

4 months ago
Status: NEW → RESOLVED
Last Resolved: 4 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

3 months ago
Depends on: 1346052
(Assignee)

Comment 7

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

Updated

26 days 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

13 days 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

13 days ago
Attachment #8867970 - Flags: review?(hikezoe)
(Assignee)

Updated

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

Comment 13

13 days 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

13 days 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

13 days 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

13 days 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

9 days 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
You need to log in before you can comment on or make changes to this bug.