RELEASE_ASSERT in WebCore: WebCore::ScrollingStateTree::insertNode()
authorsimon.fraser@apple.com <simon.fraser@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 4 Jul 2019 01:29:55 +0000 (01:29 +0000)
committersimon.fraser@apple.com <simon.fraser@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 4 Jul 2019 01:29:55 +0000 (01:29 +0000)
https://bugs.webkit.org/show_bug.cgi?id=199479
rdar://problem/52392556

Reviewed by Zalan Bujtas.
Source/WebCore:

Certain compositing tree updates could leave a layer with a ScrollingProxy role, but having an
AncestorClippingStack with no overflow scrolling layers - for example, a related scroller could become
scrollable, but we failed to mark the layer with the ancestor clippings stack as needing a geometry update.

When this happened updateScrollingNodeForScrollingProxyRole() would return 0, causing the next child to be
inserted with a parent of 0 (which should only happen for the root), and triggering a release assert in
ScrollingStateTree::insertNode().

Fix by ensuring that updateScrollingNodeForScrollingProxyRole() always returns the existing parentNodeID if we
don't have a new node to insert.

Test: scrollingcoordinator/scrolling-tree/scrolling-proxy-with-no-scrolling-layer.html

* rendering/RenderLayerBacking.cpp:
(WebCore::RenderLayerBacking::updateAncestorClippingStack):
* rendering/RenderLayerCompositor.cpp:
(WebCore::RenderLayerCompositor::updateScrollingNodeForScrollingProxyRole):

LayoutTests:

* scrollingcoordinator/scrolling-tree/scrolling-proxy-with-no-scrolling-layer-expected.txt: Added.
* scrollingcoordinator/scrolling-tree/scrolling-proxy-with-no-scrolling-layer.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/scrollingcoordinator/scrolling-tree/scrolling-proxy-with-no-scrolling-layer-expected.txt [new file with mode: 0644]
LayoutTests/scrollingcoordinator/scrolling-tree/scrolling-proxy-with-no-scrolling-layer.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/rendering/RenderLayerBacking.cpp
Source/WebCore/rendering/RenderLayerCompositor.cpp

index 3a982f8..8de37d3 100644 (file)
@@ -1,3 +1,14 @@
+2019-07-03  Simon Fraser  <simon.fraser@apple.com>
+
+        RELEASE_ASSERT in WebCore: WebCore::ScrollingStateTree::insertNode()
+        https://bugs.webkit.org/show_bug.cgi?id=199479
+        rdar://problem/52392556
+
+        Reviewed by Zalan Bujtas.
+
+        * scrollingcoordinator/scrolling-tree/scrolling-proxy-with-no-scrolling-layer-expected.txt: Added.
+        * scrollingcoordinator/scrolling-tree/scrolling-proxy-with-no-scrolling-layer.html: Added.
+
 2019-07-02  Myles C. Maxfield  <mmaxfield@apple.com>
 
         [WHLSL] Standard library is too big to directly include in WebCore
diff --git a/LayoutTests/scrollingcoordinator/scrolling-tree/scrolling-proxy-with-no-scrolling-layer-expected.txt b/LayoutTests/scrollingcoordinator/scrolling-tree/scrolling-proxy-with-no-scrolling-layer-expected.txt
new file mode 100644 (file)
index 0000000..46f0a4b
--- /dev/null
@@ -0,0 +1,3 @@
+This test should not trigger assertions or crash.
+
+
diff --git a/LayoutTests/scrollingcoordinator/scrolling-tree/scrolling-proxy-with-no-scrolling-layer.html b/LayoutTests/scrollingcoordinator/scrolling-tree/scrolling-proxy-with-no-scrolling-layer.html
new file mode 100644 (file)
index 0000000..7e99b24
--- /dev/null
@@ -0,0 +1,58 @@
+<!DOCTYPE html>
+<html>
+<head>
+    <style>
+        .scroller {
+            overflow-x: hidden;
+            height: 400px;
+            width: 400px;
+            border: 1px solid black;
+        }
+
+        .wrapper {
+            position: relative;
+        }
+
+        .absolute {
+            position: absolute;
+            top: 10px;
+            left: 20px;
+            width: 200px;
+            height: 200px;
+            background-color: green;
+            transform: translateZ(0);
+        }
+    
+        .content {
+            height: 300px;
+        }
+        
+        .content.changed {
+            height: 700px;
+        }
+    </style>
+    <script>
+        if (window.testRunner) {
+            testRunner.waitUntilDone();
+            testRunner.dumpAsText();
+        }
+        window.addEventListener('load', () => {
+            setTimeout(() => {
+                document.querySelector('.content').classList.add('changed');
+                if (window.testRunner)
+                    testRunner.notifyDone();
+            }, 0);
+        }, false);
+    </script>
+</head>
+<body>
+    <p>This test should not trigger assertions or crash.</p>
+    <div class="scroller">
+        <div class="wrapper">
+            <div class="content"></div>
+            <div class="absolute">
+            </div>
+        </div>
+    </div>
+</body>
+</html>
index 42e08ca..d062aa9 100644 (file)
@@ -1,3 +1,29 @@
+2019-07-03  Simon Fraser  <simon.fraser@apple.com>
+
+        RELEASE_ASSERT in WebCore: WebCore::ScrollingStateTree::insertNode()
+        https://bugs.webkit.org/show_bug.cgi?id=199479
+        rdar://problem/52392556
+
+        Reviewed by Zalan Bujtas.
+        
+        Certain compositing tree updates could leave a layer with a ScrollingProxy role, but having an
+        AncestorClippingStack with no overflow scrolling layers - for example, a related scroller could become
+        scrollable, but we failed to mark the layer with the ancestor clippings stack as needing a geometry update.
+
+        When this happened updateScrollingNodeForScrollingProxyRole() would return 0, causing the next child to be
+        inserted with a parent of 0 (which should only happen for the root), and triggering a release assert in
+        ScrollingStateTree::insertNode().
+
+        Fix by ensuring that updateScrollingNodeForScrollingProxyRole() always returns the existing parentNodeID if we
+        don't have a new node to insert.
+
+        Test: scrollingcoordinator/scrolling-tree/scrolling-proxy-with-no-scrolling-layer.html
+
+        * rendering/RenderLayerBacking.cpp:
+        (WebCore::RenderLayerBacking::updateAncestorClippingStack):
+        * rendering/RenderLayerCompositor.cpp:
+        (WebCore::RenderLayerCompositor::updateScrollingNodeForScrollingProxyRole):
+
 2019-07-03  Konstantin Tokarev  <annulen@yandex.ru>
 
         RenderLayerCompositor.cpp should include RenderImage.h
index 7aa820a..c00696e 100644 (file)
@@ -1575,17 +1575,17 @@ bool RenderLayerBacking::updateAncestorClippingStack(Vector<CompositedClipData>&
     
     if (!m_ancestorClippingStack) {
         m_ancestorClippingStack = std::make_unique<LayerAncestorClippingStack>(WTFMove(clippingData));
-        LOG_WITH_STREAM(Compositing, stream << "layer " << &m_owningLayer << "  ancestorClippingStack " << *m_ancestorClippingStack);
+        LOG_WITH_STREAM(Compositing, stream << "layer " << &m_owningLayer << " ancestorClippingStack " << *m_ancestorClippingStack);
         return true;
     }
     
     if (m_ancestorClippingStack->equalToClipData(clippingData)) {
-        LOG_WITH_STREAM(Compositing, stream << "layer " << &m_owningLayer << "  ancestorClippingStack " << *m_ancestorClippingStack);
+        LOG_WITH_STREAM(Compositing, stream << "layer " << &m_owningLayer << " ancestorClippingStack " << *m_ancestorClippingStack);
         return false;
     }
     
     m_ancestorClippingStack->updateWithClipData(scrollingCoordinator, WTFMove(clippingData));
-    LOG_WITH_STREAM(Compositing, stream << "layer " << &m_owningLayer << "  ancestorClippingStack " << *m_ancestorClippingStack);
+    LOG_WITH_STREAM(Compositing, stream << "layer " << &m_owningLayer << " ancestorClippingStack " << *m_ancestorClippingStack);
     return true;
 }
 
index 4c33d98..e9b5d94 100644 (file)
@@ -4483,8 +4483,10 @@ ScrollingNodeID RenderLayerCompositor::updateScrollingNodeForScrollingProxyRole(
 {
     auto* scrollingCoordinator = this->scrollingCoordinator();
     auto* clippingStack = layer.backing()->ancestorClippingStack();
-    if (!clippingStack)
-        return 0;
+    if (!clippingStack) {
+        ASSERT_NOT_REACHED();
+        return treeState.parentNodeID.valueOr(0);
+    }
 
     ScrollingNodeID nodeID = 0;
     for (auto& entry : clippingStack->stack()) {
@@ -4508,7 +4510,7 @@ ScrollingNodeID RenderLayerCompositor::updateScrollingNodeForScrollingProxyRole(
             auto overflowScrollNodeID = 0;
             if (auto* backing = entry.clipData.clippingLayer->backing())
                 overflowScrollNodeID = backing->scrollingNodeIDForRole(ScrollCoordinationRole::Scrolling);
-        
+
             Vector<ScrollingNodeID> scrollingNodeIDs;
             if (overflowScrollNodeID)
                 scrollingNodeIDs.append(overflowScrollNodeID);
@@ -4516,6 +4518,9 @@ ScrollingNodeID RenderLayerCompositor::updateScrollingNodeForScrollingProxyRole(
         }
     }
 
+    if (!nodeID)
+        return treeState.parentNodeID.valueOr(0);
+
     return nodeID;
 }