Attachment 373433 Details for Bug 199479 – Patch

[patch] Patch

bug-199479-20190703163839.patch (text/plain), 7.93 KB, created by Simon Fraser (smfr) on 2019-07-03 16:38:39 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Simon Fraser (smfr)

Created: 2019-07-03 16:38:39 PDT

Size: 7.93 KB

patch

obsolete

>Subversion Revision: 247095
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index a8fe580fa13d35396845b1a3202ccbd9a3737afe..d5456a98097aee6905681cbbd152210dd8a68aeb 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -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 NOBODY (OOPS!).
>+        
>+        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  Ryan Haddad  <ryanhaddad@apple.com>
> 
>         Unreviewed, rolling out r246616.
>diff --git a/Source/WebCore/rendering/RenderLayerBacking.cpp b/Source/WebCore/rendering/RenderLayerBacking.cpp
>index 7aa820a619799f6ca3f660f15dbc3ff3590d8127..c00696ec7d80649635568a04cc8811a03b2973be 100644
>--- a/Source/WebCore/rendering/RenderLayerBacking.cpp
>+++ b/Source/WebCore/rendering/RenderLayerBacking.cpp
>@@ -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;
> }
> 
>diff --git a/Source/WebCore/rendering/RenderLayerCompositor.cpp b/Source/WebCore/rendering/RenderLayerCompositor.cpp
>index 9060820ae90c3de9307c71a9af04c403a5f51a94..9f4dbc94668fe5b4aea06c870847e6097524f6e7 100644
>--- a/Source/WebCore/rendering/RenderLayerCompositor.cpp
>+++ b/Source/WebCore/rendering/RenderLayerCompositor.cpp
>@@ -4482,8 +4482,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()) {
>@@ -4507,7 +4509,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);
>@@ -4515,6 +4517,9 @@ ScrollingNodeID RenderLayerCompositor::updateScrollingNodeForScrollingProxyRole(
>         }
>     }
> 
>+    if (!nodeID)
>+        return treeState.parentNodeID.valueOr(0);
>+
>     return nodeID;
> }
> 
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index 98225e1bb3ec9c4e08b842ab10ab9d5971ad5b10..005a268c167065027c7c636a78af639ae5f83dd7 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -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 NOBODY (OOPS!).
>+
>+        * 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-03  Andres Gonzalez  <andresg_22@apple.com>
> 
>         Safari hanging while loading pages - WebCore::AccessibilityRenderObject::visiblePositionRangeForLine.
>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
>index 0000000000000000000000000000000000000000..46f0a4b7c8fe8b8abeb191c14600085a7ae0c683
>--- /dev/null
>+++ b/LayoutTests/scrollingcoordinator/scrolling-tree/scrolling-proxy-with-no-scrolling-layer-expected.txt
>@@ -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
>index 0000000000000000000000000000000000000000..7e99b24fb0668ddd6b027ca6a5ee7060c6f303b6
>--- /dev/null
>+++ b/LayoutTests/scrollingcoordinator/scrolling-tree/scrolling-proxy-with-no-scrolling-layer.html
>@@ -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>

Actions: View | Formatted Diff | Diff

Attachments on bug 199479: 373433