Branch: refs/heads/main
Home: https://github.com/WebKit/WebKit
Commit: a13640cea30b69809a8ad998a5ccf27420a94a1f
https://github.com/WebKit/WebKit/commit/a13640cea30b69809a8ad998a5ccf27420a94a1f
Author: Wenson Hsieh <[email protected]>
Date: 2025-07-07 (Mon, 07 Jul 2025)
Changed paths:
A
LayoutTests/compositing/scrolling/async-overflow-scrolling/sticky-in-overflow-scroller-expected.html
A
LayoutTests/compositing/scrolling/async-overflow-scrolling/sticky-in-overflow-scroller.html
M Source/WebCore/page/scrolling/ScrollingStateStickyNode.cpp
M Source/WebCore/page/scrolling/ScrollingTreeStickyNode.cpp
M Source/WebCore/page/scrolling/ScrollingTreeStickyNode.h
M Source/WebCore/page/scrolling/cocoa/ScrollingTreeStickyNodeCocoa.h
M Source/WebCore/page/scrolling/cocoa/ScrollingTreeStickyNodeCocoa.mm
M
Source/WebCore/page/scrolling/coordinated/ScrollingTreeStickyNodeCoordinated.cpp
M Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm
M
Source/WebKit/UIProcess/RemoteLayerTree/RemoteScrollingCoordinatorProxy.cpp
M Source/WebKit/UIProcess/RemoteLayerTree/RemoteScrollingCoordinatorProxy.h
M Source/WebKit/UIProcess/WebPageProxy.cpp
M Source/WebKit/UIProcess/WebPageProxyInternals.h
Log Message:
-----------
REGRESSION (296901@main): Debug assertion under
`ScrollingTreeStickyNode::findConstrainingRect` when opening Web Inspector
https://bugs.webkit.org/show_bug.cgi?id=295533
rdar://155225837
Reviewed by Abrar Rahman Protyasha.
After the changes in 296901@main, opening Web Inspector with a debug macOS
build of WebKit causes an
instant debug assertion to be hit under `findConstrainingRect()`. This happens
because:
1. The logic to `updateIsSticking` currently resides in
`commitStateBeforeChildren`.
2. `updateIsSticking` indirectly calls into `findConstrainingRect`, which may
need to look up
enclosing UI-side scrolling tree scrolling nodes by ID. If the enclosing
scrolling node ID fails
to map to its corresponding scrolling node in the tree, we end up hitting
this debug assert.
3. Scrolling tree scrolling nodes are registered (by ID) with the UI-side
scrolling tree when
committing tree state.
…so in the case where we've already visited (i.e. committed state for) the
enclosing scroller of a
sticky node by the time we visit the sticky node, the lookup in (2) succeeds,
and we don't assert.
However, in the case where the sticky node is *before* the enclosing scrolling
node in tree order,
we'll crash with the debug assert because (3) hasn't happened by the time we
attempt to commit state
for the sticky node.
To fix this, we move logic to update the "sticking" state out of
`commitStateBeforeChildren`, and
instead move it into `applyLayerPositions` (the same codepath that handles
UI-side layer blitting
when scrolling). We also ensure that at most 1 IPC message is sent back to the
web page per main-
frame rendering update, by moving the batching mechanism onto `WebPageProxy`.
*
LayoutTests/compositing/scrolling/async-overflow-scrolling/sticky-in-overflow-scroller-expected.html:
Added.
*
LayoutTests/compositing/scrolling/async-overflow-scrolling/sticky-in-overflow-scroller.html:
Added.
Add a layout test to exercise the assertion; the key piece here is to have
sticky nodes with a
negative initial `z-index` relative to its parent overflow scroller. This
ensures that when
traversing the scrolling tree during the scroll commit, we'll visit the sticky
node *before*
visiting its enclosing overflow scroller.
* Source/WebCore/page/scrolling/ScrollingStateStickyNode.cpp:
(WebCore::ScrollingStateStickyNode::computeAnchorLayerPosition const):
* Source/WebCore/page/scrolling/ScrollingTreeStickyNode.cpp:
(WebCore::ScrollingTreeStickyNode::commitStateBeforeChildren):
(WebCore::ScrollingTreeStickyNode::computeConstrainingRectAndAnchorLayerPosition
const):
Rename `computeAnchorLayerPosition` to
`computeConstrainingRectAndAnchorLayerPosition`, and make it
return both the constraining rect as well as the anchor layer position. The
call site in
(WebCore::ScrollingTreeStickyNode::scrollDeltaSinceLastCommit const):
(WebCore::ScrollingTreeStickyNode::isCurrentlySticking const):
Split this into a second method that takes a precomputed `constrainingRect`,
which we can call from
`applyLayerPositions()` below to avoid redundant work to compute the
constraining rect.
(WebCore::ScrollingTreeStickyNode::updateIsSticking): Deleted.
Move this into `ScrollingTreeStickyNodeCocoa` as `setIsSticking`, and make it
take a `bool`; see
below.
(WebCore::ScrollingTreeStickyNode::computeAnchorLayerPosition const): Deleted.
* Source/WebCore/page/scrolling/ScrollingTreeStickyNode.h:
* Source/WebCore/page/scrolling/cocoa/ScrollingTreeStickyNodeCocoa.h:
* Source/WebCore/page/scrolling/cocoa/ScrollingTreeStickyNodeCocoa.mm:
(WebCore::ScrollingTreeStickyNodeCocoa::applyLayerPositions):
Move logic to notify the scrolling tree when a sticky scrolling node has begun
to stick out of
`commitStateBeforeChildren` and into `applyLayerPositions` instead. This allows
us to:
1. Fix the bug by ensuring that `findConstrainingRect()` is only called when
the scrolling tree
nodes have already finished attaching.
2. Avoid having to recompute the constraining rect, since
`applyLayerPositions` already computes it
in the process of repositioning the anchor/clipping layer.
3. Ensure that we still observe that sticky scrolling nodes begin sticking
during UI-side
scrolling, without having to wait for layout in the web process to
reconcile layer positions for
scrolling state nodes.
(WebCore::ScrollingTreeStickyNodeCocoa::setIsSticking):
Use `ensureOnMainRunLoop` here to make sure we don't try to mutate any state
downstream on the
scrolling thread, and instead hop to the main thread if needed.
*
Source/WebCore/page/scrolling/coordinated/ScrollingTreeStickyNodeCoordinated.cpp:
(WebCore::ScrollingTreeStickyNodeCoordinated::applyLayerPositions):
* Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:
(WebKit::WebPageProxy::didCommitLayerTree):
* Source/WebKit/UIProcess/RemoteLayerTree/RemoteScrollingCoordinatorProxy.cpp:
(WebKit::RemoteScrollingCoordinatorProxy::commitScrollingTreeState):
(WebKit::RemoteScrollingCoordinatorProxy::stickyScrollingTreeNodeBeganSticking):
Remove `m_stickyScrollingTreeNodesBeganSticking` here, and instead add a flag
to `WebPageProxy`
(below) to send at most one IPC message to invalidate
`m_needsFixedContainerEdgesUpdate`, even if
a large number of sticky scrolling nodes begin sticking during the same
rendering update.
* Source/WebKit/UIProcess/RemoteLayerTree/RemoteScrollingCoordinatorProxy.h:
* Source/WebKit/UIProcess/WebPageProxy.cpp:
(WebKit::WebPageProxy::resetState):
(WebKit::WebPageProxy::stickyScrollingTreeNodeBeganSticking):
* Source/WebKit/UIProcess/WebPageProxyInternals.h:
Canonical link: https://commits.webkit.org/297097@main
To unsubscribe from these emails, change your notification settings at
https://github.com/WebKit/WebKit/settings/notifications
_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes
