Closed Bug 1227501 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: jruderman, Assigned: dbaron)

References

Details

(Keywords: assertion, testcase)

Attachments

(5 files, 1 obsolete file)

Attached file testcase (deleted) —
No description provided.
Attached file stack (deleted) —
QA Contact: dbaron
Assignee: nobody → dbaron
QA Contact: dbaron
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)
Attached patch patch 2 - Crashtest (deleted) — Splinter Review
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
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+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: