High firefox Logic Error 🔧 Commit mapped

Overview

High
Severity
—
CVSS
No
Exploited ITW
Fixed
Fix Status
Impacthigh
DescriptionUse-after-free in the DOM: Animation component
ComponentLayout
Bug ClassLogic Error
Tracker2074625
Fix commitd4dd520d145b (firefox) +75/-16
CISA KEVNot listed
CreditedAmy Burnett of OpenAI
Disclosed2026-10-13

Changed Functions

FunctionChangeNotes
if
layout/style/nsAnimationManager.cpp
modified

Files Changed

  • layout/style/nsAnimationManager.cpp
  • servo/components/style/rule_tree/core.rs
  • testing/web-platform/tests/web-animations/crashtests/keyframe-effect-cross-doc-target.html
diff --git a/layout/style/nsAnimationManager.cpp b/layout/style/nsAnimationManager.cpp
index 76f59f235c0..e52f9741967 100644
--- a/layout/style/nsAnimationManager.cpp
+++ b/layout/style/nsAnimationManager.cpp
@@ -81,6 +81,8 @@ class MOZ_STACK_CLASS ServoCSSAnimationBuilder final {
     MOZ_ASSERT(aComputedStyle);
   }
 
+  const ComputedStyle* Style() const { return mComputedStyle; }
+
   bool BuildKeyframes(const Element& aElement, nsPresContext* aPresContext,
                       nsAtom* aName,
                       const StyleComputedTimingFunction& aTimingFunction,
@@ -88,12 +90,6 @@ class MOZ_STACK_CLASS ServoCSSAnimationBuilder final {
     return aPresContext->StyleSet()->GetKeyframesForName(
         aElement, *mComputedStyle, aName, aTimingFunction, aKeyframes);
   }
-  void SetKeyframes(KeyframeEffect& aEffect, nsTArray<Keyframe>&& aKeyframes,
-                    const dom::AnimationTimeline* aTimeline,
-                    const dom::AnimationRange& aRange) {
-    aEffect.SetKeyframes(std::move(aKeyframes), mComputedStyle, aTimeline,
-                         &aRange);
-  }
 
   // Currently all the animation building code in this file is based on
   // assumption that creating and removing animations should *not* trigger
@@ -178,7 +174,8 @@ static void UpdateOldAnimationPropertiesWithNew(
     CSSAnimation& aOld, TimingParams&& aNewTiming,
     nsTArray<Keyframe>&& aNewKeyframes, bool aNewIsStylePaused,
     CSSAnimationProperties aOverriddenProperties,
-    ServoCSSAnimationBuilder& aBuilder, dom::AnimationTimeline* aTimeline,
+    ServoCSSAnimationBuilder& aBuilder, const NonOwningAnimationTarget& aTarget,
+    dom::AnimationTimeline* aTimeline,
     const dom::ScopedTimelineName& aTimelineName,
     const StyleComputedTimingFunction& aNewTimingFunction,
     dom::CompositeOperation aNewComposite, dom::AnimationRange&& aTimelineRange,
@@ -196,9 +193,7 @@ static void UpdateOldAnimationPropertiesWithNew(
 
   // Update the old from the new so we can keep the original object
   // identity (and any expando properties attached to it).
-  if (aOld.GetEffect()) {
-    dom::AnimationEffect* oldEffect = aOld.GetEffect();
-
+  if (dom::AnimationEffect* oldEffect = aOld.GetEffect()) {
     // Copy across the changes that are not overridden
     TimingParams updatedTiming = oldEffect->SpecifiedTiming();
     if (~aOverriddenProperties & CSSAnimationProperties::Duration) {
@@ -222,8 +217,13 @@ static void UpdateOldAnimationPropertiesWithNew(
 
     if (KeyframeEffect* oldKeyframeEffect = oldEffect->AsKeyframeEffect()) {
       if (~aOverriddenProperties & CSSAnimationProperties::Keyframes) {
-        aBuilder.SetKeyframes(*oldKeyframeEffect, std::move(aNewKeyframes),
-                              aTimeline, aTimelineRange);
+        // FIXME(emilio): Should we update the effect target on top if different
+        // or something?
+        auto* style = oldKeyframeEffect->GetAnimationTarget() == aTarget
+                          ? aBuilder.Style()
+                          : nullptr;
+        oldKeyframeEffect->SetKeyframes(std::move(aNewKeyframes), style,
+                                        aTimeline, &aTimelineRange);
 
         // The default timing function and default composite for CSS Keyframes
         // processing.
@@ -441,7 +441,7 @@ static already_AddRefed<CSSAnimation> BuildAnimation(
     // In order to honor what the spec said, we'd copy more data over.
     UpdateOldAnimationPropertiesWithNew(
         *oldAnim, std::move(timing), std::move(keyframes), isStylePaused,
-        oldAnim->PropertiesOverridenByJS(), aBuilder, timeline.get(),
+        oldAnim->PropertiesOverridenByJS(), aBuilder, aTarget, timeline.get(),
         timelineName, timingFunction, composition, std::move(range),
         aTimelineNamesToAnimationMap);
     // For now, only name-referenced timeline, or `none`, which is represented
@@ -457,8 +457,8 @@ static already_AddRefed<CSSAnimation> BuildAnimation(
       std::move(timing), effectOptions);
   effect->SetDefaultTimingFunction(timingFunction);
   effect->SetDefaultComposite(composition);
-
-  aBuilder.SetKeyframes(*effect, std::move(keyframes), timeline, range);
+  effect->KeyframeEffect::SetKeyframes(std::move(keyframes), aBuilder.Style(),
+                                       timeline, &range);
 
   auto animation = MakeRefPtr<CSSAnimation>(
       aPresContext->Document()->GetScopeObject(), animationName);
diff --git a/servo/components/style/rule_tree/core.rs b/servo/components/style/rule_tree/core.rs
index 0c6fd029129..479dcfe7b2a 100644
--- a/servo/components/style/rule_tree/core.rs
+++ b/servo/components/style/rule_tree/core.rs
@@ -563,12 +563,20 @@ impl StrongRuleNode {
         self.p.parent.as_ref()
     }
 
+    fn root_matches(&self, root: &StrongRuleNode) -> bool {
+        match self.p.root {
+            Some(ref r) => *r == unsafe { root.downgrade() },
+            None => root == self,
+        }
+    }
+
     pub(super) fn ensure_child(
         &self,
         root: &StrongRuleNode,
         source: StyleSourceBorrow,
         cascade_priority: CascadePriority,
     ) -> StrongRuleNode {
+        debug_assert!(self.root_matches(root), "Mismatched tree!");
         debug_assert!(
             self.p.cascade_priority <= cascade_priority,
             "Should be ordered (instead {:?} > {:?}), from {:?} and {:?}",
@@ -736,7 +744,6 @@ impl Drop for StrongRuleNode {
         }
     }
 }
-
 impl WeakRuleNode {
     /// Upgrades this weak node reference, returning a strong one.
     ///
@@ -772,6 +779,13 @@ impl PartialEq for StrongRuleNode {
     }
 }
 
+impl Eq for WeakRuleNode {}
+impl PartialEq for WeakRuleNode {
+    fn eq(&self, other: &Self) -> bool {
+        &*self.p as *const RuleNode == &*other.p
+    }
+}
+
 impl hash::Hash for StrongRuleNode {
     fn hash<H>(&self, state: &mut H)
     where
diff --git a/testing/web-platform/tests/web-animations/crashtests/keyframe-effect-cross-doc-target.html b/testing/web-platform/tests/web-animations/crashtests/keyframe-effect-cross-doc-target.html
new file mode 100644
index 00000000000..fd3eee2fa05
--- /dev/null
+++ b/testing/web-platform/tests/web-animations/crashtests/keyframe-effect-cross-doc-target.html
@@ -0,0 +1,45 @@
+<!doctype html>
+<html class="test-wait">
+<style>
+  @keyframes anim {
+    from {
+      left: 20px
+    }
+  }
+
+  .owner {
+    animation: anim 10000s linear;
+  }
+
+  .owner.bad {
+    animation-timing-function: ease !important;
+    color: red !important;
+    animation-duration: 9999s;
+  }
+</style>
+<script>
+  function tick() {
+    return new Promise(resolve => requestAnimationFrame(() => requestAnimationFrame(resolve)));
+  }
+  window.onload = async function () {
+    let owner = document.createElement("div");
+    owner.textContent = "hello";
+    owner.className = "owner";
+    document.body.append(owner);
+    let frame = document.createElement("iframe");
+    document.body.append(frame);
+    frame.contentDocument.body.clientWidth;
+
+    let animation = owner.getAnimations()[0];
+    animation.effect.target = frame.contentDocument.body;
+    let second = owner.animate([{opacity: 0.5}, {opacity: 0.9}], 1e7);
+    await tick();
+    owner.classList.add("bad");
+    owner.getAnimations();
+    second.cancel();
+    second = null;
+    owner.clientWidth;
+    await tick();
+    document.documentElement.className = "";
+  };
+</script>
Loading diff…

Regression Test / PoC

shipped with the fix
diff --git a/testing/web-platform/tests/web-animations/crashtests/keyframe-effect-cross-doc-target.html b/testing/web-platform/tests/web-animations/crashtests/keyframe-effect-cross-doc-target.html
new file mode 100644
index 00000000000..fd3eee2fa05
--- /dev/null
+++ b/testing/web-platform/tests/web-animations/crashtests/keyframe-effect-cross-doc-target.html
@@ -0,0 +1,45 @@
+<!doctype html>
+<html class="test-wait">
+<style>
+  @keyframes anim {
+    from {
+      left: 20px
+    }
+  }
+
+  .owner {
+    animation: anim 10000s linear;
+  }
+
+  .owner.bad {
+    animation-timing-function: ease !important;
+    color: red !important;
+    animation-duration: 9999s;
+  }
+</style>
+<script>
+  function tick() {
+    return new Promise(resolve => requestAnimationFrame(() => requestAnimationFrame(resolve)));
+  }
+  window.onload = async function () {
+    let owner = document.createElement("div");
+    owner.textContent = "hello";
+    owner.className = "owner";
+    document.body.append(owner);
+    let frame = document.createElement("iframe");
+    document.body.append(frame);
+    frame.contentDocument.body.clientWidth;
+
+    let animation = owner.getAnimations()[0];
+    animation.effect.target = frame.contentDocument.body;
+    let second = owner.animate([{opacity: 0.5}, {opacity: 0.9}], 1e7);
+    await tick();
+    owner.classList.add("bad");
+    owner.getAnimations();
+    second.cancel();
+    second = null;
+    owner.clientWidth;
+    await tick();
+    document.documentElement.className = "";
+  };
+</script>
Loading diff…