On Tue, 18 Oct 2022 04:56:07 GMT, Prasanta Sadhukhan <[email protected]>
wrote:
>> DefaultListSelectionModel.removeIndexInterva accepts `int` value which
>> allows it to take in Integer.MAX_VALUE theoratically but it does calculation
>> with that value which can results in IOOBE.
>> Fix is to make sure the calculation stays within bounds.
>
> Prasanta Sadhukhan has updated the pull request incrementally with one
> additional commit since the last revision:
>
> insertIndexInterval fix. Add more subtests
This is tough. The number of test cases has grown to 25. There are many
failures.
src/java.desktop/share/classes/javax/swing/DefaultListSelectionModel.java line
455:
> 453: if (i + 1 < i) {
> 454: break;
> 455: }
I would rather add `i >= 0` to the loop condition. Yet I can't see why the
reversed loop doesn't work.
src/java.desktop/share/classes/javax/swing/DefaultListSelectionModel.java line
660:
> 658: * insMinIndex).
> 659: */
> 660: for(int i = maxIndex; i >= insMinIndex; i--) {
`insertIndexInterval` doesn't verify its parameters: negative `index`, negative
`length` should be rejected right away.
src/java.desktop/share/classes/javax/swing/DefaultListSelectionModel.java line
661:
> 659: */
> 660: for(int i = maxIndex; i >= insMinIndex; i--) {
> 661: if ((i + length) >= length) {
I suggest using `i <= Integer.MAX_VALUE - length`. Yet I'm still unsure it's
enough and *correct*.
src/java.desktop/share/classes/javax/swing/DefaultListSelectionModel.java line
664:
> 662: setState(i + length, value.get(i));
> 663: } else {
> 664: setState(i, value.get(i));
This operation in the `else` block doesn't make sense, you can just skip it.
src/java.desktop/share/classes/javax/swing/DefaultListSelectionModel.java line
721:
> 719: setState(i, value.get(gapLength));
> 720: break;
> 721: }
This does not work. It must not break. With this code, 5 of my tests fail.
For example,
selectionModel.setSelectionInterval(0, Integer.MAX_VALUE);
selectionModel.removeIndexInterval(0, Integer.MAX_VALUE);
must result in empty selection. All the possible indexes were selected, you
removed all the possible indexes (so that “the list” contains no elements at
all). This test case fails either way, unfortunately:
`selectionModel.isSelectedIndex(0)` still returns `true`.
This edge case where `rmMinIndex = 0` and `rmMaxIndex = Integer.MAX_VALUE`
should probably be short-circuited: `setState(i, false)` for all the elements.
Other values are handled correctly by my suggested code, as far as I can see.
This test case:
selectionModel.setSelectionInterval(0, Integer.MAX_VALUE);
selectionModel.removeIndexInterval(1, Integer.MAX_VALUE);
passed before and now it fails again. Here, `selectionModel.isSelectedIndex(0)`
must be `true`.
test/jdk/javax/swing/TestDefListModelException.java line 62:
> 60: DefaultListSelectionModel selectionModel = new
> DefaultListSelectionModel();
> 61: selectionModel.setSelectionInterval(Integer.MAX_VALUE - 1,
> Integer.MAX_VALUE);
> 62: selectionModel.insertIndexInterval(Integer.MAX_VALUE - 1,
> Integer.MAX_VALUE, true);
I think testing that it does not throw an exception is not enough. You should
verify that the data selection model holds is correct.
According to the spec, the interval from `Integer.MAX_VALUE - 1` to
`Integer.MAX_VALUE` should remain selected.
-------------
Changes requested by aivanov (Reviewer).
PR: https://git.openjdk.org/jdk/pull/10409
