Verified Commit 9016cc66 authored by Emilio Cobos Álvarez's avatar Emilio Cobos Álvarez Committed by ma1
Browse files

Bug 2074625 - Avoid updating properties with retargeted animation. a=diannaS DONTBUILD 

This is the low risk fix. I think we need to figure out whether we even
want to use that retargeted effect at all? But that seems like a
follow-up bug.

Rule tree changes are just assertions that catch this earlier.

Original Revision: https://phabricator.services.mozilla.com/D328567

Differential Revision: https://phabricator.services.mozilla.com/D328800
parent e8a9f66c

layout/style/nsAnimationManager.cpp

+15 −13
Changes for layout/style/nsAnimationManager.cpp: 15 added lines, 13 removed lines.
Original line number Diff line number Diff line
@@ -79,6 +79,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,
@@ -86,10 +88,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) {

    aEffect.SetKeyframes(std::move(aKeyframes), mComputedStyle, aTimeline);

  }



  // Currently all the animation building code in this file is based on

  // assumption that creating and removing animations should *not* trigger
@@ -137,15 +135,13 @@ static void UpdateOldAnimationPropertiesWithNew( 
    CSSAnimation& aOld, TimingParams&& aNewTiming,

    nsTArray<Keyframe>&& aNewKeyframes, bool aNewIsStylePaused,

    CSSAnimationProperties aOverriddenProperties,

    ServoCSSAnimationBuilder& aBuilder, dom::AnimationTimeline* aTimeline,

    dom::CompositeOperation aNewComposite) {

    ServoCSSAnimationBuilder& aBuilder, const NonOwningAnimationTarget& aTarget,

    dom::AnimationTimeline* aTimeline, dom::CompositeOperation aNewComposite) {

  bool animationChanged = false;



  // 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) {
@@ -169,7 +165,12 @@ static void UpdateOldAnimationPropertiesWithNew( 


    if (KeyframeEffect* oldKeyframeEffect = oldEffect->AsKeyframeEffect()) {

      if (~aOverriddenProperties & CSSAnimationProperties::Keyframes) {

        aBuilder.SetKeyframes(*oldKeyframeEffect, std::move(aNewKeyframes),

        // 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);

      }
@@ -330,7 +331,8 @@ 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->GetOverriddenProperties(), aBuilder, timeline, composition);

        oldAnim->GetOverriddenProperties(), aBuilder, aTarget, timeline,

        composition);

    return oldAnim.forget();

  }
@@ -339,8 +341,8 @@ static already_AddRefed<CSSAnimation> BuildAnimation( 
      aPresContext->Document(),

      OwningAnimationTarget(aTarget.mElement, aTarget.mPseudoRequest),

      std::move(timing), effectOptions);



  aBuilder.SetKeyframes(*effect, std::move(keyframes), timeline);

  effect->KeyframeEffect::SetKeyframes(std::move(keyframes), aBuilder.Style(),

                                       timeline);



  auto animation = MakeRefPtr<CSSAnimation>(

      aPresContext->Document()->GetScopeObject(), animationName);

servo/components/style/rule_tree/core.rs

+15 −1
Changes for servo/components/style/rule_tree/core.rs: 15 added lines, 1 removed line.
Original line number Diff line number Diff line
@@ -548,6 +548,13 @@ 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,
@@ -556,6 +563,7 @@ impl StrongRuleNode { 
    ) -> StrongRuleNode {

        use parking_lot::RwLockUpgradableReadGuard;



        debug_assert!(self.root_matches(root), "Mismatched tree!");

        debug_assert!(

            self.p.cascade_priority <= cascade_priority,

            "Should be ordered (instead {:?} > {:?}), from {:?} and {:?}",
@@ -721,7 +729,6 @@ impl Drop for StrongRuleNode { 
        }

    }

}



impl WeakRuleNode {

    /// Upgrades this weak node reference, returning a strong one.

    ///
@@ -757,6 +764,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

testing/web-platform/tests/web-animations/crashtests/keyframe-effect-cross-doc-target.html

0 → 100644
+45 −0
Changes for testing/web-platform/tests/web-animations/crashtests/keyframe-effect-cross-doc-target.html: 45 added lines, 0 removed lines.
Original line number Diff line number Diff line 
<!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>