On Mon, 15 May 2023 12:49:41 GMT, Lukasz Kostyra <lkost...@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
