+++ This bug was initially created as a clone of Bug #1418059 +++ This bug was filed from the Socorro interface and is report bp-d5ef4bab-f8c7-411b-b8be-7b5960171116. ============================================================= I am going to fix CSS animation cases in bug 1418059. Most crashes in the wild should be fixed by bug 1418059. Apart form that I am going to fix script animations cases in this bug.

I believe this is not a topcrash, the topcrash one is bug 1418059. After landing bug 1418059, there is only one crash report (bp-5302afea-6f3d-44ec-89b6-cd47d0171120). I believe the one was reported by Emilio with a crash test similar to the one in comment 0. Use comment in the crash report is 'lol, that crashtest works :O'. :)

(In reply to Hiroyuki Ikezoe (:hiro) from comment #3) > I believe this is not a topcrash, the topcrash one is bug 1418059. After > landing bug 1418059, there is only one crash report > (bp-5302afea-6f3d-44ec-89b6-cd47d0171120). I believe the one was reported > by Emilio with a crash test similar to the one in comment 0. Use comment in > the crash report is 'lol, that crashtest works :O'. :) Oops, the test case should look like the one in bug 1418902 comment 1.

A new try based on bug 1419221. https://treeherder.mozilla.org/#/jobs?repo=try&revision=95abb36b2c654abdd6b9ffe3bd94492062eb0be2

Comment on attachment 8930400 [details] Bug 1418867 - Pass element or pseudo element to Servo_StyleSet_GetBaseComputedValuesForElement(). https://reviewboard.mozilla.org/r/201554/#review206808 ::: dom/animation/KeyframeEffectReadOnly.cpp:563 (Diff revision 1) > if (!hasAdditiveValues) { > return; > } > > if (!aBaseStyleContext) { > + Element* animatingElement = Just curious, is there a bug on file to remove the `mPseudoElement` from the animation target and just use the properties on them when the old style system is gone? We should just avoid recreating them each time we reframe, I'd think. ::: servo/ports/geckolib/glue.rs:809 (Diff revision 1) > return computed_values.clone_arc().into(); > } > > let element = GeckoElement(element); > > - let element_data = match element.borrow_data() { > + if let Some(pseudo) = element.implemented_pseudo_element() { Let's just remove this whole block, I think, to make everything more consistent.

Comment on attachment 8930401 [details] Bug 1418867 - Fall back to re-resolve style if the parent element has no style data for the given pseudo element. https://reviewboard.mozilla.org/r/201556/#review206812 r=me even though this patch is not needed with the comments addressed of the first. If you disagree re. being worth optimising (accounting that this makes slower the non-pseudo case), you can land this with r=me, using `pseudo_element_originating_element` instead of `traversal_parent`. But again, I really think it's not worth the churn :) ::: servo/ports/geckolib/glue.rs:809 (Diff revision 1) > return computed_values.clone_arc().into(); > } > > let element = GeckoElement(element); > > if let Some(pseudo) = element.implemented_pseudo_element() { This is not needed if you actually remove the block :) I honestly don't think it's worth optimizing for this case, this function isn't even _that_ hot I'd think.

Comment on attachment 8930402 [details] Bug 1418867 - Crash test for the case where the parent element has no style data for pseudo. https://reviewboard.mozilla.org/r/201558/#review206818 (I'm assuming the commit message was missing an `r` :)) ::: layout/style/crashtests/1418867.html:8 (Diff revision 1) > +<style> > +@keyframes anim { > + to { transform: rotate(360deg); } > +} > +.document-ready div::after { > + display: none; Can you also add a test for the `content: none` case from yesterday's test?

(In reply to Emilio Cobos Álvarez [:emilio] from comment #12) > Comment on attachment 8930401 [details] > Bug 1418867 - Fall back to re-resolve style if the parent element has no > style data for the given pseudo element. > > https://reviewboard.mozilla.org/r/201556/#review206812 > > r=me even though this patch is not needed with the comments addressed of the > first. > > If you disagree re. being worth optimising (accounting that this makes > slower the non-pseudo case), you can land this with r=me, using > `pseudo_element_originating_element` instead of `traversal_parent`. > > But again, I really think it's not worth the churn :) OK, if you are OK, I will drop the code, honestly I like this optimization though. (When I first saw the code I didn't understand what the purpose is, but after I realized the the optimization I was impressed with the gimmick.) (In reply to Emilio Cobos Álvarez [:emilio] from comment #13) > Comment on attachment 8930402 [details] > Bug 1418867 - Crash test for the case where the parent element has no style > data for pseudo. ?emilio > > https://reviewboard.mozilla.org/r/201558/#review206818 > > (I'm assuming the commit message was missing an `r` :)) > > ::: layout/style/crashtests/1418867.html:8 > (Diff revision 1) > > +<style> > > +@keyframes anim { > > + to { transform: rotate(360deg); } > > +} > > +.document-ready div::after { > > + display: none; > > Can you also add a test for the `content: none` case from yesterday's test? I will add the test case in bug 1418902.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #14) > OK, if you are OK, I will drop the code, honestly I like this optimization > though. (When I first saw the code I didn't understand what the purpose is, > but after I realized the the optimization I was impressed with the gimmick.) Just for the record, I just added it because the alternative was hard (since I didn't have the pseudo-element handy, so I'd have to add another special path to push_applicable_declarations or what not, etc...)

Comment on attachment 8930398 [details] Bug 1418867 - Drop pseudo type argument from KeyframeEffectReadOnly::EnsureBaseStyle(). https://reviewboard.mozilla.org/r/201550/#review207150

Comment on attachment 8930399 [details] Bug 1418867 - getUnanimatedComputedStyle throws an exception for non-existent pseudo element. https://reviewboard.mozilla.org/r/201552/#review207154 ::: dom/base/test/file_domwindowutils_animation.html:133 (Diff revision 1) > > if (utils.isStyledByServo) { > + SimpleTest.doesThrow( > + () => utils.getUnanimatedComputedStyle(div, "::before", "opacity", utils.FLUSH_NONE), > + "NS_ERROR_FAILURE", > + "Inexistent pseudo should throw"); Nit: Non-existent

The test case sometimes causes an assertion on old style system. I am going to skip the test on old style system here. Filed bug 1419641 for the assertion.

Comment on attachment 8930400 [details] Bug 1418867 - Pass element or pseudo element to Servo_StyleSet_GetBaseComputedValuesForElement(). https://reviewboard.mozilla.org/r/201554/#review206808 > Just curious, is there a bug on file to remove the `mPseudoElement` from the animation target and just use the properties on them when the old style system is gone? > > We should just avoid recreating them each time we reframe, I'd think. Filed bug 1419651 now.

Pushed by hikezoe@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/4868b9f25718 Drop pseudo type argument from KeyframeEffectReadOnly::EnsureBaseStyle(). r=birtles https://hg.mozilla.org/integration/autoland/rev/51ab3c3c9935 getUnanimatedComputedStyle throws an exception for non-existent pseudo element. r=birtles https://hg.mozilla.org/integration/autoland/rev/0fe9ca39473a Pass element or pseudo element to Servo_StyleSet_GetBaseComputedValuesForElement(). r=emilio https://hg.mozilla.org/integration/autoland/rev/00a08154f505 Crash test for the case where the parent element has no style data for pseudo. r=emilio

https://hg.mozilla.org/mozilla-central/rev/4868b9f25718 https://hg.mozilla.org/mozilla-central/rev/51ab3c3c9935 https://hg.mozilla.org/mozilla-central/rev/0fe9ca39473a https://hg.mozilla.org/mozilla-central/rev/00a08154f505