Changeset 254201 in webkit

Timestamp:

Jan 8, 2020, 8:30:39 AM (7 years ago)

Author:

graouts@webkit.org

Message:

[Web Animations] Stop creating CSS Animations for <noscript> elements
​https://bugs.webkit.org/show_bug.cgi?id=205925
<rdar://problem/58158479>

Reviewed by Antti Koivisto.

Source/WebCore:

Test: webanimations/no-css-animation-on-noscript.html

It makes no sense to create CSS Animations for a <noscript> element and it has the side effect of potential crashes.
Indeed, AnimationTimeline::updateCSSAnimationsForElement() may be called without a currentStyle and so we never have
a list of previously-applied animations to compare to the list of animations in afterChangeStyle. So on each call we
end up creating a new CSSAnimation and the previous animation for the same name is never explicitly removed from the
effect stack and is eventually destroyed and the WeakPtr for it in the stack ends up being null, which would cause a
crash under KeyframeEffectStack::ensureEffectsAreSorted().

We now prevent elements such as <noscript> from being considered for CSS Animations in TreeResolver::resolveElement().

• dom/Element.cpp:

(WebCore::Element::rendererIsNeeded):

• dom/Element.h:

(WebCore::Element::rendererIsEverNeeded):

• html/HTMLElement.cpp:

(WebCore::HTMLElement::rendererIsEverNeeded):
(WebCore::HTMLElement::rendererIsNeeded): Deleted.

• html/HTMLElement.h:
• style/StyleTreeResolver.cpp:

(WebCore::Style::TreeResolver::resolveElement):

LayoutTests:

Add a new test that checks that setting the animation property on a <noscript> element does not yield the creation of a CSSAnimation object.

• webanimations/no-css-animation-on-noscript-expected.txt: Added.
• webanimations/no-css-animation-on-noscript.html: Added.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/webanimations/no-css-animation-on-noscript-expected.txt (added)
• LayoutTests/webanimations/no-css-animation-on-noscript.html (added)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/dom/Element.cpp (modified) (1 diff)
• Source/WebCore/dom/Element.h (modified) (1 diff)
• Source/WebCore/html/HTMLElement.cpp (modified) (2 diffs)
• Source/WebCore/html/HTMLElement.h (modified) (1 diff)
• Source/WebCore/style/StyleTreeResolver.cpp (modified) (1 diff)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2020-01-08  Antoine Quint  <graouts@apple.com>
+
+        [Web Animations] Stop creating CSS Animations for <noscript> elements
+        https://bugs.webkit.org/show_bug.cgi?id=205925
+        <rdar://problem/58158479>
+
+        Reviewed by Antti Koivisto.
+
+        Add a new test that checks that setting the `animation` property on a <noscript> element does not yield the creation of a CSSAnimation object.
+
+        * webanimations/no-css-animation-on-noscript-expected.txt: Added.
+        * webanimations/no-css-animation-on-noscript.html: Added.
+
 2020-01-08  youenn fablet  <youenn@apple.com>
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2020-01-08  Antoine Quint  <graouts@apple.com>
+
+        [Web Animations] Stop creating CSS Animations for <noscript> elements
+        https://bugs.webkit.org/show_bug.cgi?id=205925
+        <rdar://problem/58158479>
+
+        Reviewed by Antti Koivisto.
+
+        Test: webanimations/no-css-animation-on-noscript.html
+
+        It makes no sense to create CSS Animations for a <noscript> element and it has the side effect of potential crashes.
+        Indeed, AnimationTimeline::updateCSSAnimationsForElement() may be called without a currentStyle and so we never have
+        a list of previously-applied animations to compare to the list of animations in afterChangeStyle. So on each call we
+        end up creating a new CSSAnimation and the previous animation for the same name is never explicitly removed from the
+        effect stack and is eventually destroyed and the WeakPtr for it in the stack ends up being null, which would cause a
+        crash under KeyframeEffectStack::ensureEffectsAreSorted().
+
+        We now prevent elements such as <noscript> from being considered for CSS Animations in TreeResolver::resolveElement().
+
+        * dom/Element.cpp:
+        (WebCore::Element::rendererIsNeeded):
+        * dom/Element.h:
+        (WebCore::Element::rendererIsEverNeeded):
+        * html/HTMLElement.cpp:
+        (WebCore::HTMLElement::rendererIsEverNeeded):
+        (WebCore::HTMLElement::rendererIsNeeded): Deleted.
+        * html/HTMLElement.h:
+        * style/StyleTreeResolver.cpp:
+        (WebCore::Style::TreeResolver::resolveElement):
+
 2020-01-08  Alicia Boya García  <aboya@igalia.com>
 

trunk/Source/WebCore/dom/Element.cpp

@@ -2115 +2115 @@
 bool Element::rendererIsNeeded(const RenderStyle& style)
 {
-    return style.display() != DisplayType::None && style.display() != DisplayType::Contents;
+    return rendererIsEverNeeded() && style.display() != DisplayType::None && style.display() != DisplayType::Contents;
 }
 

trunk/Source/WebCore/dom/Element.h

@@ -286 +286 @@
     virtual RenderPtr<RenderElement> createElementRenderer(RenderStyle&&, const RenderTreePosition&);
     virtual bool rendererIsNeeded(const RenderStyle&);
+    virtual bool rendererIsEverNeeded() { return true; }
 
     WEBCORE_EXPORT ShadowRoot* shadowRoot() const;

trunk/Source/WebCore/html/HTMLElement.cpp

@@ -734 +734 @@
 }
 
-bool HTMLElement::rendererIsNeeded(const RenderStyle& style)
+bool HTMLElement::rendererIsEverNeeded()
 {
     if (hasTagName(noscriptTag)) {
@@ -745 +745 @@
             return false;
     }
-    return StyledElement::rendererIsNeeded(style);
+    return StyledElement::rendererIsEverNeeded();
 }
 

trunk/Source/WebCore/html/HTMLElement.h

@@ -71 +71 @@
     void accessKeyAction(bool sendMouseEvents) override;
 
-    bool rendererIsNeeded(const RenderStyle&) override;
     RenderPtr<RenderElement> createElementRenderer(RenderStyle&&, const RenderTreePosition&) override;
+    bool rendererIsEverNeeded() final;
 
     WEBCORE_EXPORT virtual HTMLFormElement* form() const;

trunk/Source/WebCore/style/StyleTreeResolver.cpp

@@ -199 +199 @@
     }
 
+    if (!element.rendererIsEverNeeded())
+        return { };
+
     auto newStyle = styleForElement(element, parent().style);