Skip to content

Fix recursion panic in geometry property callbacks (#7402, #7849)#10416

Closed
tilladam wants to merge 5 commits intoslint-ui:masterfrom
tilladam:geometry-access-recursion
Closed

Fix recursion panic in geometry property callbacks (#7402, #7849)#10416
tilladam wants to merge 5 commits intoslint-ui:masterfrom
tilladam:geometry-access-recursion

Conversation

@tilladam
Copy link
Copy Markdown
Contributor

Summary
This PR fixes a recursion panic that occurred when using changed callbacks on geometry properties (like width or height) within layouts containing conditional elements (if or for loops).

Previously, ChangeTracker::init() would immediately evaluate properties, triggering layout computations. In scenarios involving dynamic components (repeaters/conditionals), this led to recursive ensure_updated() calls and subsequent panics during component initialization.

Changes

Core:
Introduced init_delayed() for ChangeTracker to defer the first property evaluation until run_change_handlers() is called.
Added a new initialization_scope module with defer_initialization() and InitializationScope RAII guards.
Refactored ChangeTracker FFI to support delayed initialization and added safety guards (evaluating flag) to prevent use-after-free.
Generators & Interpreter:
Updated Rust and C++ generators to wrap component creation in an initialization scope.
Updated the interpreter to use init_delayed.
Tests:
Added reproduction test cases for issues Querying geometry property from init callback can cause 'Recursion detected' #7402 and #7849.
Updated changes.slint expectations to reflect the new callback ordering.
Fixes

Fixes Querying geometry property from init callback can cause 'Recursion detected' #7402
Fixes #7849

@ogoffart
Copy link
Copy Markdown
Member

I finally had time to look at this PR.

The problem it solves is real, but i'm not sure this is the right way to do it.

I'm thinking the real problem here, which causes many problem, is that elements get instantiated when as we visit the item tree to instantiate property.

Consider this example:

export component Example inherits Window {
    layout := HorizontalLayout {
        if true: Rectangle {
            init => {
                debug(layout.preferred-width);
            }
        }
    }
}

This will basically create a fake binding on the layout's preferred-with that will visit all its children and add their preferred-width. The problem is that reading the preferred-width property, which is lazy, will evaluate it, and this will cause instantiation of item. And this will query the value of preferred-width from the init callback.

I see a few way out of it:

  1. We should properly detect this kind of binding loop at compile time, which means that the binding_analysis should consider the changed properties and the init callback in visit_layout_items_dependencies (we currently already visit the model via recurse_expression(&element, &r.model, vis) but we also should visit all the init and changed callback of the generated component.

  2. The real problem is that the init callback can have side effect when reading a property.
    prehaps we should not call ensure_updated when visiting the layout's geometry. Instead, we could instentiate the repeater in a call similar to ChangeTracker::run_chage_handler.
    That might be a breaking change, especially for some of our tests. But i think this has caused problem before that the init callback can cause property to change as we try to redraw a scene or something.

@tilladam tilladam force-pushed the geometry-access-recursion branch from 774093d to 4d2eff3 Compare January 23, 2026 11:57
@tilladam
Copy link
Copy Markdown
Contributor Author

Thanks for the detailed feedback! I've implemented Option 1 (compile-time detection) as a complement to the runtime fix.

What's implemented:

The binding analysis now visits properties tracked by changed callbacks in repeated components when processing layouts. This detects the binding loop at compile time:

warning: The binding for the property 'height' is part of a binding loop (... -> gb.height -> layoutinfo-v -> ...).
         This could cause unexpected behavior at runtime

Why warnings instead of errors:

I made these warnings rather than errors because:

  1. Users can't always avoid this pattern - A component author might add changed height without knowing their component will be used in a layout repeater. The composition is valid in isolation.

  2. The runtime handles it gracefully - With init_delayed() and InitializationScope, the code runs correctly. The callbacks just fire after initialization completes, which is arguably better behavior anyway.

  3. False positives are possible - The static analysis is conservative and may flag cases that wouldn't actually cause issues at runtime.

Why not Option 2 (don't call ensure_updated during layout):

This would be a significant architectural change with unclear benefits. Layout needs to know about all children to compute correctly - if repeaters aren't instantiated, we'd get incorrect layout results. The current approach (defer callback execution, not component instantiation) achieves the same goal with less disruption.

Summary:

  • Runtime fix remains the primary solution (prevents panic, handles edge cases)
  • Compile-time warning alerts users to the pattern so they can investigate if needed
  • Code compiles and runs correctly in all cases

@ogoffart
Copy link
Copy Markdown
Member

Thanks.
Would it be possible to extract the change to fix compile-time loop detection in another PR? (Separated from the InitializationScope changes)

@tilladam tilladam force-pushed the geometry-access-recursion branch 2 times, most recently from b6c4513 to ab205d7 Compare January 26, 2026 14:57
@tilladam
Copy link
Copy Markdown
Contributor Author

Done with #10552

@bryce-happel-walton
Copy link
Copy Markdown
Contributor

@ogoffart Can this be merged?

tilladam added 4 commits March 7, 2026 18:19
…lint-ui#7849)

Add minimal reproduction test cases for recursion panics that occur when
accessing geometry properties during callback initialization:

- issue_7402_init_geometry_recursion.slint: Tests init callback accessing
  absolute-position in ComponentContainer context
- issue_7849_changed_height_recursion.slint: Minimal repro with changed
  height callback + VerticalLayout + if condition
- issue_7849_changed_height_for_loop.slint: Original repro with changed
  height callback + VerticalLayout + for loop

The slint-ui#7849 tests currently panic with "Recursion detected" confirming
the bug is still present.
…t-ui#7849)

Use init_delayed() instead of init() for ChangeTracker initialization
to prevent recursion when changed callbacks are set on geometry properties
(height, width) in layouts with for/if conditions.

The problem was that ChangeTracker::init() immediately evaluates the
property, which triggers layout computation, which calls ensure_updated()
on repeaters/conditionals, which creates new components with their own
change trackers, causing recursive init() calls and a panic.

The fix defers the first property evaluation until run_change_handlers()
is called in the event loop, breaking the recursion chain.

Changes:
- Use init_delayed() in interpreter (dynamic_item_tree.rs)
- Use init_delayed() in Rust code generator (rust.rs)
- Use init_delayed() in C++ code generator (cpp.rs)

- Add run_change_handlers() FFI function and C++ static method to allow
  processing delayed change trackers from generated code
- Call run_change_handlers() at the end of component creation (Rust, C++,
  interpreter) to ensure change trackers register their dependencies before
  user code can modify properties

- Add initialization_scope module to core with defer_initialization()
- Wrap component creation in with_initialization_scope() in generators
- Add InitializationScope RAII guard for C++ API

- Add evaluating guard to FFI change tracker init_delayed

The FFI version of slint_change_tracker_init_delayed was missing the
"evaluating" guard that prevents use-after-free when a change tracker
is dropped during its own callback execution.

- Add slint_change_tracker_init_delayed FFI function (ffi.rs)
- Add init_delayed method to C++ ChangeTracker (slint_properties.h)

- Makes CHANGED_NODES pub(super) so FFI can access it

- Update test expectations in changes.slint for new callback ordering

Fixes slint-ui#7402, slint-ui#7849
@tilladam tilladam force-pushed the geometry-access-recursion branch from ab205d7 to 3be4217 Compare March 7, 2026 17:21
@ogoffart
Copy link
Copy Markdown
Member

Had a short talk with @tronical about this. We will close this PR for now.
This PR adds lots of complication and we're not ready to maintain this kind of code.
Especially since some property would be initialized delayed and some not in a non-intuitive way.
Acceptable solution would be to improve the compiler to detect these loop at compile time (#10552)

It may be also possible to consider always initializing the changed callback with a delay.
This would be a change in behavior though, but it may be acceptable.

@ogoffart ogoffart closed this Mar 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Property recursion panic when having changed event on geometry property in a layout with condition

3 participants