Bug 1552387 - Traverse and unlink EffectSet properties on non-HTML/SVG elements too; r=hiro
authorBrian Birtles <birtles@gmail.com>
Fri, 17 May 2019 04:49:38 +0000
changeset 474299 eeb6dd82b7c1a9fc36610bfe1ccecdf93c356677
parent 474298 f41aae1abbbaaddf29087d312cb91fa25578b45b
child 474300 7fdd2abe918b6ba220dd097d27d1a2cda6cfa8e6
push id36027
push usershindli@mozilla.com
push dateFri, 17 May 2019 16:24:38 +0000
treeherdermozilla-central@c94c54aff466 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewershiro
bugs1552387
milestone68.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 1552387 - Traverse and unlink EffectSet properties on non-HTML/SVG elements too; r=hiro The tests added in this patch do not fail any of their assertions with or without the code changes in this patch. However, without the code changes in this patch they will both fail due to reported memory leaks. Differential Revision: https://phabricator.services.mozilla.com/D31577
dom/base/FragmentOrElement.cpp
testing/web-platform/tests/web-animations/interfaces/Animatable/getAnimations.html
testing/web-platform/tests/web-animations/interfaces/KeyframeEffect/target.html
--- a/dom/base/FragmentOrElement.cpp
+++ b/dom/base/FragmentOrElement.cpp
@@ -1318,21 +1318,22 @@ NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(Fr
     }
 
     if (tmp->IsHTMLElement() || tmp->IsSVGElement()) {
       nsStaticAtom* const* props =
           Element::HTMLSVGPropertiesToTraverseAndUnlink();
       for (uint32_t i = 0; props[i]; ++i) {
         tmp->DeleteProperty(props[i]);
       }
-      if (tmp->MayHaveAnimations()) {
-        nsAtom** effectProps = EffectSet::GetEffectSetPropertyAtoms();
-        for (uint32_t i = 0; effectProps[i]; ++i) {
-          tmp->DeleteProperty(effectProps[i]);
-        }
+    }
+
+    if (tmp->MayHaveAnimations()) {
+      nsAtom** effectProps = EffectSet::GetEffectSetPropertyAtoms();
+      for (uint32_t i = 0; effectProps[i]; ++i) {
+        tmp->DeleteProperty(effectProps[i]);
       }
     }
   }
 
   // Unlink child content (and unbind our subtree).
   if (tmp->UnoptimizableCCNode() || !nsCCUncollectableMarker::sGeneration) {
     // Don't allow script to run while we're unbinding everything.
     nsAutoScriptBlocker scriptBlocker;
@@ -1833,24 +1834,24 @@ NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_
     if (tmp->IsHTMLElement() || tmp->IsSVGElement()) {
       nsStaticAtom* const* props =
           Element::HTMLSVGPropertiesToTraverseAndUnlink();
       for (uint32_t i = 0; props[i]; ++i) {
         nsISupports* property =
             static_cast<nsISupports*>(tmp->GetProperty(props[i]));
         cb.NoteXPCOMChild(property);
       }
-      if (tmp->MayHaveAnimations()) {
-        nsAtom** effectProps = EffectSet::GetEffectSetPropertyAtoms();
-        for (uint32_t i = 0; effectProps[i]; ++i) {
-          EffectSet* effectSet =
-              static_cast<EffectSet*>(tmp->GetProperty(effectProps[i]));
-          if (effectSet) {
-            effectSet->Traverse(cb);
-          }
+    }
+    if (tmp->MayHaveAnimations()) {
+      nsAtom** effectProps = EffectSet::GetEffectSetPropertyAtoms();
+      for (uint32_t i = 0; effectProps[i]; ++i) {
+        EffectSet* effectSet =
+            static_cast<EffectSet*>(tmp->GetProperty(effectProps[i]));
+        if (effectSet) {
+          effectSet->Traverse(cb);
         }
       }
     }
   }
 
   // Traverse attribute names.
   if (tmp->IsElement()) {
     Element* element = tmp->AsElement();
--- a/testing/web-platform/tests/web-animations/interfaces/Animatable/getAnimations.html
+++ b/testing/web-platform/tests/web-animations/interfaces/Animatable/getAnimations.html
@@ -37,16 +37,28 @@ test(t => {
   const animationParent = divParent.animate(null, 100 * MS_PER_SEC);
   const animationChild = divChild.animate(null, 100 * MS_PER_SEC);
   assert_array_equals(divParent.getAnimations(), [animationParent],
                       'divParent');
   assert_array_equals(divChild.getAnimations(), [animationChild], 'divChild');
 }, 'Returns only the animations specific to each parent/child element');
 
 test(t => {
+  const foreignElement
+    = document.createElementNS('http://example.org/test', 'test');
+  document.body.appendChild(foreignElement);
+  t.add_cleanup(() => {
+    foreignElement.remove();
+  });
+
+  const animation = foreignElement.animate(null, 100 * MS_PER_SEC);
+  assert_array_equals(foreignElement.getAnimations(), [animation]);
+}, 'Returns animations for a foreign element');
+
+test(t => {
   const div = createDiv(t);
   const animation = div.animate(null, 100 * MS_PER_SEC);
   animation.finish();
   assert_array_equals(div.getAnimations(), []);
 }, 'Does not return finished animations that do not fill forwards');
 
 test(t => {
   const div = createDiv(t);
--- a/testing/web-platform/tests/web-animations/interfaces/KeyframeEffect/target.html
+++ b/testing/web-platform/tests/web-animations/interfaces/KeyframeEffect/target.html
@@ -80,10 +80,30 @@ test(t => {
   // This makes sure the animation property is changed correctly on new
   // targeted element.
   anim.currentTime = 75 * MS_PER_SEC;
   assert_equals(getComputedStyle(b).marginLeft, '75px',
                 'Value of 2nd target (currently targeted) after ' +
                 'changing the animation current time.');
 }, 'Test setting target from a valid target to another target');
 
+promise_test(async t => {
+  const animation = createDiv(t).animate(
+    { opacity: 0 },
+    { duration: 1, fill: 'forwards' }
+  );
+
+  const foreignElement
+    = document.createElementNS('http://example.org/test', 'test');
+  document.body.appendChild(foreignElement);
+  t.add_cleanup(() => {
+    foreignElement.remove();
+  });
+
+  animation.effect.target = foreignElement;
+
+  // Wait a frame to make sure nothing bad happens when the UA tries to update
+  // style.
+  await waitForNextFrame();
+}, 'Target element can be set to a foreign element');
+
 </script>
 </body>