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

RESOLVED FIXED in Firefox 43

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: cbook, Assigned: birtles)

Tracking

(Blocks 1 bug, {assertion, regression})

unspecified
mozilla43
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox43 fixed)

Details

()

Attachments

(3 attachments, 2 obsolete attachments)

Posted file 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
brian: i guess this is a regression from the checkins that happened some days ago. Could you take a look ? thanks!
Flags: needinfo?(bbirtles)
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
Blocks: 1188251
Status: NEW → ASSIGNED
Flags: needinfo?(bbirtles)
Posted file Reduced test case (obsolete) —
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.
Flags: needinfo?(bzbarsky)
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.
Flags: needinfo?(bzbarsky)
(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.
https://hg.mozilla.org/mozilla-central/rev/66b14ac1d547
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.