Closed Bug 1330824 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: xidorn, Assigned: hiro)

References

Details

Attachments

(5 files)

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)
Hiro, it seems you are actively working on animation support for stylo, could you have a look at this crash?
Flags: needinfo?(hikezoe)
Sure, I will look into this.
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
Flags: needinfo?(hikezoe)
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
Summary: stylo: test_compute_data_with_start_struct.html fatally asserts with "Assertion failure: display->mAnimationNameCount > 0 (first item must be explicit)" → stylo: several style system tests fatally asserts with "Assertion failure: display->mAnimationNameCount > 0 (first item must be explicit)"
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.
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.
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
(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.
(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 on attachment 8826463 [details]
Bug 1330824 - Use impl_copy_animation_value for animation-timing-function.

https://reviewboard.mozilla.org/r/104424/#review105068
Attachment #8826463 - Flags: review?(cam) → review+
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 on attachment 8826465 [details]
Bug 1330824 - Add IndexMut for nsStyleAutoArray.

https://reviewboard.mozilla.org/r/104428/#review105080
Attachment #8826465 - Flags: review?(cam) → review+
Attachment #8826464 - Flags: review?(michael)
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 "#".
Attachment #8826466 - Flags: review?(cam) → review+
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.
Attachment #8826467 - Flags: review?(cam) → review+
Attachment #8826464 - Flags: review?(cam)
(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 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.
Attachment #8826464 - Flags: review?(michael) → review-
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!
Attachment #8826464 - Flags: review?(michael) → review+
Thanks!

Servo side PR:
https://github.com/servo/servo/pull/15022
Pushed by hiikezoe@mozilla-japan.org:
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
https://hg.mozilla.org/mozilla-central/rev/f9408aa59a92
https://hg.mozilla.org/mozilla-central/rev/9735ab23b943
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Blocks: 1330825
You need to log in before you can comment on or make changes to this bug.