RFR: 8301763: Adding children to wrong index leaves inconsistent state in Parent#childrenSet [v2]

[John Hendrikx]

On Wed, 17 May 2023 15:22:57 GMT, Lukasz Kostyra <lkostyra at openjdk.org> wrote:

>> This issue happened because `childSet` member of Parent was modified during `onProposedChange()` call - that call did not recognize negative indexes as invalid, which caused an exception when actually adding the Node to a List.
>> 
>> This seemed like the simplest solution which doesn't rework a lot of code underneath. Exceptions coming from a backing list/collection technically are handled by `VetoableListDecorator`'s try-catch clauses, however `VetoableListDecorator` does not provide an interface to react when such an exception happens - without it we cannot revert `childSet` back to its original state.
>
> Lukasz Kostyra has updated the pull request incrementally with three additional commits since the last revision:
> 
>  - Add index/null checks to ObservableListWrapper and VetoableListDecorator
>  - ParentTest: Expect IndexOutOfBoundsException, add test cases
>  - Parent: Remove index check from onProposedChange

I think `ObservableListWrapper#remove(int, int)` also needs a check index check, or it may end up remove some items and then throw an exception (exceptions thrown from collections should not modify its state).

modules/javafx.base/src/main/java/com/sun/javafx/collections/VetoableListDecorator.java line 116:

> 114:     public boolean setAll(Collection<? extends E> col) {
> 115:         Objects.requireNonNull(col);
> 116:         onProposedChange(Collections.unmodifiableList(new ArrayList<>(col)), 0, size());

It's good to make it explicit.  It already threw NPE because of `new ArrayList<>(col)`, so this is not a change.

modules/javafx.base/src/main/java/com/sun/javafx/collections/VetoableListDecorator.java line 270:

> 268:     @Override
> 269:     public boolean removeAll(Collection<?> c) {
> 270:         Objects.requireNonNull(c);

Looks like a good change, if `c` was `null` and the backing list was empty, this would not throw NPE, but now it does.

modules/javafx.graphics/src/test/java/test/javafx/scene/ParentTest.java line 894:

> 892:         g.getChildren().remove(0);
> 893:     }
> 894: 

I think you should also add tests for the `null` cases, if they aren't part of this test already.

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

Changes requested by jhendrikx (Committer).

PR Review: https://git.openjdk.org/jfx/pull/1136#pullrequestreview-1431766296
PR Review Comment: https://git.openjdk.org/jfx/pull/1136#discussion_r1197130391
PR Review Comment: https://git.openjdk.org/jfx/pull/1136#discussion_r1197132309
PR Review Comment: https://git.openjdk.org/jfx/pull/1136#discussion_r1197133728