All users were logged out of Bugzilla on October 13th, 2018

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

RESOLVED FIXED in Firefox 45

Status

()

--
critical
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jruderman, Assigned: dbaron)

Tracking

(Blocks: 1 bug, {assertion, testcase})

Trunk
mozilla45
assertion, testcase
Points:
---

Firefox Tracking Flags

(firefox45 fixed)

Details

Attachments

(5 attachments, 1 obsolete attachment)

(Reporter)

Description

3 years ago
Created attachment 8691354 [details]
testcase
(Reporter)

Comment 1

3 years ago
Created attachment 8691355 [details]
stack
QA Contact: dbaron
Assignee: nobody → dbaron
QA Contact: dbaron
Created attachment 8691525 [details] [diff] [review]
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.
Attachment #8691525 - Flags: review?(quanxunzhen)
Created attachment 8691527 [details] [diff] [review]
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
Attachment #8691527 - Flags: review?(quanxunzhen)
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.
Attachment #8691525 - Flags: review?(quanxunzhen) → review+
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.
Attachment #8691527 - Flags: review?(quanxunzhen)
(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
Created attachment 8691682 [details] [diff] [review]
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
Attachment #8691682 - Flags: review?(quanxunzhen)
Attachment #8691527 - Attachment is obsolete: true
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.
Attachment #8691682 - Flags: review?(quanxunzhen) → review+

Comment 12

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e37ce5b407ef
https://hg.mozilla.org/mozilla-central/rev/068458c29b69
https://hg.mozilla.org/mozilla-central/rev/7fe7a1db6ab8
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox45: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.