The attached testcases causes an assertion in m-c rev 20170901-a3585c77e2b1 with stylo enabled by pref. Assertion failure: !mPresContext->RestyleManager()->IsInStyleRefresh(), at /builds/worker/workspace/build/src/dom/animation/EffectCompositor.cpp:337 #0: mozilla::EffectCompositor::PostRestyleForAnimation, at dom/animation/EffectCompositor.cpp:337 #1: mozilla::EffectCompositor::RequestRestyle, at dom/animation/EffectCompositor.cpp:287 #2: mozilla::dom::KeyframeEffectReadOnly::NotifyAnimationTimingUpdated, at dom/animation/KeyframeEffectReadOnly.cpp:142 #3: mozilla::dom::Animation::UpdateTiming, at dom/animation/Animation.cpp:1238 #4: mozilla::dom::Animation::CancelNoUpdate, at dom/animation/Animation.cpp:817 #5: mozilla::AnimationCollection<mozilla::dom::CSSTransition>::PropertyDtor, at dom/animation/Animation.h:149 #6: nsPropertyTable::PropertyList::DeletePropertyFor, at dom/base/nsPropertyTable.cpp:296 #7: nsPropertyTable::DeleteProperty, at dom/base/nsPropertyTable.cpp:232 #8: nsINode::DeleteProperty, at dom/base/nsINode.cpp:195 #9: mozilla::dom::Element::UnbindFromTree, at dom/base/Element.cpp:1898 #10: nsGenericHTMLElement::UnbindFromTree, at dom/html/nsGenericHTMLElement.cpp:537 #11: nsFrameManager::DestroyAnonymousContent, at layout/base/nsFrameManager.cpp:695 #12: nsIFrame::DestroyAnonymousContent, at layout/generic/nsFrame.cpp:247 #13: nsMeterFrame::DestroyFrom, at layout/forms/nsMeterFrame.cpp:57 #14: nsBlockFrame::DoRemoveFrame, at layout/generic/nsBlockFrame.cpp:6014 #15: nsBlockFrame::RemoveFrame, at layout/generic/nsBlockFrame.cpp:5327 #16: nsFrameManager::RemoveFrame, at layout/base/nsFrameManager.cpp:536 #17: nsCSSFrameConstructor::ContentRemoved, at layout/base/nsCSSFrameConstructor.cpp:8908 #18: nsCSSFrameConstructor::RecreateFramesForContent, at layout/base/nsCSSFrameConstructor.cpp:10098 #19: nsCSSFrameConstructor::RecreateFramesForContent, at layout/base/nsCSSFrameConstructor.cpp:10072 #20: mozilla::RestyleManager::ProcessRestyledFrames, at layout/base/RestyleManager.cpp:1509 #21: mozilla::ServoRestyleManager::DoProcessPendingRestyles, at layout/base/ServoRestyleManager.cpp:1136

So this is a regression in bug 1393791, which causes us to synchronously invoke UnbindFromTree on native-anonymous content during frame teardown. In general, there's no reason we should be posting restyles on this stuff, since it's being unbound from the tree. I can see various ways to fix this, but I'll defer to the animation folks on what makes the most sense.

(Just curious, does this also repro without stylo?)

(In reply to Emilio Cobos Álvarez [:emilio] from comment #3) > (Just curious, does this also repro without stylo?) It does not appear to repro with Stylo disabled, in my macOS Stylo build. I confirmed it DID repro with Stylo enabled.

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #2) > So this is a regression in bug 1393791, which causes us to synchronously > invoke UnbindFromTree on native-anonymous content during frame teardown. > > In general, there's no reason we should be posting restyles on this stuff, > since it's being unbound from the tree. That's right. I think we should check that the element is associated with a document in EffectCompositor::RequestRestyle(). To do this, ClearInDocument() has to be moved right before DeleteProperty() sets in UnbindFromTree(). I think it should work, let's see what happens. https://treeherder.mozilla.org/#/jobs?repo=try&revision=673444fe3a48113ed71b62761973e7371e42263f Brian might have different ideas, so I'd like to defer him how to solve this eventually.

Forgot to run tests on gecko. https://treeherder.mozilla.org/#/jobs?repo=try&revision=04e3d9b55876e2cb519dd68b7b26fa21f7f361bf

(In reply to Hiroyuki Ikezoe (:hiro) from comment #5) > (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #2) > > So this is a regression in bug 1393791, which causes us to synchronously > > invoke UnbindFromTree on native-anonymous content during frame teardown. > > > > In general, there's no reason we should be posting restyles on this stuff, > > since it's being unbound from the tree. > > That's right. I think we should check that the element is associated with a > document in EffectCompositor::RequestRestyle(). EffectCompositor::RequestRestyle already checks that (in the call to nsComputedDOMStyle::GetPresShellForContent). > To do this, > ClearInDocument() has to be moved right before DeleteProperty() sets in > UnbindFromTree(). Yes, that part will need to change. > I think it should work, let's see what happens. > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=673444fe3a48113ed71b62761973e7371e42263f > > Brian might have different ideas, so I'd like to defer him how to solve this > eventually. I think the ClearInDocument changes are good. I'd prefer to drop the changes to EffectCompositor::PostRestyleForAnimation unless we have a test case that needs it.

(In reply to Brian Birtles (:birtles) from comment #7) > (In reply to Hiroyuki Ikezoe (:hiro) from comment #5) > > (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #2) > > > So this is a regression in bug 1393791, which causes us to synchronously > > > invoke UnbindFromTree on native-anonymous content during frame teardown. > > > > > > In general, there's no reason we should be posting restyles on this stuff, > > > since it's being unbound from the tree. > > > > That's right. I think we should check that the element is associated with a > > document in EffectCompositor::RequestRestyle(). > > EffectCompositor::RequestRestyle already checks that (in the call to > nsComputedDOMStyle::GetPresShellForContent). Oh right. I missed the part. Thanks!

The try looks good (all known oranges).

Comment on attachment 8904102 [details] Bug 1396041 - Disassociate element from document before destroying animations in UnbindFromTree(). https://reviewboard.mozilla.org/r/175872/#review180846 ::: commit-message-8e052:1 (Diff revision 1) > +Bug 1396041 - Disaccosiate element from document before destroying animations in UnbindFromTree(). r?birtles Disassociate ::: commit-message-8e052:3 (Diff revision 1) > +So that we can avoid unnecessary restyle request during destroying animations > +for such orphaned elements. This allows us to avoid posting animation-related restyles when removing elements from the document tree. ::: layout/style/crashtests/crashtests.list:213 (Diff revision 1) > +load 1396041.html > load 1395725.html Let's put it at the end so at least the order is locally sorted.

Thanks for the review!

Pushed by hikezoe@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/5e4c2725acf2 Disassociate element from document before destroying animations in UnbindFromTree(). r=birtles