Bug 1200568 - Don't create animations for elements that are not part of the document tree; r=dbaron
authorBrian Birtles <birtles@gmail.com>
Wed, 09 Sep 2015 10:10:41 +0900
changeset 294062 66b14ac1d547d17894caac7c3732580258faea8a
parent 294061 b41a88070b1ba516c9a25028a58a9d5da2ca17b5
child 294063 82e6e2bfbf9c93f11756a8aaffaf111451036d42
push id5245
push userraliiev@mozilla.com
push dateThu, 29 Oct 2015 11:30:51 +0000
treeherdermozilla-beta@dac831dc1bd0 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdbaron
bugs1200568
milestone43.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1200568 - Don't create animations for elements that are not part of the document tree; r=dbaron 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.
layout/style/AnimationCommon.cpp
layout/style/crashtests/1200568-1.html
layout/style/crashtests/crashtests.list
layout/style/nsAnimationManager.cpp
--- a/layout/style/AnimationCommon.cpp
+++ b/layout/style/AnimationCommon.cpp
@@ -366,16 +366,21 @@ CommonAnimationManager::FlushAnimations(
 {
   TimeStamp now = mPresContext->RefreshDriver()->MostRecentRefresh();
   for (AnimationCollection* collection = mElementCollections.getFirst();
        collection; collection = collection->getNext()) {
     if (collection->mStyleRuleRefreshTime == now) {
       continue;
     }
 
+    MOZ_ASSERT(collection->mElement->GetComposedDoc() ==
+                 mPresContext->Document(),
+               "Should not have a transition/animations collection for an "
+               "element that is not part of the document tree");
+
     collection->RequestRestyle(AnimationCollection::RestyleType::Standard);
   }
 }
 
 nsIStyleRule*
 CommonAnimationManager::GetAnimationRule(mozilla::dom::Element* aElement,
                                          nsCSSPseudoElements::Type aPseudoType)
 {
@@ -878,20 +883,16 @@ AnimationCollection::RequestRestyle(Rest
              "Unexpected mElementProperty; might restyle too much");
 
   nsPresContext* presContext = mManager->PresContext();
   if (!presContext) {
     // Pres context will be null after the manager is disconnected.
     return;
   }
 
-  MOZ_ASSERT(mElement->GetCrossShadowCurrentDoc() == presContext->Document(),
-             "Element::UnbindFromTree should have destroyed the element "
-             "transition/animations object");
-
   // Steps for Restyle::Layer:
 
   if (aRestyleType == RestyleType::Layer) {
     mStyleRuleRefreshTime = TimeStamp();
     // FIXME: We should be able to remove these two lines once we move
     // ticking to animation timelines as part of bug 1151731.
     mNeedsRefreshes = true;
     mManager->MaybeStartObservingRefreshDriver();
new file mode 100644
--- /dev/null
+++ b/layout/style/crashtests/1200568-1.html
@@ -0,0 +1,16 @@
+<!doctype html>
+<html>
+<head>
+<style>
+.anim { animation: anim 2s infinite linear }
+@keyframes anim { }
+</style>
+</head>
+<body>
+<script>
+var i = document.createElement('i');
+i.setAttribute('class', 'anim');
+getComputedStyle(i).display;
+</script>
+</body>
+</html>
--- a/layout/style/crashtests/crashtests.list
+++ b/layout/style/crashtests/crashtests.list
@@ -115,10 +115,11 @@ pref(layout.css.expensive-style-struct-a
 pref(layout.css.expensive-style-struct-assertions.enabled,true) load 1146101-1.html
 load 1153693-1.html
 load 1161320-1.html
 pref(dom.animations-api.core.enabled,true) load 1161320-2.html
 load 1161366-1.html
 load 1163446-1.html
 load 1164813-1.html
 load 1167782-1.html
+load 1200568-1.html
 load large_border_image_width.html
 load border-image-visited-link.html
--- a/layout/style/nsAnimationManager.cpp
+++ b/layout/style/nsAnimationManager.cpp
@@ -366,18 +366,19 @@ nsAnimationManager::SizeOfIncludingThis(
 {
   return aMallocSizeOf(this) + SizeOfExcludingThis(aMallocSizeOf);
 }
 
 nsIStyleRule*
 nsAnimationManager::CheckAnimationRule(nsStyleContext* aStyleContext,
                                        mozilla::dom::Element* aElement)
 {
-  if (!mPresContext->IsDynamic()) {
-    // For print or print preview, ignore animations.
+  // Ignore animations for print or print preview, and for elements
+  // that are not attached to the document tree.
+  if (!mPresContext->IsDynamic() || !aElement->IsInComposedDoc()) {
     return nullptr;
   }
 
   // Everything that causes our animation data to change triggers a
   // style change, which in turn triggers a non-animation restyle.
   // Likewise, when we initially construct frames, we're not in a
   // style change, but also not in an animation restyle.