Asynchronous scrolling of overflow element can enter a recursive loop
authorcathiechen@igalia.com <cathiechen@igalia.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 31 Jan 2020 17:24:37 +0000 (17:24 +0000)
committercathiechen@igalia.com <cathiechen@igalia.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 31 Jan 2020 17:24:37 +0000 (17:24 +0000)
https://bugs.webkit.org/show_bug.cgi?id=206884

Reviewed by Frédéric Wang.

Source/WebCore:

After implementing RenderLayer::requestScrollPositionUpdate, there's a recursive loop
while performing asynchronous programmatic scrolling for overflow elements. In order to break
the loop call notifyScrollPositionChanged in updateScrollPositionAfterAsyncScroll instead.

Test: fast/scrolling/ios/programmatic-scroll-element-crash.html

* page/scrolling/AsyncScrollingCoordinator.cpp:
(WebCore::AsyncScrollingCoordinator::updateScrollPositionAfterAsyncScroll):
* rendering/RenderLayer.cpp:
(WebCore::RenderLayer::requestScrollPositionUpdate):
(WebCore::RenderLayer::scrollToOffset):
* rendering/RenderLayer.h:

LayoutTests:

* fast/scrolling/ios/programmatic-scroll-element-crash-expected.txt: Added.
* fast/scrolling/ios/programmatic-scroll-element-crash.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/fast/scrolling/ios/programmatic-scroll-element-crash-expected.txt [new file with mode: 0644]
LayoutTests/fast/scrolling/ios/programmatic-scroll-element-crash.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp
Source/WebCore/rendering/RenderLayer.cpp
Source/WebCore/rendering/RenderLayer.h

index d48638b..faaa531 100644 (file)
@@ -1,3 +1,13 @@
+2020-01-31  Cathie Chen  <cathiechen@igalia.com>
+
+        Asynchronous scrolling of overflow element can enter a recursive loop
+        https://bugs.webkit.org/show_bug.cgi?id=206884
+
+        Reviewed by Frédéric Wang.
+
+        * fast/scrolling/ios/programmatic-scroll-element-crash-expected.txt: Added.
+        * fast/scrolling/ios/programmatic-scroll-element-crash.html: Added.
+
 2020-01-31  Jacob Uphoff  <jacob_uphoff@apple.com>
 
         [ macOSwk1 ] imported/w3c/web-platform-tests/html/semantics/embedded-content/media-elements/seeking/seek-to-currentTime.html is flaky failing
diff --git a/LayoutTests/fast/scrolling/ios/programmatic-scroll-element-crash-expected.txt b/LayoutTests/fast/scrolling/ios/programmatic-scroll-element-crash-expected.txt
new file mode 100644 (file)
index 0000000..1042c76
--- /dev/null
@@ -0,0 +1,2 @@
+Test passes if it does not crash.
+
diff --git a/LayoutTests/fast/scrolling/ios/programmatic-scroll-element-crash.html b/LayoutTests/fast/scrolling/ios/programmatic-scroll-element-crash.html
new file mode 100644 (file)
index 0000000..0334411
--- /dev/null
@@ -0,0 +1,20 @@
+<!DOCTYPE html> <!-- webkit-test-runner [ internal:AsyncOverflowScrollingEnabled=true ]-->
+<meta name="viewport" content="width=device-width, initial-scale=1">
+<style>
+  .scrollable {
+    overflow: auto;
+    width: 400px;
+    height: 200px;
+  }
+</style>
+Test passes if it does not crash.
+<div id="overflowNode" class="scrollable">
+  <div style="width: 2000px; height: 1000px;">
+  </div>
+</div>
+<script>
+  if (window.testRunner)
+      testRunner.dumpAsText();
+  document.getElementById('overflowNode').scrollTo(500, 250);
+</script>
+</html>
index 68a65e4..a1ff602 100644 (file)
@@ -1,3 +1,23 @@
+2020-01-31  Cathie Chen  <cathiechen@igalia.com>
+
+        Asynchronous scrolling of overflow element can enter a recursive loop
+        https://bugs.webkit.org/show_bug.cgi?id=206884
+
+        Reviewed by Frédéric Wang.
+
+        After implementing RenderLayer::requestScrollPositionUpdate, there's a recursive loop
+        while performing asynchronous programmatic scrolling for overflow elements. In order to break
+        the loop call notifyScrollPositionChanged in updateScrollPositionAfterAsyncScroll instead.
+
+        Test: fast/scrolling/ios/programmatic-scroll-element-crash.html
+
+        * page/scrolling/AsyncScrollingCoordinator.cpp:
+        (WebCore::AsyncScrollingCoordinator::updateScrollPositionAfterAsyncScroll):
+        * rendering/RenderLayer.cpp:
+        (WebCore::RenderLayer::requestScrollPositionUpdate):
+        (WebCore::RenderLayer::scrollToOffset):
+        * rendering/RenderLayer.h:
+
 2020-01-31  Antti Koivisto  <antti@apple.com>
 
         REGRESSION (r240250): Pages using smoothscroll.js can't be scrolled with trackpad
index 7aa75bd..d5797ba 100644 (file)
@@ -359,7 +359,7 @@ void AsyncScrollingCoordinator::updateScrollPositionAfterAsyncScroll(ScrollingNo
     if (auto* scrollableArea = frameView.scrollableAreaForScrollLayerID(scrollingNodeID)) {
         auto previousScrollType = scrollableArea->currentScrollType();
         scrollableArea->setCurrentScrollType(scrollType);
-        scrollableArea->scrollToOffsetWithoutAnimation(ScrollableArea::scrollOffsetFromPosition(scrollPosition, toFloatSize(scrollableArea->scrollOrigin())));
+        scrollableArea->notifyScrollPositionChanged(roundedIntPoint(scrollPosition));
         scrollableArea->setCurrentScrollType(previousScrollType);
 
         if (scrollingLayerPositionAction == ScrollingLayerPositionAction::Set)
index e85a15c..40e0fd5 100644 (file)
@@ -2596,6 +2596,15 @@ ScrollOffset RenderLayer::clampScrollOffset(const ScrollOffset& scrollOffset) co
     return scrollOffset.constrainedBetween(IntPoint(), maximumScrollOffset());
 }
 
+bool RenderLayer::requestScrollPositionUpdate(const ScrollPosition& position, ScrollType scrollType, ScrollClamping clamping)
+{
+#if ENABLE(ASYNC_SCROLLING)
+    if (ScrollingCoordinator* scrollingCoordinator = page().scrollingCoordinator())
+        return scrollingCoordinator->requestScrollPositionUpdate(*this, position, scrollType, clamping);
+#endif
+    return false;
+}
+
 void RenderLayer::scrollToOffset(const ScrollOffset& scrollOffset, ScrollType scrollType, ScrollClamping clamping)
 {
     ScrollOffset clampedScrollOffset = clamping == ScrollClamping::Clamped ? clampScrollOffset(scrollOffset) : scrollOffset;
@@ -2605,13 +2614,7 @@ void RenderLayer::scrollToOffset(const ScrollOffset& scrollOffset, ScrollType sc
     auto previousScrollType = currentScrollType();
     setCurrentScrollType(scrollType);
 
-    bool handled = false;
-#if ENABLE(ASYNC_SCROLLING)
-    if (ScrollingCoordinator* scrollingCoordinator = page().scrollingCoordinator())
-        handled = scrollingCoordinator->requestScrollPositionUpdate(*this, scrollPositionFromOffset(clampedScrollOffset), scrollType, clamping);
-#endif
-
-    if (!handled)
+    if (!requestScrollPositionUpdate(scrollPositionFromOffset(clampedScrollOffset), scrollType, clamping))
         scrollToOffsetWithoutAnimation(clampedScrollOffset, clamping);
 
     setCurrentScrollType(previousScrollType);
index dcc345d..6286e09 100644 (file)
@@ -439,6 +439,8 @@ public:
     // Scrolling methods for layers that can scroll their overflow.
     void scrollByRecursively(const IntSize& delta, ScrollableArea** scrolledArea = nullptr);
 
+    bool requestScrollPositionUpdate(const ScrollPosition&, ScrollType = ScrollType::User, ScrollClamping = ScrollClamping::Clamped) override;
+
     WEBCORE_EXPORT void scrollToOffset(const ScrollOffset&, ScrollType = ScrollType::Programmatic, ScrollClamping = ScrollClamping::Clamped);
 
     void scrollToXPosition(int x, ScrollType, ScrollClamping = ScrollClamping::Clamped);