Found via bughunter on sites like: http://www.sunnycars.de Steps to reproduce: --> Load http://www.sunnycars.de ---> Assertion failure: mElement->GetCrossShadowCurrentDoc() == presContext->Document() (Element::UnbindF romTree should have destroyed the element transition/animations object), at c:/Users/mozilla/debug-builds/mozilla-central/layout/style/AnimationCommon.cpp:948
brian: i guess this is a regression from the checkins that happened some days ago. Could you take a look ? thanks!
Appears to be from: https://hg.mozilla.org/mozilla-central/rev/754717f00c5e which was simply moving assertions around. So I guess we are evaluating this assertion in different circumstances now.
Assignee: nobody → bbirtles
Status: NEW → ASSIGNED
I finally managed to reduce this to a minimal test case. The problem is basically that we're creating an element that is not part of the document and putting animations on it. The assertion in question tests that animations are removed when the element is removed from the document tree but in this case we never actually add the element to the tree. This assertion used to be ok because it only covered transitions and this situation shouldn't occur for transitions. As of bug 1188251, however, we now apply this assertion to animations too where it can occur.
Further simplified. The trigger is that we call getComputedStyle on the element which causes us to add animations to it, despite the fact it is not part of the document. Seeing as we deliberately do not create animations when the element is part of a display:none subtree, it seems like we should do the same when the element is not part of the document.
Attachment #8655824 - Attachment is obsolete: true
I still need to work out why we don't encounter this for transitions.
(In reply to Brian Birtles (:birtles) from comment #5) > I still need to work out why we don't encounter this for transitions. For transitions, we only call TryStartingTransition when we have a frame (i.e. from ElementRestyler::RestyleSelf or CSSFrameConstructor) so we shouldn't end up generating transitions for an element that is not part of the document tree. I've also tested that this is the case.
As well as ensuring that we don't create animations for elements that are not part of the document tree, this test also adjusts the assertion that checks this in the following ways: * Calls GetComposedDoc() instead of GetCrossShadowCurrentDoc() since the latter is deprecated. * Moves it from RequestRestyle to FlushAnimations since, depending on how we refactor this code in the future, it's possible we might end up calling RequestRestyle even for animations on elements that have been removed from the document but we shouldn't call FlushAnimations in this case.
Attachment #8656343 - Flags: review?(dbaron)
Attachment #8655847 - Attachment is obsolete: true
Comment on attachment 8656343 [details] [diff] [review] Don't create animations for elements that are not part of the document tree This seems like an improvement, although I'm not sure it's yet completely correct. We currently remove animations in UnbindFromTree. Does UnbindFromTree correspond to when IsInComposedDoc changes between true and false? If not, it seems worth aligning where we do the removal with what we test here one way or another.
Attachment #8656343 - Flags: review?(dbaron) → review+
> Does UnbindFromTree correspond to when IsInComposedDoc changes between true and false? It will certainly be called when IsInComposedDoc goes from true to false. It can be called in other cases when IsInComposedDoc is already false too. After UnbindFromTree, IsInComposedDoc can never be true until a BindToTree happens.
(In reply to Boris Zbarsky [:bz] from comment #9) > > Does UnbindFromTree correspond to when IsInComposedDoc changes between true and false? > > It will certainly be called when IsInComposedDoc goes from true to false. Yeah, that's actually the guarantee we need, so this sounds good.
You need to log in before you can comment on or make changes to this bug.