Document should not be mutated under SMILTimeContainer::updateAnimations()
authorsaid@apple.com <said@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 18 Jun 2018 19:28:33 +0000 (19:28 +0000)
committersaid@apple.com <said@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 18 Jun 2018 19:28:33 +0000 (19:28 +0000)
https://bugs.webkit.org/show_bug.cgi?id=186658

Reviewed by Simon Fraser.

Source/WebCore:

To update the animation of an SVG <animate> element, we call
SVGAnimateElementBase::resetAnimatedType(). It ensures the pointer m_animator
is valid. If it animates a css property, it calls computeCSSPropertyValue()
which calls resolveStyle() via other calls. resolveStyle() may call delayed
callbacks through the destructor of PostResolutionCallbackDisabler. These
callbacks may fire events. These events may execute JS event handlers.
If one of these event handlers deletes the same SVG <animate> we animate,
we will end up calling SVGAnimateElementBase::resetAnimatedPropertyType()
of the same <animate> element. This function  will delete the same m_animator
which resetAnimatedType() still holds and will use later. This code
re-entrance is unexpected and unwanted.

The fix is to disable mutating the DOM while updating the SVG animations.

Test: svg/dom/css-animate-input-foucs-crash.html

* svg/animation/SMILTimeContainer.cpp:
(WebCore::SMILTimeContainer::updateAnimations):

LayoutTests:

* svg/dom/css-animate-input-foucs-crash-expected.txt: Added.
* svg/dom/css-animate-input-foucs-crash.html: Added.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@232941 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/svg/dom/css-animate-input-foucs-crash-expected.txt [new file with mode: 0644]
LayoutTests/svg/dom/css-animate-input-foucs-crash.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/svg/animation/SMILTimeContainer.cpp

index 97f64fc..5ce4103 100644 (file)
@@ -1,3 +1,13 @@
+2018-06-18  Said Abou-Hallawa  <sabouhallawa@apple.com>
+
+        Document should not be mutated under SMILTimeContainer::updateAnimations()
+        https://bugs.webkit.org/show_bug.cgi?id=186658
+
+        Reviewed by Simon Fraser.
+
+        * svg/dom/css-animate-input-foucs-crash-expected.txt: Added.
+        * svg/dom/css-animate-input-foucs-crash.html: Added.
+
 2018-06-18  Wenson Hsieh  <wenson_hsieh@apple.com>
 
         fast/forms/button-set-display-flex-justifyContent-center.html is failing on macOS Mojave
diff --git a/LayoutTests/svg/dom/css-animate-input-foucs-crash-expected.txt b/LayoutTests/svg/dom/css-animate-input-foucs-crash-expected.txt
new file mode 100644 (file)
index 0000000..3b8e838
--- /dev/null
@@ -0,0 +1,4 @@
+This test passes if it doesn't crash.
+
+
diff --git a/LayoutTests/svg/dom/css-animate-input-foucs-crash.html b/LayoutTests/svg/dom/css-animate-input-foucs-crash.html
new file mode 100644 (file)
index 0000000..2af1609
--- /dev/null
@@ -0,0 +1,23 @@
+<body>
+    <p>This test passes if it doesn't crash.</p>
+    <svg id="svgRoot">
+        <animate attributeName="fill" />
+    </svg>
+    <div id="inputParent" onfocusin="onFoucsIn()">
+        <keygen id="input">
+    </div>
+    <details ontoggle="onToggle()" open="true"></details>
+    <script>
+        if (window.testRunner)
+            testRunner.dumpAsText();
+
+        function onFoucsIn() {
+            svgRoot.remove();
+        }
+
+        function onToggle() {
+            input.autofocus = true;
+            inputParent.after(inputParent);
+        }
+    </script>
+</body>
index 2e8eb6f..c384be0 100644 (file)
@@ -1,3 +1,29 @@
+2018-06-18  Said Abou-Hallawa  <sabouhallawa@apple.com>
+
+        Document should not be mutated under SMILTimeContainer::updateAnimations()
+        https://bugs.webkit.org/show_bug.cgi?id=186658
+
+        Reviewed by Simon Fraser.
+
+        To update the animation of an SVG <animate> element, we call
+        SVGAnimateElementBase::resetAnimatedType(). It ensures the pointer m_animator
+        is valid. If it animates a css property, it calls computeCSSPropertyValue()
+        which calls resolveStyle() via other calls. resolveStyle() may call delayed
+        callbacks through the destructor of PostResolutionCallbackDisabler. These
+        callbacks may fire events. These events may execute JS event handlers.
+        If one of these event handlers deletes the same SVG <animate> we animate,
+        we will end up calling SVGAnimateElementBase::resetAnimatedPropertyType()
+        of the same <animate> element. This function  will delete the same m_animator
+        which resetAnimatedType() still holds and will use later. This code
+        re-entrance is unexpected and unwanted.
+
+        The fix is to disable mutating the DOM while updating the SVG animations.
+
+        Test: svg/dom/css-animate-input-foucs-crash.html
+
+        * svg/animation/SMILTimeContainer.cpp:
+        (WebCore::SMILTimeContainer::updateAnimations):
+
 2018-06-18  Commit Queue  <commit-queue@webkit.org>
 
         Unreviewed, rolling out r232935.
index 56a46a6..e0423ce 100644 (file)
@@ -31,6 +31,7 @@
 #include "Page.h"
 #include "SVGSMILElement.h"
 #include "SVGSVGElement.h"
+#include "ScopedEventQueue.h"
 
 namespace WebCore {
 
@@ -254,6 +255,9 @@ void SMILTimeContainer::updateAnimations(SMILTime elapsed, bool seekToTime)
 {
     SMILTime earliestFireTime = SMILTime::unresolved();
 
+    // Don't mutate the DOM while updating the animations.
+    EventQueueScope scope;
+    
 #ifndef NDEBUG
     // This boolean will catch any attempts to schedule/unschedule scheduledAnimations during this critical section.
     // Similarly, any elements removed will unschedule themselves, so this will catch modification of animationsToApply.