REGRESSION (r246725 ): Crashes on twitch.tv
authorsimon.fraser@apple.com <simon.fraser@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 24 Jun 2019 22:44:09 +0000 (22:44 +0000)
committersimon.fraser@apple.com <simon.fraser@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 24 Jun 2019 22:44:09 +0000 (22:44 +0000)
https://bugs.webkit.org/show_bug.cgi?id=199176
Source/WebCore:

rdar://problem/52071249

Reviewed by Zalan Bujtas.

With a composited negative z-index child inside a scroller, we can register the overflow scroll
proxy node before we've traversed the overflow layer, so it that layer hasn't got its OverflowScrollingNode
yet. Thus, AsyncScrollingCoordinator::setRelatedOverflowScrollingNodes() can be called with an empty vector.
Avoid crashing when this happens.

Test: scrollingcoordinator/scrolling-tree/scroller-with-negative-z-child.html

* page/scrolling/AsyncScrollingCoordinator.cpp:
(WebCore::AsyncScrollingCoordinator::setRelatedOverflowScrollingNodes):
* page/scrolling/cocoa/ScrollingTreeOverflowScrollProxyNode.mm:
(WebCore::ScrollingTreeOverflowScrollProxyNode::commitStateBeforeChildren):

LayoutTests:

Reviewed by Zalan Bujtas.

* scrollingcoordinator/scrolling-tree/scroller-with-negative-z-child-expected.txt: Added.
* scrollingcoordinator/scrolling-tree/scroller-with-negative-z-child.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/scrollingcoordinator/scrolling-tree/scroller-with-negative-z-child-expected.txt [new file with mode: 0644]
LayoutTests/scrollingcoordinator/scrolling-tree/scroller-with-negative-z-child.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/page/scrolling/AsyncScrollingCoordinator.cpp
Source/WebCore/page/scrolling/cocoa/ScrollingTreeOverflowScrollProxyNode.mm

index bbeccf1..2a5c776 100644 (file)
@@ -1,3 +1,13 @@
+2019-06-24  Simon Fraser  <simon.fraser@apple.com>
+
+        REGRESSION (r246725 ): Crashes on twitch.tv
+        https://bugs.webkit.org/show_bug.cgi?id=199176
+
+        Reviewed by Zalan Bujtas.
+
+        * scrollingcoordinator/scrolling-tree/scroller-with-negative-z-child-expected.txt: Added.
+        * scrollingcoordinator/scrolling-tree/scroller-with-negative-z-child.html: Added.
+
 2019-06-24  Alexey Shvayka  <shvaikalesh@gmail.com>
 
         Add Array.prototype.{flat,flatMap} to unscopables
diff --git a/LayoutTests/scrollingcoordinator/scrolling-tree/scroller-with-negative-z-child-expected.txt b/LayoutTests/scrollingcoordinator/scrolling-tree/scroller-with-negative-z-child-expected.txt
new file mode 100644 (file)
index 0000000..5ba3e62
--- /dev/null
@@ -0,0 +1,3 @@
+This test should not assert or crash
+
+
diff --git a/LayoutTests/scrollingcoordinator/scrolling-tree/scroller-with-negative-z-child.html b/LayoutTests/scrollingcoordinator/scrolling-tree/scroller-with-negative-z-child.html
new file mode 100644 (file)
index 0000000..01ed674
--- /dev/null
@@ -0,0 +1,48 @@
+<!DOCTYPE html> <!-- webkit-test-runner [ internal:AsyncOverflowScrollingEnabled=true ] -->
+<html>
+<head>
+    <style>
+               .scroller {
+                       position: relative;
+                       overflow-y: scroll;
+                       width: 300px;
+                       height: 300px;
+                       border: 1px solid black;
+               }
+               
+               .box {
+                       position: relative;
+                       margin: 20px;
+                   width: 100px;
+                   height: 100px;
+                   background-color: green;
+               }
+
+               .inside {
+                       left: 20px;
+               }
+               
+               .negative {
+                       z-index: -1;
+                   transform: translateZ(0);
+               }
+
+               .spacer {
+                   width: 40px;
+                   height: 500px;
+                   background-color: silver;
+               }
+    </style>
+       <script>
+               if (window.testRunner)
+                       testRunner.dumpAsText();
+       </script>
+</head>
+<body>
+    <p>This test should not assert or crash</p>
+       <div class="scroller">
+        <div class="inside negative box"></div>
+        <div class="spacer"></div>
+       </div>
+</body>
+</html>
index 44bf9b6..f1f54a5 100644 (file)
@@ -1,3 +1,23 @@
+2019-06-24  Simon Fraser  <simon.fraser@apple.com>
+
+        REGRESSION (r246725 ): Crashes on twitch.tv
+        https://bugs.webkit.org/show_bug.cgi?id=199176
+        rdar://problem/52071249
+
+        Reviewed by Zalan Bujtas.
+        
+        With a composited negative z-index child inside a scroller, we can register the overflow scroll
+        proxy node before we've traversed the overflow layer, so it that layer hasn't got its OverflowScrollingNode
+        yet. Thus, AsyncScrollingCoordinator::setRelatedOverflowScrollingNodes() can be called with an empty vector.
+        Avoid crashing when this happens.
+
+        Test: scrollingcoordinator/scrolling-tree/scroller-with-negative-z-child.html
+
+        * page/scrolling/AsyncScrollingCoordinator.cpp:
+        (WebCore::AsyncScrollingCoordinator::setRelatedOverflowScrollingNodes):
+        * page/scrolling/cocoa/ScrollingTreeOverflowScrollProxyNode.mm:
+        (WebCore::ScrollingTreeOverflowScrollProxyNode::commitStateBeforeChildren):
+
 2019-06-24  Chris Dumez  <cdumez@apple.com>
 
         Pages using Google's anti-flicker optimization may take ~5 seconds to do initial paint
index 509b62d..ca68f76 100644 (file)
@@ -715,8 +715,10 @@ void AsyncScrollingCoordinator::setRelatedOverflowScrollingNodes(ScrollingNodeID
         downcast<ScrollingStatePositionedNode>(node)->setRelatedOverflowScrollingNodes(WTFMove(relatedNodes));
     else if (is<ScrollingStateOverflowScrollProxyNode>(node)) {
         auto* overflowScrollProxyNode = downcast<ScrollingStateOverflowScrollProxyNode>(node);
-        ASSERT(relatedNodes.size() == 1);
-        overflowScrollProxyNode->setOverflowScrollingNode(relatedNodes[0]);
+        if (!relatedNodes.isEmpty())
+            overflowScrollProxyNode->setOverflowScrollingNode(relatedNodes[0]);
+        else
+            overflowScrollProxyNode->setOverflowScrollingNode(0);
     } else
         ASSERT_NOT_REACHED();
 }
index 94cd6a3..d5f3d14 100644 (file)
@@ -57,10 +57,12 @@ void ScrollingTreeOverflowScrollProxyNode::commitStateBeforeChildren(const Scrol
     if (overflowProxyStateNode.hasChangedProperty(ScrollingStateOverflowScrollProxyNode::OverflowScrollingNode))
         m_overflowScrollingNodeID = overflowProxyStateNode.overflowScrollingNode();
 
-    auto& relatedNodes = scrollingTree().overflowRelatedNodes();
-    relatedNodes.ensure(m_overflowScrollingNodeID, [] {
-        return Vector<ScrollingNodeID>();
-    }).iterator->value.append(scrollingNodeID());
+    if (m_overflowScrollingNodeID) {
+        auto& relatedNodes = scrollingTree().overflowRelatedNodes();
+        relatedNodes.ensure(m_overflowScrollingNodeID, [] {
+            return Vector<ScrollingNodeID>();
+        }).iterator->value.append(scrollingNodeID());
+    }
 
     scrollingTree().nodesWithRelatedOverflow().add(scrollingNodeID());
 }