RFR: 8360940: Layout stops updating when using Parent#setNeedsLayout(true) due to incorrect state management [v2]

[Andy Goryachev]

On Wed, 1 Oct 2025 08:44:14 GMT, John Hendrikx <jhendrikx at openjdk.org> wrote:

>> Ensures proper propagation of layout flags when using `forceParentLayout = true`.
>> 
>> This was the root cause of issue #1874
>> 
>> ### Note
>> 
>> Apparently it is still quite easy to mess up the layout flags. Basically, the layout flag tracked by Parent should always either be `CLEAN` for any scene graph branch, or `!CLEAN` + a layout pulse is scheduled on the corresponding `Scene`.
>> 
>> However, with careful use of the public API `requestLayout` one can get these flags in a bad state still:
>> 
>> Let's say I have a branch `A (root node under Scene) -> B -> C`, **and** a layout is in progress, **and** we're currently in the `layoutChildren` method of `C`. The flag `performingLayout` will be `true` for all nodes in the branch `A` ->  `B` -> `C`.  The `layout` method will set the layout flag to `CLEAN` as its first action, so when we're at `C::layoutChildren`, all flags have been reset to `CLEAN` already.  See the `Parent::layout` method for how all this works.
>> 
>> Now, to mess up the flags, all you need to do is call `requestLayout` on `B` or `C` from the `layoutChildren` of `C` (or indirectly by changing something and something is listening to this and schedules a layout on something somewhere in this branch); note that `requestLayout` is not documented to be illegal to call during layout, and some classes in FX will do so (ScrollPaneSkin, NumberAxis, etc..) risking the flags getting in a bad state... -- usually you get away with this, as there are many ways that layout is triggered, and eventually the flags will get overwritten and reset to a consistent state.
>> 
>> The bad state occurs because this code path is followed (all code from `Parent`):
>> 
>>     public void requestLayout() {
>>         clearSizeCache();
>>         markDirtyLayout(false, forceParentLayout);
>>     }
>>     
>> Calls to `markDirtyLayout(false, false)`:
>> 
>>     private void markDirtyLayout(boolean local, boolean forceParentLayout) {
>>         setLayoutFlag(LayoutFlags.NEEDS_LAYOUT);
>>         if (local || layoutRoot) {
>>             if (sceneRoot) {
>>                 Toolkit.getToolkit().requestNextPulse();
>>                 if (getSubScene() != null) {
>>                     getSubScene().setDirtyLayout(this);
>>                 }
>>             } else {
>>                 markDirtyLayoutBranch();
>>             }
>>         } else {
>>             requestParentLayout(forceParentLayout);
>>         }
>>     }
>>     
>> Before going into the `else` (as none of the nodes is a layout root, and `lo...
>
> John Hendrikx has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Add test case

Marked as reviewed by angorya (Reviewer).

modules/javafx.graphics/src/main/java/javafx/scene/Parent.java line 1077:

> 1075:                  */
> 1076: 
> 1077:                 p.requestLayout(forceParentLayout);

very, very minor suggestion to makes it more compact (it's ok to leave things as is):



                // The forceParentLayout flag must be propagated to mark all ancestors
                // as needing layout. Failure to do so while performingLayout is true
                // would stop the propagation mid-tree. This leaves some nodes as needing
                // layout, while its ancestors are clean, which is an inconsistent state.
                p.requestLayout(forceParentLayout);

-------------

PR Review: https://git.openjdk.org/jfx/pull/1879#pullrequestreview-3290055086
PR Review Comment: https://git.openjdk.org/jfx/pull/1879#discussion_r2395189222