Branch: refs/heads/main
Home: https://github.com/WebKit/WebKit
Commit: 4a0a8eee54a492f3059110adfb08ed46ba356f5e
https://github.com/WebKit/WebKit/commit/4a0a8eee54a492f3059110adfb08ed46ba356f5e
Author: Sean Patterson <[email protected]>
Date: 2026-08-18 (Tue, 18 Aug 2026)
Changed paths:
A
LayoutTests/http/tests/site-isolation/scrolling/remove-iframe-with-active-scroll-proxy-expected.txt
A
LayoutTests/http/tests/site-isolation/scrolling/remove-iframe-with-active-scroll-proxy.html
A
LayoutTests/http/tests/site-isolation/scrolling/resources/scroll-proxy-iframe.html
M Source/WebCore/page/scrolling/ScrollingTree.cpp
M Source/WebCore/page/scrolling/ScrollingTree.h
M Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivateForTesting.h
M Source/WebKit/UIProcess/API/Cocoa/WKWebViewTesting.mm
M
Source/WebKit/UIProcess/RemoteLayerTree/RemoteScrollingCoordinatorProxy.cpp
M Source/WebKit/UIProcess/RemoteLayerTree/RemoteScrollingCoordinatorProxy.h
M Tools/TestRunnerShared/UIScriptContext/Bindings/UIScriptController.idl
M Tools/TestRunnerShared/UIScriptContext/UIScriptController.h
M Tools/TestWebKitAPI/Tests/WebKit/WKWebView/SiteIsolation.mm
M Tools/WebKitTestRunner/cocoa/UIScriptControllerCocoa.h
M Tools/WebKitTestRunner/cocoa/UIScriptControllerCocoa.mm
Log Message:
-----------
[Site isolation] Stale ScrollingTree active nodes crash the UI process in
establishLayerTreeScrollingRelations()
https://bugs.webkit.org/show_bug.cgi?id=321051
rdar://175191840
Reviewed by Matt Woodrow.
ScrollingTree keeps two active node sets, m_activePositionedNodes and
m_activeOverflowScrollProxyNodes, which hold Ref<>. Their only cleanup was a
frame-scoped removeIf in commitTreeStateInternal() that drops entries whose
frameIdentifier() matches the committing frame.
A node removed without its owning frame committing therefore leaked into those
sets permanently: the Ref keeps it alive while nodeForID() can no longer
resolve it. establishLayerTreeScrollingRelations() then walked the stale entry
and failed MESSAGE_CHECK_BASE. Under site isolation WebPageProxy sets
setShouldCrashOnMessageCheckFailure(true), so this terminated the UI process
rather than the web process.
The dominant path is hosted subtree teardown: every node in a hosted subtree is
registered as a hosted child, and ScrollingTreeFrameHostingNode::
removeHostedChildren() calls removeNode() for each one without the committing
frame ever being the iframe's own frame. Removing a cross-origin iframe,
navigating it cross-origin, or committing it empty all reach this.
Add ScrollingTree::removeFromActiveNodes(), called from removeNode(). It drops
the removed node itself, and also any active node that *references* a removed
overflow scrolling node -- the latter is what the failing check actually reads,
since it resolves the proxy's overflowScrollingNodeID() rather than the proxy
itself. removeAllNodes() clears both sets outright.
Two supporting changes were needed to make this testable at all:
- The active sets are only dumped under ScrollingStateTreeAsTextBehavior::
IncludeNodeIDs, which no test-reachable API passed. Add
scrollingTreeIncludingNodeIDsAsText() ->
-[WKWebView _scrollingTreeIncludingNodeIDsAsText] ->
uiController.scrollingTreeIncludingNodeIDsAsText.
- That dump sat inside #if ENABLE(SCROLLING_THREAD), which is mac-only, while
both sets are declared and populated unconditionally. Remove the guard so the
state is observable everywhere; without this the new layout test is a
permanent no-op off mac. The unrelated SCROLLING_THREAD guard around
synchronousScrollingNodes is left alone.
Note a single-process layout test cannot cover this: removing content from the
frame that owns the nodes makes that frame commit, so the frame-scoped removeIf
cleans up correctly. The test uses a cross-origin iframe so the removal happens
with no commit from the iframe's process.
Both tests were verified in both directions -- they pass with the fix and crash
without it. (On Release a MESSAGE_CHECK_BASE failure calls CRASH(), since
CRASH_IF_TESTING is only compiled out under ENABLE(IPC_TESTING_API).) Also
verified against the live repro: saratogasun.com -> Obituaries in a private
window with site isolation on crashed twice before the fix and survived eight
navigation cycles after.
*
LayoutTests/http/tests/site-isolation/scrolling/remove-iframe-with-active-scroll-proxy-expected.txt:
Added.
*
LayoutTests/http/tests/site-isolation/scrolling/remove-iframe-with-active-scroll-proxy.html:
Added.
*
LayoutTests/http/tests/site-isolation/scrolling/resources/scroll-proxy-iframe.html:
Added.
* Source/WebCore/page/scrolling/ScrollingTree.cpp:
(WebCore::ScrollingTree::removeNode):
(WebCore::ScrollingTree::removeFromActiveNodes):
(WebCore::ScrollingTree::removeAllNodes):
(WebCore::ScrollingTree::scrollingTreeAsText):
* Source/WebCore/page/scrolling/ScrollingTree.h:
* Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivateForTesting.h:
* Source/WebKit/UIProcess/API/Cocoa/WKWebViewTesting.mm:
(-[WKWebView _scrollingTreeIncludingNodeIDsAsText]):
* Source/WebKit/UIProcess/RemoteLayerTree/RemoteScrollingCoordinatorProxy.cpp:
(WebKit::RemoteScrollingCoordinatorProxy::scrollingTreeIncludingNodeIDsAsText
const):
* Source/WebKit/UIProcess/RemoteLayerTree/RemoteScrollingCoordinatorProxy.h:
* Tools/TestRunnerShared/UIScriptContext/Bindings/UIScriptController.idl:
* Tools/TestRunnerShared/UIScriptContext/UIScriptController.h:
(WTR::UIScriptController::scrollingTreeIncludingNodeIDsAsText const):
* Tools/TestWebKitAPI/Tests/WebKit/WKWebView/SiteIsolation.mm:
(TestWebKitAPI::TEST(SiteIsolation, RemoveIframeWithActiveScrollProxyNodes)):
* Tools/WebKitTestRunner/cocoa/UIScriptControllerCocoa.h:
* Tools/WebKitTestRunner/cocoa/UIScriptControllerCocoa.mm:
(WTR::UIScriptControllerCocoa::scrollingTreeIncludingNodeIDsAsText const):
Canonical link: https://commits.webkit.org/319425@main
To unsubscribe from these emails, change your notification settings at
https://github.com/WebKit/WebKit/settings/notifications
