RFR: 6187113: DefaultListSelectionModel.removeIndexInterval(0, Integer.MAX_VALUE) fails [v6]

[Alexey Ivanov]

On Tue, 18 Oct 2022 04:56:07 GMT, Prasanta Sadhukhan <psadhukhan at openjdk.org> 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