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)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla45
Tracking | Status | |
---|---|---|
firefox45 | --- | fixed |
People
(Reporter: jruderman, Assigned: dbaron)
References
Details
(Keywords: assertion, testcase)
Attachments
(5 files, 1 obsolete file)
(deleted),
text/html
|
Details | |
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
xidorn
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
xidorn
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Reporter | ||
Comment 1•9 years ago
|
||
Assignee | ||
Updated•9 years ago
|
QA Contact: dbaron
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → dbaron
QA Contact: dbaron
Assignee | ||
Comment 2•9 years ago
|
||
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)
Assignee | ||
Comment 3•9 years ago
|
||
Assignee | ||
Comment 4•9 years ago
|
||
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)
Assignee | ||
Comment 5•9 years ago
|
||
Comment 6•9 years ago
|
||
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 7•9 years ago
|
||
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)
Assignee | ||
Comment 8•9 years ago
|
||
(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
Assignee | ||
Comment 9•9 years ago
|
||
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)
Assignee | ||
Updated•9 years ago
|
Attachment #8691527 -
Attachment is obsolete: true
Comment 10•9 years ago
|
||
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+
Assignee | ||
Comment 11•9 years ago
|
||
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•9 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
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in
before you can comment on or make changes to this bug.
Description
•