Changeset 286642 in webkit

Timestamp:

Dec 7, 2021, 10:28:55 PM (5 years ago)

Author:

graouts@webkit.org

Message:

ActiveDOMObject::suspendIfNeeded() should not be called in the WebAnimation constructor
​https://bugs.webkit.org/show_bug.cgi?id=233932

Reviewed by Chris Dumez.

It is not safe to call ActiveDOMObject::suspendIfNeeded() in a constructor. Since WebAnimation has several subclasses
(DeclarativeAnimation, and then CSSTransition and CSSAnimation deriving from DeclarativeAnimation) each with their own
create() methods, we add a new protected initialize() method which refactors code from the WebAnimation constructor
and is called from the two WebAnimation::create() methods.

DeclarativeAnimation already had an initialize method for common setup for the create() methods in CSSTransition and
CSS Animation, so we can simply call WebAnimation::initialize() from DeclarativeAnimation::initialize() and this guarantees
all create() methods for WebAnimation and all of its subclasses are correctly calling ActiveDOMObject::suspendIfNeeded().

• animation/DeclarativeAnimation.cpp:

(WebCore::DeclarativeAnimation::initialize):

• animation/DeclarativeAnimation.h:
• animation/WebAnimation.cpp:

(WebCore::WebAnimation::create):
(WebCore::WebAnimation::initialize):
(WebCore::WebAnimation::WebAnimation):

• animation/WebAnimation.h:

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• animation/DeclarativeAnimation.cpp (modified) (1 diff)
• animation/DeclarativeAnimation.h (modified) (1 diff)
• animation/WebAnimation.cpp (modified) (4 diffs)
• animation/WebAnimation.h (modified) (1 diff)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2021-12-07  Antoine Quint  <graouts@webkit.org>
+
+        ActiveDOMObject::suspendIfNeeded() should not be called in the WebAnimation constructor
+        https://bugs.webkit.org/show_bug.cgi?id=233932
+
+        Reviewed by Chris Dumez.
+
+        It is not safe to call ActiveDOMObject::suspendIfNeeded() in a constructor. Since WebAnimation has several subclasses
+        (DeclarativeAnimation, and then CSSTransition and CSSAnimation deriving from DeclarativeAnimation) each with their own
+        create() methods, we add a new protected initialize() method which refactors code from the WebAnimation constructor
+        and is called from the two WebAnimation::create() methods.
+
+        DeclarativeAnimation already had an initialize method for common setup for the create() methods in CSSTransition and
+        CSS Animation, so we can simply call WebAnimation::initialize() from DeclarativeAnimation::initialize() and this guarantees
+        all create() methods for WebAnimation and all of its subclasses are correctly calling ActiveDOMObject::suspendIfNeeded().
+
+        * animation/DeclarativeAnimation.cpp:
+        (WebCore::DeclarativeAnimation::initialize):
+        * animation/DeclarativeAnimation.h:
+        * animation/WebAnimation.cpp:
+        (WebCore::WebAnimation::create):
+        (WebCore::WebAnimation::initialize):
+        (WebCore::WebAnimation::WebAnimation):
+        * animation/WebAnimation.h:
+
 2021-12-07  Wenson Hsieh  <wenson_hsieh@apple.com>
 

trunk/Source/WebCore/animation/DeclarativeAnimation.cpp

@@ -108 +108 @@
 void DeclarativeAnimation::initialize(const RenderStyle* oldStyle, const RenderStyle& newStyle, const Style::ResolutionContext& resolutionContext)
 {
+    WebAnimation::initialize();
+
     // We need to suspend invalidation of the animation's keyframe effect during its creation
     // as it would otherwise trigger invalidation of the document's style and this would be

trunk/Source/WebCore/animation/DeclarativeAnimation.h

@@ -76 +76 @@
     DeclarativeAnimation(const Styleable&, const Animation&);
 
-    virtual void initialize(const RenderStyle* oldStyle, const RenderStyle& newStyle, const Style::ResolutionContext&);
+    void initialize(const RenderStyle* oldStyle, const RenderStyle& newStyle, const Style::ResolutionContext&);
     virtual void syncPropertiesWithBackingAnimation();
     // elapsedTime is the animation's current time at the time the event is added and is exposed through the DOM API, timelineTime is the animations'

trunk/Source/WebCore/animation/WebAnimation.cpp

@@ -67 +67 @@
 {
     auto result = adoptRef(*new WebAnimation(document));
+    result->initialize();
     result->setEffect(effect);
     result->setTimeline(&document.timeline());
@@ -78 +79 @@
 {
     auto result = adoptRef(*new WebAnimation(document));
+    result->initialize();
     result->setEffect(effect);
     if (timeline)
@@ -85 +87 @@
 
     return result;
+}
+
+void WebAnimation::initialize()
+{
+    suspendIfNeeded();
+    m_readyPromise->resolve(*this);
 }
 
@@ -92 +100 @@
     , m_finishedPromise(makeUniqueRef<FinishedPromise>(*this, &WebAnimation::finishedPromiseResolve))
 {
-    m_readyPromise->resolve(*this);
-    suspendIfNeeded();
-
     instances().add(this);
 }

trunk/Source/WebCore/animation/WebAnimation.h

@@ -156 +156 @@
     explicit WebAnimation(Document&);
 
+    void initialize();
     void enqueueAnimationEvent(Ref<AnimationEventBase>&&);