Title: [214819] trunk
Revision
214819
Author
simon.fra...@apple.com
Date
2017-04-03 10:51:29 -0700 (Mon, 03 Apr 2017)

Log Message

Clean up touch event handler registration when moving nodes between documents
https://bugs.webkit.org/show_bug.cgi?id=170384
rdar://problem/30816694

Reviewed by Chris Dumez.

Source/WebCore:

Make sure that Node::didMoveToNewDocument() does the correct unregistration on the
old document, and registration on the new document for nodes with touch event listeners,
and gesture event listeners. Touch "handler" nodes (those for overflow and sliders) are
already correctly moved via renderer-related teardown.

Add assertions that fire when removal was not complete.

Use references in more places.

Tests: fast/events/touch/ios/gesture-node-move-between-documents.html
       fast/events/touch/ios/overflow-node-move-between-documents.html
       fast/events/touch/ios/slider-node-move-between-documents.html
       fast/events/touch/ios/touch-node-move-between-documents.html

* dom/EventNames.h:
(WebCore::EventNames::gestureEventNames):
* dom/Node.cpp:
(WebCore::Node::willBeDeletedFrom):
(WebCore::Node::didMoveToNewDocument):
(WebCore::tryAddEventListener):
(WebCore::tryRemoveEventListener):
* html/shadow/SliderThumbElement.cpp:
(WebCore::SliderThumbElement::registerForTouchEvents):
(WebCore::SliderThumbElement::unregisterForTouchEvents):
* rendering/RenderLayer.cpp:
(WebCore::RenderLayer::registerAsTouchEventListenerForScrolling):
(WebCore::RenderLayer::unregisterAsTouchEventListenerForScrolling):

LayoutTests:

Tests for moving nodes with various listener/handler combinations between documents.

* fast/events/touch/ios/gesture-node-move-between-documents-expected.txt: Added.
* fast/events/touch/ios/gesture-node-move-between-documents.html: Added.
* fast/events/touch/ios/overflow-node-move-between-documents-expected.txt: Added.
* fast/events/touch/ios/overflow-node-move-between-documents.html: Added.
* fast/events/touch/ios/slider-node-move-between-documents-expected.txt: Added.
* fast/events/touch/ios/slider-node-move-between-documents.html: Added.
* fast/events/touch/ios/touch-node-move-between-documents-expected.txt: Added.
* fast/events/touch/ios/touch-node-move-between-documents.html: Added.

Modified Paths

Added Paths

Diff

Modified: trunk/LayoutTests/ChangeLog (214818 => 214819)


--- trunk/LayoutTests/ChangeLog	2017-04-03 17:40:06 UTC (rev 214818)
+++ trunk/LayoutTests/ChangeLog	2017-04-03 17:51:29 UTC (rev 214819)
@@ -1,3 +1,22 @@
+2017-04-01  Simon Fraser  <simon.fra...@apple.com>
+
+        Clean up touch event handler registration when moving nodes between documents
+        https://bugs.webkit.org/show_bug.cgi?id=170384
+        rdar://problem/30816694
+
+        Reviewed by Chris Dumez.
+
+        Tests for moving nodes with various listener/handler combinations between documents.
+
+        * fast/events/touch/ios/gesture-node-move-between-documents-expected.txt: Added.
+        * fast/events/touch/ios/gesture-node-move-between-documents.html: Added.
+        * fast/events/touch/ios/overflow-node-move-between-documents-expected.txt: Added.
+        * fast/events/touch/ios/overflow-node-move-between-documents.html: Added.
+        * fast/events/touch/ios/slider-node-move-between-documents-expected.txt: Added.
+        * fast/events/touch/ios/slider-node-move-between-documents.html: Added.
+        * fast/events/touch/ios/touch-node-move-between-documents-expected.txt: Added.
+        * fast/events/touch/ios/touch-node-move-between-documents.html: Added.
+
 2017-04-03  Carlos Garcia Campos  <cgar...@igalia.com>
 
         [SOUP] URI Fragment is lost after redirect

Added: trunk/LayoutTests/fast/events/touch/ios/gesture-node-move-between-documents-expected.txt (0 => 214819)


--- trunk/LayoutTests/fast/events/touch/ios/gesture-node-move-between-documents-expected.txt	                        (rev 0)
+++ trunk/LayoutTests/fast/events/touch/ios/gesture-node-move-between-documents-expected.txt	2017-04-03 17:51:29 UTC (rev 214819)
@@ -0,0 +1 @@
+This test should not crash or assert.

Added: trunk/LayoutTests/fast/events/touch/ios/gesture-node-move-between-documents.html (0 => 214819)


--- trunk/LayoutTests/fast/events/touch/ios/gesture-node-move-between-documents.html	                        (rev 0)
+++ trunk/LayoutTests/fast/events/touch/ios/gesture-node-move-between-documents.html	2017-04-03 17:51:29 UTC (rev 214819)
@@ -0,0 +1,47 @@
+<!DOCTYPE html>
+
+<html>
+<head>
+    <script src=""
+    <script>
+        if (window.testRunner) {
+            testRunner.dumpAsText();
+            testRunner.waitUntilDone();
+        }
+
+        function doTest()
+        {
+            var iframe = document.querySelector('iframe');
+
+            var node = iframe.contentDocument.createElement('div');
+            node.addEventListener('gesturestart', function() { }, false);
+            node.addEventListener('gestureend', function() { }, false);
+            node.textContent = 'test node';
+            iframe.contentDocument.body.appendChild(node);
+
+            document.body.appendChild(node);
+            
+            node.remove();
+            
+            window.setTimeout(removeFrameAndGC, 0)
+        }
+        
+        function removeFrameAndGC()
+        {
+            var iframe = document.querySelector('iframe');
+            iframe.remove();
+            iframe = undefined;
+
+            gc();
+            if (window.testRunner)
+                testRunner.notifyDone();
+        }
+
+        window.addEventListener('load', doTest, false);
+    </script>
+</head>
+<body>
+    This test should not crash or assert.
+    <iframe srcdoc="<body></body>"></iframe>
+</body>
+</html>

Added: trunk/LayoutTests/fast/events/touch/ios/overflow-node-move-between-documents-expected.txt (0 => 214819)


--- trunk/LayoutTests/fast/events/touch/ios/overflow-node-move-between-documents-expected.txt	                        (rev 0)
+++ trunk/LayoutTests/fast/events/touch/ios/overflow-node-move-between-documents-expected.txt	2017-04-03 17:51:29 UTC (rev 214819)
@@ -0,0 +1 @@
+This test should not crash or assert.

Added: trunk/LayoutTests/fast/events/touch/ios/overflow-node-move-between-documents.html (0 => 214819)


--- trunk/LayoutTests/fast/events/touch/ios/overflow-node-move-between-documents.html	                        (rev 0)
+++ trunk/LayoutTests/fast/events/touch/ios/overflow-node-move-between-documents.html	2017-04-03 17:51:29 UTC (rev 214819)
@@ -0,0 +1,80 @@
+<!DOCTYPE html>
+
+<html>
+<head>
+    <style>
+        .container {
+            height: 100px;
+            width: 100px;
+            overflow: scroll;
+            border: 1px solid black;
+        }
+        
+        .contents {
+            height: 400px;
+            background-color: silver;
+        }
+    </style>
+    <script src=""
+    <script>
+        if (window.testRunner) {
+            testRunner.dumpAsText();
+            testRunner.waitUntilDone();
+        }
+
+        function doTest()
+        {
+            var iframe = document.querySelector('iframe');
+
+            var node = iframe.contentDocument.createElement('div');
+            node.className = 'container';
+            node.addEventListener('touchstart', function() { }, false);
+            node.addEventListener('gesturestart', function() { }, false);
+            
+            var contents = iframe.contentDocument.createElement('div');
+            contents.className = 'contents';
+            node.appendChild(contents);
+
+            iframe.contentDocument.body.appendChild(node);
+            iframe.contentDocument.body.offsetWidth;
+
+            document.body.appendChild(node);
+            document.body.offsetWidth;
+            
+            node.remove();
+
+            window.setTimeout(removeFrameAndGC, 0)
+        }
+
+        function removeFrameAndGC()
+        {
+            var iframe = document.querySelector('iframe');
+            iframe.remove();
+            iframe = undefined;
+
+            gc();
+            if (window.testRunner)
+                testRunner.notifyDone();
+        }
+
+        window.addEventListener('load', doTest, false);
+    </script>
+</head>
+<body>
+    This test should not crash or assert.
+    <iframe srcdoc="<style>
+        .container {
+            height: 100px;
+            width: 100px;
+            overflow: scroll;
+            border: 1px solid black;
+        }
+        
+        .contents {
+            height: 400px;
+            background-color: silver;
+        }
+        </style>"
+    ></iframe>
+</body>
+</html>

Added: trunk/LayoutTests/fast/events/touch/ios/slider-node-move-between-documents-expected.txt (0 => 214819)


--- trunk/LayoutTests/fast/events/touch/ios/slider-node-move-between-documents-expected.txt	                        (rev 0)
+++ trunk/LayoutTests/fast/events/touch/ios/slider-node-move-between-documents-expected.txt	2017-04-03 17:51:29 UTC (rev 214819)
@@ -0,0 +1 @@
+This test should not crash or assert.

Added: trunk/LayoutTests/fast/events/touch/ios/slider-node-move-between-documents.html (0 => 214819)


--- trunk/LayoutTests/fast/events/touch/ios/slider-node-move-between-documents.html	                        (rev 0)
+++ trunk/LayoutTests/fast/events/touch/ios/slider-node-move-between-documents.html	2017-04-03 17:51:29 UTC (rev 214819)
@@ -0,0 +1,47 @@
+<!DOCTYPE html>
+
+<html>
+<head>
+    <script src=""
+    <script>
+        if (window.testRunner) {
+            testRunner.dumpAsText();
+            testRunner.waitUntilDone();
+        }
+        
+        function doTest()
+        {
+            var iframe = document.querySelector('iframe');
+
+            var node = iframe.contentDocument.createElement('input');
+            node.setAttribute('type', 'range');
+            
+            iframe.contentDocument.body.appendChild(node);
+            iframe.contentDocument.body.offsetWidth;
+
+            document.body.appendChild(node);
+            document.body.offsetWidth;
+            node.remove();
+
+            window.setTimeout(removeFrameAndGC, 0)
+        }
+        
+        function removeFrameAndGC()
+        {
+            var iframe = document.querySelector('iframe');
+            iframe.remove();
+            iframe = undefined;
+
+            gc();
+            if (window.testRunner)
+                testRunner.notifyDone();
+        }
+
+        window.addEventListener('load', doTest, false);
+    </script>
+</head>
+<body>
+    This test should not crash or assert.
+    <iframe srcdoc="<body></body>"></iframe>
+</body>
+</html>

Added: trunk/LayoutTests/fast/events/touch/ios/touch-node-move-between-documents-expected.txt (0 => 214819)


--- trunk/LayoutTests/fast/events/touch/ios/touch-node-move-between-documents-expected.txt	                        (rev 0)
+++ trunk/LayoutTests/fast/events/touch/ios/touch-node-move-between-documents-expected.txt	2017-04-03 17:51:29 UTC (rev 214819)
@@ -0,0 +1 @@
+This test should not crash or assert.

Added: trunk/LayoutTests/fast/events/touch/ios/touch-node-move-between-documents.html (0 => 214819)


--- trunk/LayoutTests/fast/events/touch/ios/touch-node-move-between-documents.html	                        (rev 0)
+++ trunk/LayoutTests/fast/events/touch/ios/touch-node-move-between-documents.html	2017-04-03 17:51:29 UTC (rev 214819)
@@ -0,0 +1,49 @@
+<!DOCTYPE html>
+
+<html>
+<head>
+    <script src=""
+    <script>
+        if (window.testRunner) {
+            testRunner.dumpAsText();
+            testRunner.waitUntilDone();
+        }
+
+        function doTest()
+        {
+            var iframe = document.querySelector('iframe');
+
+            var node = iframe.contentDocument.createElement('div');
+            node.addEventListener('touchstart', function() { }, false);
+            node.addEventListener('touchstart', function() { }, false);
+            node.addEventListener('touchmove', function() { }, false);
+            node.addEventListener('touchend', function() { }, false);
+            node.textContent = 'test node';
+            iframe.contentDocument.body.appendChild(node);
+
+            document.body.appendChild(node);
+            
+            node.remove();
+            
+            window.setTimeout(removeFrameAndGC, 0)
+        }
+        
+        function removeFrameAndGC()
+        {
+            var iframe = document.querySelector('iframe');
+            iframe.remove();
+            iframe = undefined;
+
+            gc();
+            if (window.testRunner)
+                testRunner.notifyDone();
+        }
+
+        window.addEventListener('load', doTest, false);
+    </script>
+</head>
+<body>
+    This test should not crash or assert.
+    <iframe srcdoc="<body></body>"></iframe>
+</body>
+</html>

Modified: trunk/Source/WebCore/ChangeLog (214818 => 214819)


--- trunk/Source/WebCore/ChangeLog	2017-04-03 17:40:06 UTC (rev 214818)
+++ trunk/Source/WebCore/ChangeLog	2017-04-03 17:51:29 UTC (rev 214819)
@@ -1,3 +1,39 @@
+2017-04-01  Simon Fraser  <simon.fra...@apple.com>
+
+        Clean up touch event handler registration when moving nodes between documents
+        https://bugs.webkit.org/show_bug.cgi?id=170384
+        rdar://problem/30816694
+
+        Reviewed by Chris Dumez.
+
+        Make sure that Node::didMoveToNewDocument() does the correct unregistration on the
+        old document, and registration on the new document for nodes with touch event listeners,
+        and gesture event listeners. Touch "handler" nodes (those for overflow and sliders) are
+        already correctly moved via renderer-related teardown.
+
+        Add assertions that fire when removal was not complete.
+
+        Use references in more places.
+
+        Tests: fast/events/touch/ios/gesture-node-move-between-documents.html
+               fast/events/touch/ios/overflow-node-move-between-documents.html
+               fast/events/touch/ios/slider-node-move-between-documents.html
+               fast/events/touch/ios/touch-node-move-between-documents.html
+
+        * dom/EventNames.h:
+        (WebCore::EventNames::gestureEventNames):
+        * dom/Node.cpp:
+        (WebCore::Node::willBeDeletedFrom):
+        (WebCore::Node::didMoveToNewDocument):
+        (WebCore::tryAddEventListener):
+        (WebCore::tryRemoveEventListener):
+        * html/shadow/SliderThumbElement.cpp:
+        (WebCore::SliderThumbElement::registerForTouchEvents):
+        (WebCore::SliderThumbElement::unregisterForTouchEvents):
+        * rendering/RenderLayer.cpp:
+        (WebCore::RenderLayer::registerAsTouchEventListenerForScrolling):
+        (WebCore::RenderLayer::unregisterAsTouchEventListenerForScrolling):
+
 2017-04-03  Youenn Fablet  <you...@apple.com>
 
         captureStream is getting black frames with webgl canvas

Modified: trunk/Source/WebCore/dom/EventNames.h (214818 => 214819)


--- trunk/Source/WebCore/dom/EventNames.h	2017-04-03 17:40:06 UTC (rev 214818)
+++ trunk/Source/WebCore/dom/EventNames.h	2017-04-03 17:51:29 UTC (rev 214819)
@@ -319,6 +319,7 @@
 #endif
 
     std::array<std::reference_wrapper<const AtomicString>, 5> touchEventNames() const;
+    std::array<std::reference_wrapper<const AtomicString>, 3> gestureEventNames() const;
 
 private:
     EventNames(); // Private to prevent accidental call to EventNames() instead of eventNames().
@@ -359,6 +360,11 @@
     return { { touchstartEvent, touchmoveEvent, touchendEvent, touchcancelEvent, touchforcechangeEvent } };
 }
 
+inline std::array<std::reference_wrapper<const AtomicString>, 3> EventNames::gestureEventNames() const
+{
+    return { { gesturestartEvent, gesturechangeEvent, gestureendEvent } };
+}
+
 #if ENABLE(GAMEPAD)
 
 inline bool EventNames::isGamepadEventType(const AtomicString& eventType) const

Modified: trunk/Source/WebCore/dom/Node.cpp (214818 => 214819)


--- trunk/Source/WebCore/dom/Node.cpp	2017-04-03 17:40:06 UTC (rev 214818)
+++ trunk/Source/WebCore/dom/Node.cpp	2017-04-03 17:51:29 UTC (rev 214819)
@@ -315,15 +315,16 @@
 {
     if (hasEventTargetData()) {
         document.didRemoveWheelEventHandler(*this, EventHandlerRemoval::All);
-#if ENABLE(TOUCH_EVENTS) && PLATFORM(IOS)
-        document.removeTouchEventListener(this, true);
-#else
-        // FIXME: This should call didRemoveTouchEventHandler().
+#if ENABLE(TOUCH_EVENTS)
+#if PLATFORM(IOS)
+        document.removeTouchEventListener(*this, EventHandlerRemoval::All);
 #endif
+        document.didRemoveTouchEventHandler(*this, EventHandlerRemoval::All);
+#endif
     }
 
 #if ENABLE(TOUCH_EVENTS) && PLATFORM(IOS)
-    document.removeTouchEventHandler(this, true);
+    document.removeTouchEventHandler(*this, EventHandlerRemoval::All);
 #endif
 
     if (AXObjectCache* cache = document.existingAXObjectCache())
@@ -1919,15 +1920,31 @@
         document().didAddWheelEventHandler(*this);
     }
 
-    unsigned numTouchEventHandlers = 0;
+    unsigned numTouchEventListeners = 0;
     for (auto& name : eventNames().touchEventNames())
-        numTouchEventHandlers += eventListeners(name).size();
+        numTouchEventListeners += eventListeners(name).size();
 
-    for (unsigned i = 0; i < numTouchEventHandlers; ++i) {
+    for (unsigned i = 0; i < numTouchEventListeners; ++i) {
         oldDocument.didRemoveTouchEventHandler(*this);
         document().didAddTouchEventHandler(*this);
+
+#if ENABLE(TOUCH_EVENTS) && PLATFORM(IOS)
+        oldDocument.removeTouchEventListener(*this);
+        document().addTouchEventListener(*this);
+#endif
     }
 
+#if ENABLE(TOUCH_EVENTS) && ENABLE(IOS_GESTURE_EVENTS)
+    unsigned numGestureEventListeners = 0;
+    for (auto& name : eventNames().gestureEventNames())
+        numGestureEventListeners += eventListeners(name).size();
+
+    for (unsigned i = 0; i < numGestureEventListeners; ++i) {
+        oldDocument.removeTouchEventHandler(*this);
+        document().addTouchEventHandler(*this);
+    }
+#endif
+
     if (auto* registry = mutationObserverRegistry()) {
         for (auto& registration : *registry)
             document().addMutationObserverTypes(registration->mutationTypes());
@@ -1937,6 +1954,16 @@
         for (auto& registration : *transientRegistry)
             document().addMutationObserverTypes(registration->mutationTypes());
     }
+
+#if !ASSERT_DISABLED || ENABLE(SECURITY_ASSERTIONS)
+#if ENABLE(TOUCH_EVENTS) && PLATFORM(IOS)
+    ASSERT_WITH_SECURITY_IMPLICATION(!oldDocument.touchEventListenersContain(*this));
+    ASSERT_WITH_SECURITY_IMPLICATION(!oldDocument.touchEventHandlersContain(*this));
+#endif
+#if ENABLE(TOUCH_EVENTS) && ENABLE(IOS_GESTURE_EVENTS)
+    ASSERT_WITH_SECURITY_IMPLICATION(!oldDocument.touchEventTargetsContain(*this));
+#endif
+#endif
 }
 
 static inline bool tryAddEventListener(Node* targetNode, const AtomicString& eventType, Ref<EventListener>&& listener, const EventTarget::AddEventListenerOptions& options)
@@ -1963,13 +1990,13 @@
 
 #if ENABLE(TOUCH_EVENTS)
     if (eventNames().isTouchEventType(eventType))
-        targetNode->document().addTouchEventListener(targetNode);
+        targetNode->document().addTouchEventListener(*targetNode);
 #endif
 #endif // PLATFORM(IOS)
 
 #if ENABLE(IOS_GESTURE_EVENTS) && ENABLE(TOUCH_EVENTS)
     if (eventNames().isGestureEventType(eventType))
-        targetNode->document().addTouchEventHandler(targetNode);
+        targetNode->document().addTouchEventHandler(*targetNode);
 #endif
 
     return true;
@@ -2004,13 +2031,13 @@
 
 #if ENABLE(TOUCH_EVENTS)
     if (eventNames().isTouchEventType(eventType))
-        targetNode->document().removeTouchEventListener(targetNode);
+        targetNode->document().removeTouchEventListener(*targetNode);
 #endif
 #endif // PLATFORM(IOS)
 
 #if ENABLE(IOS_GESTURE_EVENTS) && ENABLE(TOUCH_EVENTS)
     if (eventNames().isGestureEventType(eventType))
-        targetNode->document().removeTouchEventHandler(targetNode);
+        targetNode->document().removeTouchEventHandler(*targetNode);
 #endif
 
     return true;

Modified: trunk/Source/WebCore/html/shadow/SliderThumbElement.cpp (214818 => 214819)


--- trunk/Source/WebCore/html/shadow/SliderThumbElement.cpp	2017-04-03 17:40:06 UTC (rev 214818)
+++ trunk/Source/WebCore/html/shadow/SliderThumbElement.cpp	2017-04-03 17:51:29 UTC (rev 214819)
@@ -549,7 +549,7 @@
 
     ASSERT(shouldAcceptTouchEvents());
 
-    document().addTouchEventHandler(this);
+    document().addTouchEventHandler(*this);
     m_isRegisteredAsTouchEventListener = true;
 }
 
@@ -561,7 +561,7 @@
     clearExclusiveTouchIdentifier();
     stopDragging();
 
-    document().removeTouchEventHandler(this);
+    document().removeTouchEventHandler(*this);
     m_isRegisteredAsTouchEventListener = false;
 }
 

Modified: trunk/Source/WebCore/rendering/RenderLayer.cpp (214818 => 214819)


--- trunk/Source/WebCore/rendering/RenderLayer.cpp	2017-04-03 17:40:06 UTC (rev 214818)
+++ trunk/Source/WebCore/rendering/RenderLayer.cpp	2017-04-03 17:51:29 UTC (rev 214819)
@@ -2194,7 +2194,7 @@
     if (!renderer().element() || m_registeredAsTouchEventListenerForScrolling)
         return;
     
-    renderer().document().addTouchEventHandler(renderer().element());
+    renderer().document().addTouchEventHandler(*renderer().element());
     m_registeredAsTouchEventListenerForScrolling = true;
 }
 
@@ -2203,7 +2203,7 @@
     if (!renderer().element() || !m_registeredAsTouchEventListenerForScrolling)
         return;
 
-    renderer().document().removeTouchEventHandler(renderer().element());
+    renderer().document().removeTouchEventHandler(*renderer().element());
     m_registeredAsTouchEventListenerForScrolling = false;
 }
 #endif // ENABLE(IOS_TOUCH_EVENTS)
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to