Closed
Bug 1343168
Opened 8 years ago
Closed 8 years ago
stylo: several tests crash with "panicked at 'assertion failed: v.0.len() > 0'"
Categories
(Core :: CSS Parsing and Computation, defect, P1)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: xidorn, Assigned: xidorn)
References
Details
Attachments
(8 files, 1 obsolete file)
59 bytes,
text/x-review-board-request
|
Details | |
59 bytes,
text/x-review-board-request
|
xidorn
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
Details | |
59 bytes,
text/x-review-board-request
|
Details | |
59 bytes,
text/x-review-board-request
|
Details | |
59 bytes,
text/x-review-board-request
|
Details | |
59 bytes,
text/x-review-board-request
|
emilio
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
emilio
:
review+
|
Details |
See this log: https://treeherder.mozilla.org/logviewer.html#?job_id=80577755&repo=autoland&lineNumber=94958
It seems to be something related to animation.
What may be related is that, in this servo change, I make the specified value to be an empty vector for `animation: none` and `transition: none`. Probably some animation code doesn't handle that properly.
Comment 1•8 years ago
|
||
Yeah, I did add an assertion for each setter of animation properties other than animation name in bug 1330824, we have to drop them.
Assignee | ||
Comment 2•8 years ago
|
||
Hmmm, test_value_computation.html also crashes on this. It makes this one a pretty high priority crash then.
Priority: -- → P1
Comment hidden (mozreview-request) |
Comment 4•8 years ago
|
||
Xidorn, does the patch fix this?
Comment 5•8 years ago
|
||
Assignee | ||
Comment 6•8 years ago
|
||
panicked at 'attempt to calculate the remainder with a divisor of zero'
Assignee | ||
Comment 7•8 years ago
|
||
mozreview-review |
Comment on attachment 8841911 [details]
Bug 1343168 - Drop assertions and set initial values when setting animation property with empty vector.
https://reviewboard.mozilla.org/r/115972/#review117354
It fixes that assertion, , but hits another assertion that "attempt to calculate the remainder with a divisor of zero".
Attachment #8841911 -
Flags: review?(xidorn+moz)
Assignee | ||
Comment 8•8 years ago
|
||
Please also remove the skip-ifs I added in bug 1343166 when this gets fixed.
Comment 9•8 years ago
|
||
Oh thanks, I missed test_value_computation.html. Hmm I wonder the change does not cause the zero division on servo...
https://hg.mozilla.org/mozilla-central/file/1bc2ad020aee/servo/components/style/properties/properties.mako.rs#l1266
Assignee | ||
Comment 10•8 years ago
|
||
Happens in these three tests:
/layout/style/test/test_value_cloning.html
/layout/style/test/test_value_computation.html
/layout/style/test/test_value_storage.html
Summary: stylo: test_value_cloning.html crashes with "panicked at 'assertion failed: v.0.len() > 0'" → stylo: several tests crash with "panicked at 'assertion failed: v.0.len() > 0'"
Comment hidden (mozreview-request) |
Comment 12•8 years ago
|
||
Comment on attachment 8841911 [details]
Bug 1343168 - Drop assertions and set initial values when setting animation property with empty vector.
I did drop reviewer name, but MozReview did not drop it.
Attachment #8841911 -
Flags: review?(xidorn+moz)
Comment 13•8 years ago
|
||
This patch fixes the assertion and the zero division. Note that this patch does not include mochitest.ini change since the change has not been merged into mozilla-central. MozReview does not allow us to push patches against autoland.:(
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6589d7eca506892154d773fa0fcf47215ebc9093
Two concerns:
* I am still wondering whether the zero division also happens on servo or not, if it happens we should pass initial values in stead of empty vector in the first place?
* The failure cases for animation in test_value_storage.html increased. I haven't looked into the cases yet, I will handle it after bug 1335938 is fixed.
Xidorn, what do you think?
Flags: needinfo?(xidorn+moz)
Assignee | ||
Comment 14•8 years ago
|
||
I don't think I can review that patch... I'm not familiar enough with animation code in servo. Probably you want to ask someone else to review.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #13)
> Two concerns:
> * I am still wondering whether the zero division also happens on servo or
> not, if it happens we should pass initial values in stead of empty vector in
> the first place?
I have no idea what really happens here. Initial value would be computed to an empty vec in style struct, no? How do you handle that currently? Or animation code is operating on specified value rather than computed value?
> * The failure cases for animation in test_value_storage.html increased. I
> haven't looked into the cases yet, I will handle it after bug 1335938 is
> fixed.
I probably should come to those failures and try to identify individual failure patterns there. I used a unified item for them because at that moment animation wasn't supposed to work at all. But now I think at list animation properties should really work? If you will come to that quicker than me, that would be great as well :)
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(xidorn+moz)
Comment 15•8 years ago
|
||
Thank you Xidorn! I don't know so much about servo's animation code either. I will check it.
As for the test case, I'd like to leave it to you because you are really a quick worker. :-)
Comment 16•8 years ago
|
||
OK. Now I believe we should set initial values instead of empty value for animation and transition properties in servo side because we are setting allow_empty[1] to False for those properties (The comment there is something wrong).
[1] https://hg.mozilla.org/mozilla-central/file/1bc2ad020aee/servo/components/style/properties/helpers.mako.rs#l64
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a338770dfd99275b3b2a15c48334bdb63197c2a5
Assignee | ||
Comment 17•8 years ago
|
||
Why not just set allow_empty to be True for them?
Comment 18•8 years ago
|
||
Oops I did send a PR now.
https://github.com/servo/servo/pull/15780
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #17)
> Why not just set allow_empty to be True for them?
I am not sure. :-)
Assignee | ||
Comment 19•8 years ago
|
||
It doesn't seem to me pushing an "initial" value to the vec is the right solution. Those properties can be set to none, and in that case, the computed value should be an empty vec. So it seems to me making them allow_empty=True is the right solution. Actually why do we have allow_empty at all... shouldn't all properties allow empty vec?
Comment 20•8 years ago
|
||
That makes sense to me. Xidorn, would you mind fixing it altogether?
Updated•8 years ago
|
Attachment #8841911 -
Attachment is obsolete: true
Assignee | ||
Comment 22•8 years ago
|
||
OK, I think I was wrong, and all animation subproperties (including animation-name) should have allow_empty be False, any they all have an initial value with a single value.
So the fix should be:
* set allow_empty to False for animation-name as well, and
* fill all properties with their initial value in shorthand parsing
Assignee | ||
Comment 23•8 years ago
|
||
The weird part is that transition-property allows empty, but animation-name doesn't. animation-name can have multiple none specified. I don't know what does that mean, and whether Gecko follows that.
Assignee | ||
Comment 24•8 years ago
|
||
servo/servo#15793 should probably fix this (although I'm too lazy to check with stylo build). And I'm going to re-enable those tests in this bug.
I'm not sure whether transition shorthand would need the same fix. Maybe not given there is no glue code at the moment?
It seems to me transition-property has some more fundamental issue that Servo's parsing code currently rejects unknown properties, which the spec explicitly forbids. Also unlike animation-name, it seems transition-property should really have allow_empty=True, so how we should handle that with the transition array in style struct is still an open question.
Comment 25•8 years ago
|
||
Wow! Great Xidorn!
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #24)
> I'm not sure whether transition shorthand would need the same fix. Maybe not
> given there is no glue code at the moment?
No, at this moment. I started to write it but based on current code. I am waiting for
your work for transition as well. ;-)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 34•8 years ago
|
||
emilio, the only things changes since your last review on GitHub are
* the change in servo/components/style/matching.rs to check the animation, and
* the last non-test patch that treats empty animation item invalid
There are several failure increments in the expectation. Some of them are probably from servo/servo#15779 which would need further investigation.
Comment 35•8 years ago
|
||
mozreview-review |
Comment on attachment 8843806 [details]
Make animation-name parse none
https://reviewboard.mozilla.org/r/117388/#review119054
::: servo/components/style/matching.rs:401
(Diff revision 1)
> if box_style.transition_property_count() > 0 {
> debug!("Failing to insert to the cache: transitions");
> return;
> }
>
> - if box_style.animation_name_count() > 0 {
> + if (0..box_style.animation_name_count()).any(|i| {
nit: I think you can write this down with box_style.animation_name_iter().any(..), but actually, can we encapsulate this bits with something like `ComputedStyle::specifies_animations()`?
::: servo/components/style/properties/longhand/box.mako.rs:829
(Diff revision 1)
>
> impl Parse for SpecifiedValue {
> fn parse(_context: &ParserContext, input: &mut ::cssparser::Parser) -> Result<Self, ()> {
> use cssparser::Token;
> Ok(match input.next() {
> - Ok(Token::Ident(ref value)) if value != "none" => SpecifiedValue(Atom::from(&**value)),
> + Ok(Token::Ident(ref value)) => SpecifiedValue(if value == "none" {
Perhaps mentioning in a comment that this needs to change to support `@keyframes ""` is worth it?
Attachment #8843806 -
Flags: review?(emilio+bugs) → review+
Comment 36•8 years ago
|
||
mozreview-review |
Comment on attachment 8843811 [details]
Treat empty animation entry invalid
https://reviewboard.mozilla.org/r/117398/#review119056
I'm assuming this is tested by the style system mochitests, please land with a test if not.
Attachment #8843811 -
Flags: review?(emilio+bugs) → review+
Comment 37•8 years ago
|
||
mozreview-review |
Comment on attachment 8843812 [details]
Bug 1343168 - Update mochitest expectation.
https://reviewboard.mozilla.org/r/117400/#review119060
::: layout/reftests/bugs/reftest-stylo.list:577
(Diff revision 1)
> == 364968-1.xul 364968-1.xul
> fails == 364989-1.html 364989-1.html
> fails == 365173-1.html 365173-1.html
> == 366207-1.xul 366207-1.xul
> == 366616-1.xul 366616-1.xul
> -fails asserts-if(stylo,30) == 367220-1.html 367220-1.html # bug 1324704
> +skip fails asserts-if(stylo,30) == 367220-1.html 367220-1.html # bug 1324704 # crash bug 1342106
Hmm, interesting, I can try to look into why.
::: layout/style/test/stylo-failures.md:452
(Diff revision 1)
> * test_specified_value_serialization.html [27]
> * test_units_angle.html [3]
> - * test_value_storage.html `columns:`: servo/servo#15190 [32]
> + * test_value_storage.html `columns:`: **need investigation** [20]
> * {background,mask}-position lacks comma for serialization servo/servo#15200
> * test_value_storage.html `background-position` [81]
> - * ... `for 'mask-position` [18]
> + * ... `for 'mask-position` [94]
These changes seem unrelated, what's the reason for these?
Assuming it's just syncing. r=me
Attachment #8843812 -
Flags: review?(emilio+bugs) → review+
Assignee | ||
Comment 38•8 years ago
|
||
(In reply to Emilio Cobos Álvarez [:emilio] from comment #36)
> Comment on attachment 8843811 [details]
> Treat empty animation entry invalid
>
> https://reviewboard.mozilla.org/r/117398/#review119056
>
> I'm assuming this is tested by the style system mochitests, please land with
> a test if not.
This was added to fix a big bump on failure number of test_property_syntax_errors.html... so yes it is tested by the mochitests.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8843806 -
Flags: review+ → review?(emilio+bugs)
Assignee | ||
Comment 46•8 years ago
|
||
Comment on attachment 8843806 [details]
Make animation-name parse none
bholley did a quick review on the inter-diff for this patch.
Attachment #8843806 -
Flags: review?(emilio+bugs) → review+
Comment 47•8 years ago
|
||
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2830ef14f98b
Update mochitest expectation. r=emilio
Comment 48•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Comment 49•8 years ago
|
||
Thank you Xidorn!
You need to log in
before you can comment on or make changes to this bug.
Description
•