Closed Bug 1292283 Opened 5 years ago Closed 4 years ago

stylo: Enable test_transitions_per_property.html

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- wontfix
firefox56 --- wontfix
firefox57 --- fixed

People

(Reporter: cpeterson, Assigned: boris)

References

Details

(Keywords: assertion, Whiteboard: [stylo:m2])

Attachments

(1 file)

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
Priority: -- → P1
Duplicate of this bug: 1292284
layout/style/test/test_transitions_per_property.html
Assignee: nobody → boris.chiou
I didn't see the assertions now on the tip of bookmarks/stylo today, so this might be fixed by our other changes.
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Blocks: 1337599
No longer blocks: stylo-style-mochitest
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
Depends on: 1346052
test_transitions_per_property.html still got some assertions after fixing bug 1346052, so I will keep looking at those problems.
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)
(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)
Attachment #8867970 - Flags: review?(hikezoe)
Attachment #8867970 - Flags: review?(hikezoe)
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 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?
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 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.
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
Depends on: 1353918
(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)
Depends on: 1370803
Depends on: 1370808
Depends on: 1369624, 1369625, 1360133
No longer depends on: 1353918
Priority: P2 → P1
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)
Depends on: 1370845
Depends on: 1370846
Depends on: 1362897
No longer blocks: stylo
Depends on: 1374233
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]
Depends on: 1374564, 1374513
Depends on: 1374966
Priority: P1 → --
Although this is mostly a meta bug, there are code changes, so marking as P1 so we don't forget to turn this on.
Priority: -- → P1
Depends on: 1386864
(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)
Blocks: 1387080
No longer blocks: stylo-reftest-crashes, 1337599
No longer depends on: 1386864
No longer blocks: 1292284, 1302945
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...
(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.
Depends on: 1386864
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.
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.
(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?
(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.
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]
(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.
No longer depends on: 1374513
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.
Attachment #8867970 - Flags: review?(xidorn+moz) → review+
Pushed by bchiou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4b44e6320ba0
Enable test_transitions_per_property.html. r=hiro,xidorn
https://hg.mozilla.org/mozilla-central/rev/4b44e6320ba0
Status: ASSIGNED → RESOLVED
Closed: 5 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.