From d1ea03f420faa0db0b9293ff5e4356ba1cb49764 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mart=C3=ADn=20Lucas=20Golini?= Date: Tue, 13 Oct 2026 00:17:21 -0300 Subject: [PATCH] fix(ui): clear stale dirty queues when widgets change scenes Remove pending layout, style and style-state entries from the old scene during rebinding without invalidating shared accessibility identities. Requeue dirty layouts in the destination scene while preserving their invalidation reasons. Fix a use-after-free exposed by TabFocus.WidgetCreatorDefaultsAreIntentional after deleting a WebView. Add regression coverage for WebView deletion, subtree and reverse rebinding, and coalesced layout work. --- include/eepp/ui/uilayout.hpp | 2 + include/eepp/ui/uiscenenode.hpp | 4 + src/eepp/ui/uilayout.cpp | 9 ++ src/eepp/ui/uiscenenode.cpp | 17 +-- src/eepp/ui/uiwidget.cpp | 5 + src/tests/unit_tests/uiscenenode_tests.cpp | 125 +++++++++++++++++++++ 6 files changed, 154 insertions(+), 8 deletions(-) diff --git a/include/eepp/ui/uilayout.hpp b/include/eepp/ui/uilayout.hpp index d6d36081c..bf3c00857 100644 --- a/include/eepp/ui/uilayout.hpp +++ b/include/eepp/ui/uilayout.hpp @@ -79,6 +79,8 @@ class EE_API UILayout : public UIWidget { virtual void onLayoutUpdate(); + virtual void onUISceneNodeChange( UISceneNode* scene ); + virtual void tryUpdateLayout(); virtual void updateLayoutTree(); diff --git a/include/eepp/ui/uiscenenode.hpp b/include/eepp/ui/uiscenenode.hpp index d12236df7..aec073e7a 100644 --- a/include/eepp/ui/uiscenenode.hpp +++ b/include/eepp/ui/uiscenenode.hpp @@ -1346,6 +1346,10 @@ class EE_API UISceneNode : public SceneNode { */ void onWidgetDelete( Node* node ); + /** Removes this scene's queued widget work without invalidating accessibility identities. + * Used on deletion and before a widget changes its owning UI scene. */ + void removeWidgetFromDirtyQueues( UIWidget* widget ); + void onWidgetAccessibilitySourceDelete( UIWidget* widget ); /** The manager this scene's widgets report to, or nullptr when none exists yet. Never creates diff --git a/src/eepp/ui/uilayout.cpp b/src/eepp/ui/uilayout.cpp index 9834020d4..d4df038e6 100644 --- a/src/eepp/ui/uilayout.cpp +++ b/src/eepp/ui/uilayout.cpp @@ -51,6 +51,15 @@ void UILayout::onLayoutUpdate() { sendCommonEvent( Event::OnLayoutUpdate ); } +void UILayout::onUISceneNodeChange( UISceneNode* scene ) { + UIWidget::onUISceneNodeChange( scene ); + // The old scene no longer owns this pending pass. Preserve its reasons in the new scene + // rather than leaving mDirtyLayout set with no queue entry to process it. A dirty ancestor + // may have coalesced this layout's old entry, so use its flag instead of queue membership. + if ( scene && mDirtyLayout ) + scene->invalidateLayout( this, mDirtyReasons ); +} + const Sizef& UILayout::getSize() const { if ( mDirtyLayout ) const_cast( this )->updateLayout(); diff --git a/src/eepp/ui/uiscenenode.cpp b/src/eepp/ui/uiscenenode.cpp index 5a5be9a2c..7799b2ba5 100644 --- a/src/eepp/ui/uiscenenode.cpp +++ b/src/eepp/ui/uiscenenode.cpp @@ -1378,17 +1378,18 @@ void UISceneNode::onWidgetDelete( Node* node ) { manager->onWidgetDelete( widget ); } - if ( node->isLayout() ) { - mDirtyLayouts.erase( node->asType() ); - } - - mDirtyStyle.erase( widget ); - - mDirtyStyleState.erase( widget ); - mDirtyStyleStateCSSAnimations.erase( widget ); + removeWidgetFromDirtyQueues( widget ); } } +void UISceneNode::removeWidgetFromDirtyQueues( UIWidget* widget ) { + if ( widget->isLayout() ) + mDirtyLayouts.erase( widget->asType() ); + mDirtyStyle.erase( widget ); + mDirtyStyleState.erase( widget ); + mDirtyStyleStateCSSAnimations.erase( widget ); +} + void UISceneNode::onWidgetAccessibilitySourceDelete( UIWidget* widget ) { if ( mHostUISceneNode ) { mHostUISceneNode->onWidgetAccessibilitySourceDelete( widget ); diff --git a/src/eepp/ui/uiwidget.cpp b/src/eepp/ui/uiwidget.cpp index f4b942e98..dc4a44f1d 100644 --- a/src/eepp/ui/uiwidget.cpp +++ b/src/eepp/ui/uiwidget.cpp @@ -2195,6 +2195,11 @@ void UIWidget::onMarginChange() { void UIWidget::onThemeLoaded() {} void UIWidget::onUISceneNodeChange( UISceneNode* scene ) { + // Dirty queues belong to the local scene, even when embedded scenes share an accessibility + // manager. Otherwise deletion in the new scene leaves stale pointers queued in the old one. + if ( mUISceneNode ) + mUISceneNode->removeWidgetFromDirtyQueues( this ); + // Lifecycle bookkeeping never creates a manager, even with accessibility disabled. auto* previousManager = mUISceneNode ? mUISceneNode->getExistingAccessibilityManager() : nullptr; diff --git a/src/tests/unit_tests/uiscenenode_tests.cpp b/src/tests/unit_tests/uiscenenode_tests.cpp index 2b61097f5..c07632992 100644 --- a/src/tests/unit_tests/uiscenenode_tests.cpp +++ b/src/tests/unit_tests/uiscenenode_tests.cpp @@ -14,6 +14,7 @@ #include #include #include +#include #include #include #include @@ -21,6 +22,7 @@ #include #include #include +#include #include #include #include @@ -630,6 +632,18 @@ class InvalidationTestSceneNode : public UISceneNode { size_t pendingLayoutCount() const { return mDirtyLayouts.size(); } + bool hasPendingLayout( UILayout* layout ) const { return mDirtyLayouts.count( layout ) != 0; } + + bool hasPendingStyle( UIWidget* widget ) const { return mDirtyStyle.count( widget ) != 0; } + + bool hasPendingStyleState( UIWidget* widget ) const { + return mDirtyStyleState.count( widget ) != 0; + } + + bool hasPendingStyleStateAnimations( UIWidget* widget ) const { + return mDirtyStyleStateCSSAnimations.count( widget ) != 0; + } + size_t processedStyleRootCount() const { return mDirtyStylesSnapshot.size(); } UIWidget* processedStyleRoot( size_t index ) const { return mDirtyStylesSnapshot[index].first; } @@ -1005,6 +1019,117 @@ class InvalidationTestLayout : public UILayout { LayoutInvalidationFlags lastReasons{ 0 }; }; +UTEST( UISceneNode, WebViewDeletionDoesNotLeaveHostLayoutWork ) { + Engine::instance()->createWindow( WindowSettings( 320, 240, "WebView Dirty Queue Ownership", + WindowStyle::Default | WindowStyle::Hidden, + WindowBackend::Default, 32, {}, 1, false, + true ), + ContextSettings( false, 0, 0, GLv_default, true, false ) ); + auto* host = InvalidationTestSceneNode::New(); + init_test_scene_node( host ); + host->setAccessibilityPolicy( AccessibilityPolicy::Disabled ); + host->flushDirtyStyleAndLayout(); + + auto* webView = UIWebView::New(); + auto* document = webView->getDocumentSceneNode()->getRoot()->findByTag( "webview::doc" ); + ASSERT_TRUE( document != nullptr ); + ASSERT_TRUE( document->isLayout() ); + auto* layout = document->asType(); + EXPECT_EQ( layout->getUISceneNode(), webView->getDocumentSceneNode() ); + EXPECT_FALSE( host->hasPendingLayout( layout ) ); + EXPECT_FALSE( host->hasPendingStyle( layout ) ); + EXPECT_FALSE( host->hasPendingStyleState( layout ) ); + EXPECT_FALSE( host->hasPendingStyleStateAnimations( layout ) ); + + eeDelete( webView ); + // Compare queue membership only: the document pointer is no longer dereferenceable. + const bool staleLayout = host->hasPendingLayout( layout ); + EXPECT_FALSE( staleLayout ); + EXPECT_FALSE( host->hasPendingStyle( layout ) ); + EXPECT_FALSE( host->hasPendingStyleState( layout ) ); + EXPECT_FALSE( host->hasPendingStyleStateAnimations( layout ) ); + // Keep the ownership failure readable when deliberately testing without the fix. + if ( !staleLayout ) { + auto* input = UIHTMLInput::New(); + EXPECT_EQ( input->getUISceneNode(), host ); + EXPECT_TRUE( input->getChildWidget() != nullptr ); + host->flushDirtyStyleAndLayout(); + } + + Engine::destroySingleton(); +} + +UTEST( UISceneNode, DirtyLayoutAndStyleWorkFollowsSceneRebinding ) { + Engine::instance()->createWindow( WindowSettings( 320, 240, "Dirty Queue Scene Rebinding", + WindowStyle::Default | WindowStyle::Hidden, + WindowBackend::Default, 32, {}, 1, false, + true ), + ContextSettings( false, 0, 0, GLv_default, true, false ) ); + auto* host = InvalidationTestSceneNode::New(); + init_test_scene_node( host ); + host->setAccessibilityPolicy( AccessibilityPolicy::Disabled ); + auto* embedded = InvalidationTestSceneNode::New(); + embedded->setParent( host->getRoot() ); + host->setStyleSheet( "* { opacity: 1; }" ); + auto* layout = eeNew( InvalidationTestLayout, () ); + auto* child = UIWidget::New(); + child->setParent( layout ); + host->flushDirtyStyleAndLayout(); + embedded->flushDirtyStyleAndLayout(); + + layout->setLayoutDirty( LayoutInvalidation::TextFormatting ); + host->invalidateStyle( layout ); + host->invalidateStyleState( layout, true ); + host->invalidateStyle( child ); + host->invalidateStyleState( child, true ); + EXPECT_TRUE( host->hasPendingLayout( layout ) ); + layout->setParent( embedded->getRoot() ); + EXPECT_EQ( layout->getUISceneNode(), embedded ); + EXPECT_FALSE( host->hasPendingLayout( layout ) ); + EXPECT_FALSE( host->hasPendingStyle( layout ) ); + EXPECT_FALSE( host->hasPendingStyleState( layout ) ); + EXPECT_FALSE( host->hasPendingStyleStateAnimations( layout ) ); + EXPECT_FALSE( host->hasPendingStyle( child ) ); + EXPECT_FALSE( host->hasPendingStyleState( child ) ); + EXPECT_FALSE( host->hasPendingStyleStateAnimations( child ) ); + EXPECT_TRUE( embedded->hasPendingLayout( layout ) ); + EXPECT_TRUE( embedded->hasPendingStyle( layout ) ); + EXPECT_TRUE( embedded->hasPendingStyleState( layout ) ); + EXPECT_TRUE( embedded->hasPendingStyleStateAnimations( layout ) ); + + embedded->flushDirtyStyleAndLayout(); + EXPECT_TRUE( layout->lastReasons & LayoutInvalidation::TextFormatting ); + EXPECT_FALSE( layout->isLayoutDirty() ); + layout->setLayoutDirty( LayoutInvalidation::TextFormatting ); + layout->setParent( host->getRoot() ); + EXPECT_FALSE( embedded->hasPendingLayout( layout ) ); + EXPECT_FALSE( embedded->hasPendingStyle( layout ) ); + EXPECT_FALSE( embedded->hasPendingStyleState( layout ) ); + EXPECT_TRUE( host->hasPendingLayout( layout ) ); + host->updateDirtyLayouts(); + EXPECT_TRUE( layout->lastReasons & LayoutInvalidation::TextFormatting ); + EXPECT_FALSE( layout->isLayoutDirty() ); + + // Coalescing into a dirty parent must not strand a child moved into another scene. + auto* parent = eeNew( InvalidationTestLayout, () ); + layout->setParent( parent ); + host->flushDirtyStyleAndLayout(); + layout->setLayoutDirty( LayoutInvalidation::TextFormatting ); + parent->setLayoutDirty( LayoutInvalidation::Self ); + EXPECT_FALSE( host->hasPendingLayout( layout ) ); + EXPECT_TRUE( layout->isLayoutDirty() ); + layout->setParent( embedded->getRoot() ); + EXPECT_TRUE( embedded->hasPendingLayout( layout ) ); + embedded->updateDirtyLayouts(); + EXPECT_TRUE( layout->lastReasons & LayoutInvalidation::TextFormatting ); + EXPECT_FALSE( layout->isLayoutDirty() ); + eeDelete( layout ); + host->flushDirtyStyleAndLayout(); + embedded->flushDirtyStyleAndLayout(); + + Engine::destroySingleton(); +} + UTEST( UISceneNode, LeafLayoutInvalidationPreservesSiblingRootsAndDescendantReasons ) { Engine::instance()->createWindow( WindowSettings( 320, 240, "Leaf Layout Invalidation", WindowStyle::Default, WindowBackend::Default,