1292283 - stylo: Enable test_transitions_per_property.html

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P1)

Description

[Chris Peterson [:cpeterson]]

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

Comment 1

[Brian Birtles (:birtles, Feb 26~ parental leave)]

This might be fixed by bug 1302949 (and should certainly be fixed by bug 1302945)

Comment 3

[Xidorn Quan [:xidorn] UTC+11]

layout/style/test/test_transitions_per_property.html

Comment 4

[Boris Chiou [:boris]]

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

Comment 5

[Xidorn Quan [:xidorn] UTC+11]

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|.

Comment 6

[Xidorn Quan [:xidorn] UTC+11]

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: ??? (???:???)

Comment 7

[Boris Chiou [:boris]]

test_transitions_per_property.html still got some assertions after fixing bug 1346052, so I will keep looking at those problems.

Comment 8

[Hiroyuki Ikezoe (:hiro)]

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.

Comment 9

[Boris Chiou [:boris]]

(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.

Comment 10

[Boris Chiou [:boris]]

Attachment: Bug 1292283 - Enable test_transitions_per_property.html.

Review commit: https://reviewboard.mozilla.org/r/139502/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/139502/

Comment 11

[Boris Chiou [:boris]]

Comment on attachment 8867970 [details]
Bug 1292283 - Enable test_transitions_per_property.html.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139502/diff/1-2/

Comment 12

[Boris Chiou [:boris]]

Comment on attachment 8867970 [details]
Bug 1292283 - Enable test_transitions_per_property.html.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139502/diff/2-3/

Comment 13

[Hiroyuki Ikezoe (:hiro)]

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.

Comment 14

[Hiroyuki Ikezoe (:hiro)]

(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

[Xidorn Quan [:xidorn] UTC+11]

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.

Comment 16

[Hiroyuki Ikezoe (:hiro)]

(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?

Comment 17

[Boris Chiou [:boris]]

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.

Comment 18

[Boris Chiou [:boris]]

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 19

[Boris Chiou [:boris]]

The number of test failures (in test_transitions_per_property.html) is reduced to 760 today.

Comment 20

[Boris Chiou [:boris]]

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

Comment 21

[Boris Chiou [:boris]]

(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)

Comment 22

[Boris Chiou [:boris]]

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)

Comment 23

[Boris Chiou [:boris]]

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]

Comment 24

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Although this is mostly a meta bug, there are code changes, so marking as P1 so we don't forget to turn this on.

Comment 25

[Boris Chiou [:boris]]

(In reply to Brian Birtles (:birtles, away 31 July~7 Aug) from comment #24)
> Although this is mostly a meta bug, there are code changes, so marking as P1
> so we don't forget to turn this on.

There are still hundreds of failures in this test files, so I am going to file a meta for this test file, and I'd like to enable this test file in this bug. (will update the patch after Bug 1374233 landed)

Comment 26

[Xidorn Quan [:xidorn] UTC+11]

I'm a bit concerned about this, though, because I'm going to retire stylo-failures.md and enable all mochitest-plain soonish (in bug 1383992), which means we would not be able to track the failure number precisely.

How long would it take to fix all the failures in this test?

I don't want to regress test coverage (by disabling this test after it gets enabled), nor do I want to block bug 1383992 for too long...

Comment 27

[Boris Chiou [:boris]]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #26)
> I'm a bit concerned about this, though, because I'm going to retire
> stylo-failures.md and enable all mochitest-plain soonish (in bug 1383992),
> which means we would not be able to track the failure number precisely.
> 
> How long would it take to fix all the failures in this test?
> 
> I don't want to regress test coverage (by disabling this test after it gets
> enabled), nor do I want to block bug 1383992 for too long...

I see. I will check how many things we have to do for Enable test_transitions_per_property.html today. Thanks for this info.

Comment 28

[Boris Chiou [:boris]]

Excluding mask-image (about 500 failures or less?), there are still 656 failures (based on the tip of m-c today), so I think we might still need two or three more weeks to fix them all. Need to enable this ASAP because we have to avoid more regressions.

Comment 29

[Hiroyuki Ikezoe (:hiro)]

It seems to me that all other failures are scroll-snap-{coordinate,destination} and the failure reason is that gecko does not support them as animatable yet.

Comment 30

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #29)
> It seems to me that all other failures are
> scroll-snap-{coordinate,destination} and the failure reason is that gecko
> does not support them as animatable yet.

Can we conditionally disable those subtests inside the test, so that we can drop stylo-failures.md without regressing test coverage?

Comment 31

[Hiroyuki Ikezoe (:hiro)]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #30)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #29)
> > It seems to me that all other failures are
> > scroll-snap-{coordinate,destination} and the failure reason is that gecko
> > does not support them as animatable yet.
> 
> Can we conditionally disable those subtests inside the test, so that we can
> drop stylo-failures.md without regressing test coverage?

Yes we can, I think we should run test cases as discrete type on gecko and run as animatable on stylo.

Comment 32

[Boris Chiou [:boris]]

After applying bug 1374233 and bug 1386864, we still have these failures:

* Animation support:
  * SMIL Animation
    * test_transitions_per_property.html `stroke-dasharray` [8]
    * test_transitions_per_property.html `stroke-dashoffset` [1]
    * test_transitions_per_property.html `stroke-width` [2]
  * Compositor aniamtions
    * test_transitions_per_property.html `compositor transform transition` [34]
  * 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]
* Transition support:
  * test_transitions_per_property.html `Clip-path` [22]
  * test_transitions_per_property.html `Filter property` [2]
  * test_transitions_per_property.html `background-position` [24]
  * test_transitions_per_property.html `background-size` [8]
  * test_transitions_per_property.html `box-shadow` [10]
  * test_transitions_per_property.html `caret-color` [1]
  * test_transitions_per_property.html `column-count` [4]
  * test_transitions_per_property.html `font-weight` [3]
  * test_transitions_per_property.html `function parameters mismatch`: for clip-patch [22]
  * test_transitions_per_property.html `interpolation of transition`: transform [21]
  * 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 `scroll-snap-coordinate` [156]
  * test_transitions_per_property.html `scroll-snap-destination` [289]
  * test_transitions_per_property.html `text-shadow` [8]
  * test_transitions_per_property.html `vertical-align` [1]
  * test_transitions_per_property.html `z-index` [4]

Comment 33

[Boris Chiou [:boris]]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #30)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #29)
> > It seems to me that all other failures are
> > scroll-snap-{coordinate,destination} and the failure reason is that gecko
> > does not support them as animatable yet.
> 
> Can we conditionally disable those subtests inside the test, so that we can
> drop stylo-failures.md without regressing test coverage?

I can revise this file. If we are running Servo backend, skip the properties or test cases in comment 32.

Comment 34

[Boris Chiou [:boris]]

Comment on attachment 8867970 [details]
Bug 1292283 - Enable test_transitions_per_property.html.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139502/diff/3-4/

Comment 35

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8867970 [details]
Bug 1292283 - Enable test_transitions_per_property.html.

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

Cool. As far as we don't rely on stylo-failures.md anymore, all fine.

It would probably be better if we can do some kind of "fail-if" rather than just skip, but that would probably add much unnecessary complexity given that we are approaching the goal.

Comment 36

[Boris Chiou [:boris]]

Comment on attachment 8867970 [details]
Bug 1292283 - Enable test_transitions_per_property.html.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/139502/diff/4-5/

Comment 37

[Boris Chiou [:boris]]

I'm waiting for try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8531475c1a2c

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4b44e6320ba0
Enable test_transitions_per_property.html. r=hiro,xidorn

Comment 39

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4b44e6320ba0