On Wed, 8 Jan 2025 11:25:37 GMT, John Hendrikx <jhendr...@openjdk.org> 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