On Wed, 8 Jan 2025 11:25:37 GMT, John Hendrikx <[email protected]> wrote:
>> Michael Strauß has updated the pull request with a new target base due to a
>> merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains seven additional
>> commits since the last revision:
>>
>> - Merge branch 'master' into feature/root-pseudo-class
>> - whitespace error
>> - add child-indexed pseudo-classes
>> - add pseudo-class table to Parent
>> - update cssref
>> - Set :root pseudo-class on sub-scene root node
>> - Set :root pseudo-class on root node
>
> modules/javafx.graphics/src/main/java/javafx/scene/Parent.java line 341:
>
>> 339:
>> 340: @Override
>> 341: protected void onChanged(Change<Node> c) {
>
> I hate to bring this up, and it perhaps should be dealt with separately, but
> how safe is this code in `onProposedChange` / `onChanged` when it is
> re-entered due to the many potential listener callbacks? For example, the
> removal of a child on `parent` can trigger list change listeners from the
> user that can trigger all kinds of other changes that eventually lead to more
> children changes on this node, triggering a nested `onChanged`; the new
> changes are adding more opportunities in the form of listeners on pseudo
> state changes.
>
> For example, `onProposedChange` will blatantly reset `geomChanged` to `false`
> in all cases, even if it rejects the change later. It doesn't check if an
> `onChange` is in progress higher up the call stack, effectively resetting a
> flag that is in active use...
>
> I believe this may be quite a difficult problem to solve if nested calls
> remain allowed, and one of the easiest ways to fix it IMHO is to reject any
> changes (in `onProposedChange`) when a change is already in progress. For
> users this would effectively mean that listeners triggered during a child
> modification will not be allowed to make further modifications to the same
> children list until the first one completes. This is not a common situation,
> but can happen with very dynamic UI's that construct new nodes in response to
> triggers; without such protection for nested changes, the errors that crop up
> are often of a form that seem to be multi-threading issues (where certain
> invariants seem to be broken), while in reality they're just re-entrant code
> calls that use instance fields in an unsafe manner.
One option might be to move all pseudo-class modifications to the end of the
`onChange` method. This would make the code equivalent to being called after
the callback has completed.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1652#discussion_r1907567157
