Closed Bug 1343168 Opened 7 years ago Closed 7 years ago

stylo: several tests crash with "panicked at 'assertion failed: v.0.len() > 0'"

Categories

(Core :: CSS Parsing and Computation, defect, P1)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

Details

Attachments

(8 files, 1 obsolete file)

Replace assert! in animation code with debug_assert!
59 bytes, text/x-review-board-request
Details
Make animation-name parse none
59 bytes, text/x-review-board-request
xidorn
: review+
Details
Add / fix get_initial_specified_value for animation longhands
59 bytes, text/x-review-board-request
Details
Allow animation-name to be omitted from shorthand
59 bytes, text/x-review-board-request
Details
Simplify animation shorthand parsing code
59 bytes, text/x-review-board-request
Details
Parse none as just a normal animation item
59 bytes, text/x-review-board-request
Details
Treat empty animation entry invalid
59 bytes, text/x-review-board-request
emilio
: review+
Details
Bug 1343168 - Update mochitest expectation.
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.
Yeah, I did add an assertion for each setter of animation properties other than animation name in bug 1330824,  we have to drop them.
Hmmm, test_value_computation.html also crashes on this. It makes this one a pretty high priority crash then.
Priority: -- → P1
Xidorn, does the patch fix this?
panicked at 'attempt to calculate the remainder with a divisor of zero'
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)
Please also remove the skip-ifs I added in bug 1343166 when this gets fixed.
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
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 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)
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)
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 :)
Flags: needinfo?(xidorn+moz)
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. :-)
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
Why not just set allow_empty to be True for them?
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. :-)
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?
That makes sense to me. Xidorn, would you mind fixing it altogether?
Attachment #8841911 - Attachment is obsolete: true
OK, I can try...
Assignee: nobody → xidorn+moz
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
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.
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.
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. ;-)
Depends on: 1344209
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 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 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 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+
(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.
Attachment #8843806 - Flags: review+ → review?(emilio+bugs)
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+
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2830ef14f98b
Update mochitest expectation. r=emilio
https://hg.mozilla.org/mozilla-central/rev/2830ef14f98b
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Thank you Xidorn!
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: