This bug was filed from the Socorro interface and is report bp-d5ef4bab-f8c7-411b-b8be-7b5960171116. ============================================================= Seen while looking at nightly crash stats: http://bit.ly/2im8CRp. 215 crashes/38 installs. Crashes started using 20171116100106 Crash is reproducible for me on latest mac nightly using http://bit.ly/2zONFGE Top 10 frames of crashing thread: 0 xul.dll std::panicking::rust_panic_with_hook src/libstd/panicking.rs:617 1 xul.dll std::panicking::begin_panic<alloc::string::String> src/libstd/panicking.rs:572 2 xul.dll std::panicking::begin_panic_fmt src/libstd/panicking.rs:522 3 xul.dll core::panicking::panic_fmt src/libcore/panicking.rs:70 4 xul.dll core::option::expect_failed src/libcore/option.rs:819 5 xul.dll geckoservo::glue::Servo_StyleSet_GetBaseComputedValuesForElement servo/ports/geckolib/glue.rs:840 6 xul.dll mozilla::ServoStyleSet::GetBaseContextForElement layout/style/ServoStyleSet.cpp:1255 7 xul.dll mozilla::dom::KeyframeEffectReadOnly::EnsureBaseStyles dom/animation/KeyframeEffectReadOnly.cpp:534 8 xul.dll mozilla::dom::KeyframeEffectReadOnly::DoUpdateProperties<mozilla::ServoStyleContext const > dom/animation/KeyframeEffectReadOnly.cpp:351 9 xul.dll mozilla::EffectCompositor::UpdateEffectProperties<mozilla::ServoStyleContext const > dom/animation/EffectCompositor.cpp:383 =============================================================

Hiro, would you like to look at this one?

Oops, bug 1415013 is not related. The commit caused this crash is https://hg.mozilla.org/integration/autoland/rev/c04e7d6027d7

Yeah, so... That PR removed the check to skip traversing pseudo-elements that are going away, and what's causing the crash is us trying to update animations for a pseudo that just went away... I guess I can put back a simplified version of that check, but it feels kinda wallpaperish...

Actually, I changed my mind, and that's more wallpaper-ish than what I'd like. Hiro, ideally we can just return anything from that function when that doesn't exist, right? The right fix is passing the pseudo-element to that function instead of host element / pseudoType. Meanwhile I guess we can just not panic provided the pseudo is going away, and return the style that we get as an argument or what not... Hiro, you know the EffectCompositor code better than I do, what do you think? I'm happy to add the wallpaper, but if the animation code could be fixed that'd be even better.

Here is a test case that causes this crash. The crash happens when setting display:none for the animating pseudo element. (In reply to Emilio Cobos Álvarez [:emilio] from comment #4) > Actually, I changed my mind, and that's more wallpaper-ish than what I'd > like. > > Hiro, ideally we can just return anything from that function when that > doesn't exist, right? Yes, we can. Apart from it, we try to update (recreate?) CSS animations in display:none subtree, we should avoid it.

This is the #1 non-ShutdownKill topcrash in Nightly 20171117100127, with 503 occurrences, which is extremely high for Nightly. Please fix ASAP!

Comment on attachment 8929973 [details] Bug 1418059 - Stop eagerly CSS animations on the root of display:none subtree. https://reviewboard.mozilla.org/r/201140/#review206236 I presume we don't need to test the case for when a grandparent goes display:none since we only store style data on the root nodes of display:none subtrees? ::: layout/style/nsAnimationManager.cpp:1071 (Diff revision 1) > if (!aStyleContext) { > // If we are in a display:none subtree we will have no computed values. > // Since CSS animations should not run in display:none subtrees we should > // stop (actually, destroy) any animations on this element here. > StopAnimationsForElement(aElement, aPseudoType); > return; > } > > + const nsStyleDisplay *disp = > + aStyleContext->ComputedData()->GetStyleDisplay(); > + if (disp->mDisplay == StyleDisplay::None) { > + // Check display style and stop animations in the case of display:none since > + // the servo computed data has not been cleared at this moment in the case > + // where the target element itself has 'display: none'. > + StopAnimationsForElement(aElement, aPseudoType); > + return; > + } Looks good, but perhaps we could combine these two early returns like so: const nsStyleDisplay* disp = aStyleContext ? aStyleContext->ComputedData()->GetStyleDisplay() : nullptr; if (!disp || disp->mDisplay == StyleDisplay::None) { // If we are in a display:none subtree we will have no computed values. // However, if we are a pseudo-element that is about to go away // because our parent is now display:none, the computed values // might not have been cleared yet. // In either case, since CSS animations should not run in display:none // subtrees we should stop (actually, destroy) any animations on this // element here. StopAnimationsForElement(aElement, aPseudoType); return; } (You'll need to check the comment makes sense since I haven't followed this bug closely. Specifically, will the computed data actually go away? Or is this just due to the fact that we store style data on the root node of display:none subtrees?)

(In reply to Brian Birtles (:birtles) from comment #9) > Comment on attachment 8929973 [details] > Bug 1418059 - Stop eagerly CSS animations on elements which has display:none > style. > > https://reviewboard.mozilla.org/r/201140/#review206236 > > I presume we don't need to test the case for when a grandparent goes > display:none since we only store style data on the root nodes of > display:none subtrees? Yep, exactly. > ::: layout/style/nsAnimationManager.cpp:1071 > (Diff revision 1) > > if (!aStyleContext) { > > // If we are in a display:none subtree we will have no computed values. > > // Since CSS animations should not run in display:none subtrees we should > > // stop (actually, destroy) any animations on this element here. > > StopAnimationsForElement(aElement, aPseudoType); > > return; > > } > > > > + const nsStyleDisplay *disp = > > + aStyleContext->ComputedData()->GetStyleDisplay(); > > + if (disp->mDisplay == StyleDisplay::None) { > > + // Check display style and stop animations in the case of display:none since > > + // the servo computed data has not been cleared at this moment in the case > > + // where the target element itself has 'display: none'. > > + StopAnimationsForElement(aElement, aPseudoType); > > + return; > > + } > > Looks good, but perhaps we could combine these two early returns like so: > > const nsStyleDisplay* disp = aStyleContext > ? aStyleContext->ComputedData()->GetStyleDisplay() > : nullptr; > > if (!disp || disp->mDisplay == StyleDisplay::None) { > // If we are in a display:none subtree we will have no computed values. > // However, if we are a pseudo-element that is about to go away > // because our parent is now display:none, the computed values > // might not have been cleared yet. > // In either case, since CSS animations should not run in display:none > // subtrees we should stop (actually, destroy) any animations on this > // element here. > StopAnimationsForElement(aElement, aPseudoType); > return; > } > > (You'll need to check the comment makes sense since I haven't followed this > bug closely. Specifically, will the computed data actually go away? Or is > this just due to the fact that we store style data on the root node of > display:none subtrees?) Yep, the latter is right. For references, we do clear servo style data for descendant elements in display:none subtree in traversal, whereas we do preserve servo style data for element which has 'display:none' in traversal, since the style data is sometimes used for something. An example that I know is, display:none canvas element for double buffering. Thanks for the comment, I will modify it.

I though the arrangement was that if we have: a (display:block) | +->b (display:none) | +-> c (display:none) We store style data only on 'a' and 'b', not 'c' and the reason we store it on 'b' is so we can detect changes from 'none' to 'block' etc. That is, we only store it on the _root_ of a display:none subtree. Is that right?

(In reply to Brian Birtles (:birtles) from comment #11) > I though the arrangement was that if we have: > > a (display:block) > | > +->b (display:none) > | > +-> c (display:none) > > We store style data only on 'a' and 'b', not 'c' and the reason we store it > on 'b' is so we can detect changes from 'none' to 'block' etc. That is, we > only store it on the _root_ of a display:none subtree. Oh right. I haven't thought out the reason, but it makes totally sense.

Pushed by hikezoe@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/3ccf0877126d Stop eagerly CSS animations on the root of display:none subtree. r=birtles

(In reply to Pulsebot from comment #14) > Pushed by hikezoe@mozilla.com: > https://hg.mozilla.org/integration/autoland/rev/3ccf0877126d > Stop eagerly CSS animations on the root of display:none subtree. r=birtles Hiro, does your patch also fix the content: none case? If I replace in your crashtest "display: none" by "content: none", it also crashes, as expected, so I suspect we also need to check StyleContent()->ContentCount(). I doubt your patch fixes it but ICBW.

Oh, I did not notice changing content value to 'none' also causes this crash. The patches for bug 1418867 will fix the case but we need to figure out how to avoid update keyframe data for the content:none case. I will open a new bug for it. Thanks for the notice.

There still are 403 crashes with signature 'core::option::expect_failed | geckoservo::glue::Servo_StyleSet_GetBaseComputedValuesForElement' and 143 with 'mozalloc_abort | abort | core::option::expect_failed | geckoservo::glue::Servo_StyleSet_GetBaseComputedValuesForElement' after the patch hash landed (see [1]). For information, a lot of crashes are related to url: www.gog.com. [1] http://bit.ly/2mQj2gX

Those crashes should be fixed by bug 1418867. If there are still, please open a new bug. Thanks!