1330824 - stylo: several style system tests fatally asserts with "Assertion failure: display->mAnimationNameCount > 0 (first item must be explicit)"

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Xidorn Quan [:xidorn] UTC+10]

This is a new crash since last time. Likely related to recent animation work.

To reproduce:
./mach mochitest --disable-e10s layout/style/test/test_compute_data_with_start_struct.html

Stack:
Assertion failure: display->mAnimationNameCount > 0 (first item must be explicit), at c:/mozilla-source/stylo/layout/style/nsComputedDOMStyle.cpp:6457
#01: nsComputedDOMStyle::GetPropertyCSSValue (c:\mozilla-source\stylo\layout\style\nscomputeddomstyle.cpp:840)
#02: nsComputedDOMStyle::GetPropertyValue (c:\mozilla-source\stylo\layout\style\nscomputeddomstyle.cpp:402)
#03: mozilla::dom::CSSStyleDeclarationBinding::getPropertyValue (c:\mozilla-source\stylo\obj-firefox-stylo\dom\bindings\cssstyledeclarationbinding.cpp:165)
#04: mozilla::dom::GenericBindingMethod (c:\mozilla-source\stylo\dom\bindings\bindingutils.cpp:2914)

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

Hiro, it seems you are actively working on animation support for stylo, could you have a look at this crash?

Comment 2

[Hiroyuki Ikezoe (:hiro)]

Sure, I will look into this.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

It seems that we shouldn't set 0 to mAnimationNameCount in set_animation_name(). (also and other properties)

[1] https://hg.mozilla.org/incubator/stylo/file/4ed9af9b5810/servo/components/style/properties/gecko.mako.rs#l1368

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

In addition to that, the following three (big) tests also crash with the same assertion:
layout/style/test/test_value_cloning.html
layout/style/test/test_value_computation.html
layout/style/test/test_value_storage.html

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1330824 - Use impl_copy_animation_value for animation-timing-function.

I did forget to do it in bug 1328786.

Review commit: https://reviewboard.mozilla.org/r/104424/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/104424/

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1330824 - Add truncate for nsAString and nsACString.

Review commit: https://reviewboard.mozilla.org/r/104426/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/104426/

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1330824 - Add IndexMut for nsStyleAutoArray.

Review commit: https://reviewboard.mozilla.org/r/104428/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/104428/

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1330824 - Do not set mAnimationNameCount less than 1.

animation-name property is 'allow_empty=True', we have to handle 0-length
array case.

Review commit: https://reviewboard.mozilla.org/r/104430/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/104430/

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1330824 - Do not copy animation property over mAnimationXXCount.

Review commit: https://reviewboard.mozilla.org/r/104432/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/104432/

Comment 10

[Hiroyuki Ikezoe (:hiro)]

I did include changes for mochitest.ini to clarify which test cases are fixed by the patch, but I will not include the changes when I push these patches to inbound, since it will not work until patches for servo-side has been merged.

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Also note that the assertion is fixed by these patches, but the test cases still fail, since, I guess handling initial value or serialization is something wrong.

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #10)
> I did include changes for mochitest.ini to clarify which test cases are
> fixed by the patch, but I will not include the changes when I push these
> patches to inbound, since it will not work until patches for servo-side has
> been merged.

Please include the mochitest.ini change at the same time.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #11)
> Also note that the assertion is fixed by these patches, but the test cases
> still fail, since, I guess handling initial value or serialization is
> something wrong.

Not crashing is enough. Thanks for fixing.

Comment 13

[Hiroyuki Ikezoe (:hiro)]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #12)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #10)
> > I did include changes for mochitest.ini to clarify which test cases are
> > fixed by the patch, but I will not include the changes when I push these
> > patches to inbound, since it will not work until patches for servo-side has
> > been merged.
> 
> Please include the mochitest.ini change at the same time.

OK, will do.

Comment 14

[Cameron McCormack (:heycam)]

Comment on attachment 8826463 [details]
Bug 1330824 - Use impl_copy_animation_value for animation-timing-function.

https://reviewboard.mozilla.org/r/104424/#review105068

Comment 15

[Cameron McCormack (:heycam)]

Comment on attachment 8826464 [details]
Bug 1330824 - Add truncate for nsAString and nsACString.

https://reviewboard.mozilla.org/r/104426/#review105074

Looks fine to me, but should get mystor to sign off on it.

::: servo/components/style/gecko_bindings/nsstring_vendor/src/lib.rs:678
(Diff revision 1)
> +    pub unsafe fn truncate(&mut self) {
> +        Gecko_TruncateString(self);

Nit: prefer unsafe inside the function, so that callers can call this safely.

::: xpcom/rust/nsstring/src/lib.rs:708
(Diff revision 1)
> +    pub unsafe fn truncate(&mut self) {
> +        Gecko_TruncateString(self);

And here.

Comment 16

[Cameron McCormack (:heycam)]

Comment on attachment 8826465 [details]
Bug 1330824 - Add IndexMut for nsStyleAutoArray.

https://reviewboard.mozilla.org/r/104428/#review105080

Comment 17

[Cameron McCormack (:heycam)]

Comment on attachment 8826466 [details]
Bug 1330824 - Do not set mAnimationNameCount less than 1.

https://reviewboard.mozilla.org/r/104430/#review105082

::: layout/style/test/mochitest.ini:305
(Diff revision 1)
>  [test_unprefixing_service.html]
>  support-files = unprefixing_service_iframe.html unprefixing_service_utils.js
>  [test_unprefixing_service_prefs.html]
>  support-files = unprefixing_service_iframe.html unprefixing_service_utils.js
>  [test_value_cloning.html]
> -skip-if = toolkit == 'android' || stylo # bug 775227 for android, bug 1330824 for stylo
> +skip-if = toolkit == 'android'# bug 775227 for android

Nit: space before "#".

Comment 18

[Cameron McCormack (:heycam)]

Comment on attachment 8826467 [details]
Bug 1330824 - Do not copy animation property over mAnimationXXCount.

https://reviewboard.mozilla.org/r/104432/#review105084

::: servo/components/style/properties/gecko.mako.rs:1024
(Diff revision 1)
>          self.gecko.mAnimation${gecko_ffi_name}Count = other.gecko.mAnimation${gecko_ffi_name}Count;
>          for (index, animation) in self.gecko.mAnimations.iter_mut().enumerate() {
> +            // The length of mAnimations is often greater than mAnimationXXCount,
> +            // don't copy values over the count.
> +            if index >= self.gecko.mAnimation${gecko_ffi_name}Count as usize {
> +                break;
> +            }
>              animation.m${gecko_ffi_name} = other.gecko.mAnimations[index].m${gecko_ffi_name};
>          }

If you want to be a bit shorter, you can use take():

let count = other.gecko.mAnimation${gecko_ffi_name}Count;
self.gecko.mAnimation${gecko_ffi_name}Count = count;

for (index, animation) in self.gecko.mAnimations.iter_mut().enumerate().take(count) {
  animation.m${gecko_ffi_name} = ...
}

Same below, too.

Comment 19

[Hiroyuki Ikezoe (:hiro)]

(In reply to Cameron McCormack (:heycam) from comment #18)
> Comment on attachment 8826467 [details]
> Bug 1330824 - Do not copy animation property over mAnimationXXCount.
> 
> https://reviewboard.mozilla.org/r/104432/#review105084
> 
> ::: servo/components/style/properties/gecko.mako.rs:1024
> (Diff revision 1)
> >          self.gecko.mAnimation${gecko_ffi_name}Count = other.gecko.mAnimation${gecko_ffi_name}Count;
> >          for (index, animation) in self.gecko.mAnimations.iter_mut().enumerate() {
> > +            // The length of mAnimations is often greater than mAnimationXXCount,
> > +            // don't copy values over the count.
> > +            if index >= self.gecko.mAnimation${gecko_ffi_name}Count as usize {
> > +                break;
> > +            }
> >              animation.m${gecko_ffi_name} = other.gecko.mAnimations[index].m${gecko_ffi_name};
> >          }
> 
> If you want to be a bit shorter, you can use take():
> 
> let count = other.gecko.mAnimation${gecko_ffi_name}Count;
> self.gecko.mAnimation${gecko_ffi_name}Count = count;
> 
> for (index, animation) in
> self.gecko.mAnimations.iter_mut().enumerate().take(count) {
>   animation.m${gecko_ffi_name} = ...
> }

Wow! I thought there must be a good way to limit iterator, but could not find it. Thanks!

Comment 20

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8826464 [details]
Bug 1330824 - Add truncate for nsAString and nsACString.

https://reviewboard.mozilla.org/r/104426/#review105338

I'd like this to be a safe method and also avaliable on nsACString, then I'll look at it again :). Thanks!

::: servo/components/style/gecko_bindings/nsstring_vendor/src/lib.rs:679
(Diff revision 1)
>              Gecko_AppendUTF8toString(self as *mut _, other as *const _);
>          }
>      }
> +
> +    pub unsafe fn truncate(&mut self) {
> +        Gecko_TruncateString(self);

Truncate should be safe to call, please rewrite this as:

pub fn truncate(&mut self) {
    unsafe {
        Gecko_TruncateString(self);
    }
}

::: servo/components/style/gecko_bindings/nsstring_vendor/src/lib.rs:738
(Diff revision 1)
>      fn Gecko_AppendCString(this: *mut nsACString, other: *const nsACString);
>  
>      fn Gecko_FinalizeString(this: *mut nsAString);
>      fn Gecko_AssignString(this: *mut nsAString, other: *const nsAString);
>      fn Gecko_AppendString(this: *mut nsAString, other: *const nsAString);
> +    fn Gecko_TruncateString(this: *mut nsAString);

Please also add Gecko_TruncateCString and the corresponding truncate() methods on nsACString.

Comment 21

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8826463 [details]
Bug 1330824 - Use impl_copy_animation_value for animation-timing-function.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104424/diff/1-2/

Comment 22

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8826464 [details]
Bug 1330824 - Add truncate for nsAString and nsACString.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104426/diff/1-2/

Comment 23

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8826465 [details]
Bug 1330824 - Add IndexMut for nsStyleAutoArray.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104428/diff/1-2/

Comment 24

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8826466 [details]
Bug 1330824 - Do not set mAnimationNameCount less than 1.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104430/diff/1-2/

Comment 25

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8826467 [details]
Bug 1330824 - Do not copy animation property over mAnimationXXCount.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/104432/diff/1-2/

Comment 26

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8826464 [details]
Bug 1330824 - Add truncate for nsAString and nsACString.

https://reviewboard.mozilla.org/r/104426/#review105406

Looking forward to having servo in m-c so we can get rid of nsstring_vendor, but looks good to me!

Comment 27

[Hiroyuki Ikezoe (:hiro)]

Thanks!

Servo side PR:
https://github.com/servo/servo/pull/15022

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9408aa59a92
Add truncate for nsAString and nsACString. r=mystor
https://hg.mozilla.org/integration/mozilla-inbound/rev/9735ab23b943
Drop skip-if from test cases that failed due to animation property handling for stylo. r=heycam

Comment 29

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/f9408aa59a92
https://hg.mozilla.org/mozilla-central/rev/9735ab23b943