1200568 - Assertion failure: mElement->GetCrossShadowCurrentDoc() == presContext->Document() (Element::UnbindFromTree should have destroyed the element transition/animations object)

Status: RESOLVED FIXED

(Core :: DOM: Animation, defect)

Description

[Carsten Book [:Tomcat]]

Attachment: bughunter stack

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

Comment 1

[Carsten Book [:Tomcat]]

brian: i guess this is a regression from the checkins that happened some days ago. Could you take a look ? thanks!

Comment 2

[Brian Birtles (:birtles, Feb 26~ parental leave)]

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.

Comment 3

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Reduced test case

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.

Comment 4

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Further reduced test case

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.

Comment 5

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Don't create animations for elements that are not part of the document tree

I still need to work out why we don't encounter this for transitions.

Comment 6

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(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.

Comment 7

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: Don't create animations for elements that are not part of the document tree

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.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 9

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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.

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/66b14ac1d547

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/66b14ac1d547