⚠ Archived content — this site is no longer maintained.   Current WebKit documentation is at docs.webkit.org.

Changeset 286532 in webkit


Ignore:
Timestamp:
Dec 4, 2021, 7:28:47 AM (5 years ago)
Author:
graouts@webkit.org
Message:

CSS animation sorting may crash due to AnimationList copy upon CSS Animation removal
​https://bugs.webkit.org/show_bug.cgi?id=233819
rdar://85596065

Reviewed by Dean Jackson and Darin Adler.

Source/WebCore:

Test: webanimations/css-animation-sorting-crash-2.html

When resolving animations during style resolution, we sort them to ensure they're in the right
composite order as defined by the Web Animations and related specifications, in this case the
CSS Animations specification defining how CSS Animations set on a given element are sorted.

Indeed, a given element may have multiple animations specified on it, and in fact it may have
multiple instances of the same animation. For instance, setting animation-name: anim, anim, anim
would create three separate CSS Animations all with the same keyframes and timing properties.

The CSS machinery within WebKit creates an AnimationList to reference the animations parsed from
CSS. Each animation is an Animation object. These Animation objects are ref-counted.

When we update animations, using Styleable::updateCSSAnimations(), we compare the current AnimationList
for this style resolution, with the previous AnimationList specified when this method was last called
for this element. The outcome of this comparison will yield new CSSAnimation objects, the removal of
such objects or the update of existing objects by setting the Animation object as its "backing animation".

When we're done we keep a reference to the current AnimationList on the element's KeyframeEffectStack.

Later, when we resolve animations during style resolution and we get to sort the animations, we will
use the AnimationList which contains the Animation objects in the order they were specified in the
animation-list property to establish the order in which the CSSAnimation objects should be ordered
relative to one another, based on their "backing animation" which must be an Animation object found
in the AnimationList.

If we fail to find matching Animation objects, we crash due to a call to RELEASE_ASSERT_NOT_REACHED()
in compareCSSAnimations(const CSSAnimation&, const CSSAnimation&).

So, why would we ever get in a situation where we reach this RELEASE_ASSERT_NOT_REACHED? Well, there is
a situation where we manipulate the AnimationList set on the KeyframeEffectStack in Styleable::updateCSSAnimations().
That case is when Styleable::cancelDeclarativeAnimations() is called, and we call the static function
removeCSSAnimationCreatedByMarkup(). In this function, we actually make a copy of the previously recorded
AnimationList because that list is const so we can't manipulate it directly. To make this copy we call
AnimationList::copy() which creates a new AnimationList object, which itself is not the issue, but also
makes copies of each Animation object within. Now, that's the problem, because at this point our pointer
comparisons in compareCSSAnimations() will fail since the Animation objects we recorded in
Styleable::updateCSSAnimations() will no longer be the same instances as those in the manipulated AnimationList.

To fix this, we add a new AnimationList::shallowCopy() method to specify whether we want clones or references
of the Animation members, and when calling removeCSSAnimationCreatedByMarkup(), we use this new method such
that we get references and not clones. This ensures that removing an animation from the list will indeed
create a new AnimationList, but the two lists will have references to the same Animation objects.

  • platform/animation/AnimationList.cpp:

(WebCore::AnimationList::AnimationList):

  • platform/animation/AnimationList.h:

(WebCore::AnimationList::copy const):
(WebCore::AnimationList::shallowCopy const):

  • style/Styleable.cpp:

(WebCore::removeCSSAnimationCreatedByMarkup):

LayoutTests:

Add a test that used to crash before this patch.

  • webanimations/css-animation-sorting-crash-2-expected.txt: Added.
  • webanimations/css-animation-sorting-crash-2.html: Added.
Location:
trunk
Files:
2 added
5 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r286531 r286532  
     12021-12-04  Antoine Quint  <graouts@webkit.org>
     2
     3        CSS animation sorting may crash due to AnimationList copy upon CSS Animation removal
     4        https://bugs.webkit.org/show_bug.cgi?id=233819
     5        rdar://85596065
     6
     7        Reviewed by Dean Jackson and Darin Adler.
     8
     9        Add a test that used to crash before this patch.
     10
     11        * webanimations/css-animation-sorting-crash-2-expected.txt: Added.
     12        * webanimations/css-animation-sorting-crash-2.html: Added.
     13
    1142021-12-04  Rob Buis  <rbuis@igalia.com>
    215

  • trunk/Source/WebCore/ChangeLog

    r286531 r286532  
     12021-12-04  Antoine Quint  <graouts@webkit.org>
     2
     3        CSS animation sorting may crash due to AnimationList copy upon CSS Animation removal
     4        https://bugs.webkit.org/show_bug.cgi?id=233819
     5        rdar://85596065
     6
     7        Reviewed by Dean Jackson and Darin Adler.
     8
     9        Test: webanimations/css-animation-sorting-crash-2.html
     10
     11        When resolving animations during style resolution, we sort them to ensure they're in the right
     12        composite order as defined by the Web Animations and related specifications, in this case the
     13        CSS Animations specification defining how CSS Animations set on a given element are sorted.
     14
     15        Indeed, a given element may have multiple animations specified on it, and in fact it may have
     16        multiple instances of the same animation. For instance, setting `animation-name: anim, anim, anim`
     17        would create three separate CSS Animations all with the same keyframes and timing properties.
     18
     19        The CSS machinery within WebKit creates an AnimationList to reference the animations parsed from
     20        CSS. Each animation is an Animation object. These Animation objects are ref-counted.
     21
     22        When we update animations, using Styleable::updateCSSAnimations(), we compare the current AnimationList
     23        for this style resolution, with the previous AnimationList specified when this method was last called
     24        for this element. The outcome of this comparison will yield new CSSAnimation objects, the removal of
     25        such objects or the update of existing objects by setting the Animation object as its "backing animation".
     26
     27        When we're done we keep a reference to the current AnimationList on the element's KeyframeEffectStack.
     28
     29        Later, when we resolve animations during style resolution and we get to sort the animations, we will
     30        use the AnimationList which contains the Animation objects in the order they were specified in the
     31        `animation-list` property to establish the order in which the CSSAnimation objects should be ordered
     32        relative to one another, based on their "backing animation" which must be an Animation object found
     33        in the AnimationList.
     34
     35        If we fail to find matching Animation objects, we crash due to a call to RELEASE_ASSERT_NOT_REACHED()
     36        in compareCSSAnimations(const CSSAnimation&, const CSSAnimation&).
     37
     38        So, why would we ever get in a situation where we reach this RELEASE_ASSERT_NOT_REACHED? Well, there is
     39        a situation where we manipulate the AnimationList set on the KeyframeEffectStack in Styleable::updateCSSAnimations().
     40        That case is when Styleable::cancelDeclarativeAnimations() is called, and we call the static function
     41        removeCSSAnimationCreatedByMarkup(). In this function, we actually make a copy of the previously recorded
     42        AnimationList because that list is `const` so we can't manipulate it directly. To make this copy we call
     43        AnimationList::copy() which creates a new AnimationList object, which itself is not the issue, but also
     44        makes copies of each Animation object within. Now, that's the problem, because at this point our pointer
     45        comparisons in compareCSSAnimations() will fail since the Animation objects we recorded in
     46        Styleable::updateCSSAnimations() will no longer be the same instances as those in the manipulated AnimationList.
     47
     48        To fix this, we add a new AnimationList::shallowCopy() method to specify whether we want clones or references
     49        of the Animation members, and when calling removeCSSAnimationCreatedByMarkup(), we use this new method such
     50        that we get references and not clones. This ensures that removing an animation from the list will indeed
     51        create a new AnimationList, but the two lists will have references to the same Animation objects.
     52
     53        * platform/animation/AnimationList.cpp:
     54        (WebCore::AnimationList::AnimationList):
     55        * platform/animation/AnimationList.h:
     56        (WebCore::AnimationList::copy const):
     57        (WebCore::AnimationList::shallowCopy const):
     58        * style/Styleable.cpp:
     59        (WebCore::removeCSSAnimationCreatedByMarkup):
     60
    1612021-12-04  Rob Buis  <rbuis@igalia.com>
    262

  • trunk/Source/WebCore/platform/animation/AnimationList.cpp

    r284312 r286532  
    3636AnimationList::AnimationList() = default;
    3737
    38 AnimationList::AnimationList(const AnimationList& other)
     38AnimationList::AnimationList(const AnimationList& other, CopyBehavior copyBehavior)
    3939    : RefCounted()
    4040{
    4141    m_animations.reserveInitialCapacity(other.size());
    42     for (auto& animation : other.m_animations)
    43         m_animations.uncheckedAppend(Animation::create(animation.get()));
     42    for (auto& animation : other.m_animations) {
     43        if (copyBehavior == CopyBehavior::Reference)
     44            m_animations.uncheckedAppend(animation.get());
     45        else
     46            m_animations.uncheckedAppend(Animation::create(animation.get()));
     47    }
    4448}
    4549

  • trunk/Source/WebCore/platform/animation/AnimationList.h

    r255076 r286532  
    3636    static Ref<AnimationList> create() { return adoptRef(*new AnimationList); }
    3737
    38     Ref<AnimationList> copy() const { return adoptRef(*new AnimationList(*this)); }
     38    Ref<AnimationList> copy() const { return adoptRef(*new AnimationList(*this, CopyBehavior::Clone)); }
     39    Ref<AnimationList> shallowCopy() const { return adoptRef(*new AnimationList(*this, CopyBehavior::Reference)); }
    3940
    4041    void fillUnsetProperties();
    … …  
    5758private:
    5859    AnimationList();
    59     AnimationList(const AnimationList&);
     60
     61    enum class CopyBehavior : uint8_t { Clone, Reference };
     62    AnimationList(const AnimationList&, CopyBehavior);
    6063
    6164    AnimationList& operator=(const AnimationList&);

  • trunk/Source/WebCore/style/Styleable.cpp

    r284871 r286532  
    164164    for (size_t i = 0; i < cssAnimationList->size(); ++i) {
    165165        if (cssAnimationList->animation(i) == backingAnimation) {
    166             auto newAnimationList = cssAnimationList->copy();
     166            // It is important we do not make a clone of the Animation references contained
     167            // within cssAnimationList since sorting animations in compareCSSAnimations()
     168            // makes pointer comparisons to distinguish between backing animations of various
     169            // CSSAnimation objects.
     170            auto newAnimationList = cssAnimationList->shallowCopy();
    167171            newAnimationList->remove(i);
    168172            keyframeEffectStack.setCSSAnimationList(WTFMove(newAnimationList));
Note: See TracChangeset for help on using the changeset viewer.