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

[John Hendrikx]

On Mon, 15 May 2023 12:49:41 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.

I think it may be better to do this check in the callers of `onProposedChange` (using the assertions in `Objects` to test for valid indices.).  This will also allow to check when the index is too large, which is a similar problem. Note that you can't do the "too large" check in `onProposedChange` as for `remove`, `size()` would be too large, while for `add(int, E)` `size()` would still be valid.

Also, for the cases where `onProposedChange` is called with multiple sets of indices, those don't need a check (they come from calls like `removeAll`, which will all be valid).

Furthermore, it should be an `IndexOutOfBoundsException` as this is specified by the `List` interface.

I noticed there are similar problems in `ObservableListWrapper`.  This method for example:

    @Override
    protected void doAdd(int index, E element) {
        if (elementObserver != null)
            elementObserver.attachListener(element);
        backingList.add(index, element);
    }

This will attach a listener to the element (assuming it is an `Observable`) even if `backingList.add` fails...

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

PR Comment: https://git.openjdk.org/jfx/pull/1136#issuecomment-1548075656