1227501 - "Assertion failure: 0 <= aProperty && aProperty < eCSSProperty_COUNT (out of range)" with "will-change: --t"

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Jesse Ruderman]

Attachment: testcase

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 1 - Don't check flags for eCSSPropertyExtra_variable

I tested locally that the crashtest in patch 2 hits the fatal assertion
without this patch, and passes with the patch.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 2 - Crashtest

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 3 - Handle custom properties correctly in transition-property

Without the code change, the property_database.js changes lead to:

 * 3 failures in test_value_cloning.html (one each for
   transition-property, -moz-transition-property, and
   -webkit-transition-property) that "computed value should be nonempty"

 * 6 failures in test_value_computation.html (two each for
   *transition-property) that "should not get empty value"

 * 16 failures in test_value_storage.html "parse+compute+serialize...
   should be idempotent":  2 for each longhand, 2 for the shorthand for
   the unprefixed, and 4 for the shorthand for each prefixed

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ad5633db33bf&group_state=expanded

Comment 6

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8691525 [details] [diff] [review]
patch 1 - Don't check flags for eCSSPropertyExtra_variable

Review of attachment 8691525 [details] [diff] [review]:
-----------------------------------------------------------------

It probably doesn't much what the spec says. The spec says:
> If any non-initial value of a property would create a stacking context on
> the element, specifying that property in will-change must create a stacking
> context on the element.
> 
> If any non-initial value of a property would cause the element to generate
> a containing block for fixed-position elements, specifying that property in
> will-change must cause the element to generate a containing block for
> fixed-position elements.

So it seems to me if the variable is going to be applied on properties like "position", it could create a stacking context, and the same for "transform" which generates containing block.

But that may make will-change unnecessarily complicated. We probably should get the spec explicitly exclude the custom properties.

Fixing crash is more important anyway, so r=me.

Comment 7

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8691527 [details] [diff] [review]
patch 3 - Handle custom properties correctly in transition-property

Review of attachment 8691527 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/nsStyleStruct.cpp
@@ +2730,1 @@
>    mProperty = eCSSProperty_UNKNOWN;

It should be eCSSPropertyExtra_variable, no?

All callsites of StyleTransition::GetProperty() except that in nsComputedDOMStyle explicitly check against eCSSPropertyExtra_variable. We should either remove those checks in nsTransitionManager, or (probably more right) set mProperty to prop and get nsComputedDOMStyle::DoGetTransitionProperty() fixed.

Also the spec says custom properties 'always use the "flips at 50%" behavior', which doesn't seem to be what we are currently doing. That should probably be in another bug, though.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #6)
> But that may make will-change unnecessarily complicated. We probably should
> get the spec explicitly exclude the custom properties.

Posted https://lists.w3.org/Archives/Public/www-style/2015Nov/0332.html

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 3 - Handle custom properties correctly in transition-property

Without the code change, the property_database.js changes lead to:

 * 3 failures in test_value_cloning.html (one each for
   transition-property, -moz-transition-property, and
   -webkit-transition-property) that "computed value should be nonempty"

 * 6 failures in test_value_computation.html (two each for
   *transition-property) that "should not get empty value"

 * 16 failures in test_value_storage.html "parse+compute+serialize...
   should be idempotent":  2 for each longhand, 2 for the shorthand for
   the unprefixed, and 4 for the shorthand for each prefixed

Comment 10

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8691682 [details] [diff] [review]
patch 3 - Handle custom properties correctly in transition-property

Review of attachment 8691682 [details] [diff] [review]:
-----------------------------------------------------------------

The name of mUnknownProperty and SetUnknownProperty could be misleading, but I guess it's fine.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e37ce5b407eff2a70a2e5547db955298d30ee986
Bug 1227501 patch 1 - Don't check flags for eCSSPropertyExtra_variable.  r=xidorn

https://hg.mozilla.org/integration/mozilla-inbound/rev/068458c29b6941b082e054ab974fb540679df895
Bug 1227501 patch 2 - Crashtest.

https://hg.mozilla.org/integration/mozilla-inbound/rev/7fe7a1db6ab8a89dca40e374f87260b6467c19f0
Bug 1227501 patch 3 - Handle custom properties correctly in transition-property.  r=xidorn

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e37ce5b407ef
https://hg.mozilla.org/mozilla-central/rev/068458c29b69
https://hg.mozilla.org/mozilla-central/rev/7fe7a1db6ab8