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

--HG--
extra : moz-landing-system : lando

dom/base/FragmentOrElement.cpp

@@ -1323,11 +1323,12 @@ NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(FragmentOrElement)
       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]);
       }
     }
   }
@@ -1838,14 +1839,14 @@ NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INTERNAL(FragmentOrElement)
             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);
         }
       }
     }

testing/web-platform/tests/web-animations/interfaces/Animatable/getAnimations.html

@@ -41,6 +41,18 @@ test(t => {
   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);

testing/web-platform/tests/web-animations/interfaces/KeyframeEffect/target.html

@@ -85,5 +85,25 @@ test(t => {
                 '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>