JDK-8230098: NPEs in ListView with no SelectionModel

[vlaaad]

While there are many places in ListViewBehavior that check if list's selection model is not null, there are still a couple of places that assume it exists. One of them is mousePressed method, triggered by clicking on cells, and another is activate, triggered by pressing either enter, space or f2 when list view has focus.

You can reproduce it by creating a list view, setting it's selection model to null and then clicking on it's items and pressing enter.

[sghpjuikit]

If you know how to reproduce the issue, it would be really great if you provided a simple self-contained example application and specified exact java/javafx version.

That being said, I think I've been running into very similar bug (but in TableView) again and again for a while now, but I never took time to actually understand how to reproduce it. But I'm certain there is a TableView/ListView selectionModel bug.

[vlaaad]

repro:

public static class TestApp extends Application {

    @Override
    public void start(Stage primaryStage) throws Exception {
        ListView<String> listView = new ListView<>(FXCollections.observableArrayList("a", "b", "c"));
        listView.setSelectionModel(null);
        primaryStage.setScene(new Scene(listView));
        primaryStage.show();

    }
}

List view appears, click on item, see exception. Tried on latest develop branch.

[vlaaad]

Complete test case (save it into /tests/system/src/test/java/test/javafx/scene/control/ListViewWithoutSelectionModelTest.java):

package test.javafx.scene.control;

import javafx.application.Application;
import javafx.application.Platform;
import javafx.collections.FXCollections;
import javafx.geometry.Point2D;
import javafx.scene.Scene;
import javafx.scene.control.ListView;
import javafx.scene.input.KeyCode;
import javafx.scene.input.MouseButton;
import javafx.scene.robot.Robot;
import javafx.stage.Stage;
import javafx.stage.WindowEvent;
import org.junit.AfterClass;
import org.junit.Assert;
import org.junit.BeforeClass;
import org.junit.Test;
import test.util.Util;

import java.io.ByteArrayOutputStream;
import java.io.PrintStream;
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.TimeUnit;

public class ListViewWithoutSelectionModelTest {
    static ListView<String> listView;
    static CountDownLatch startupLatch;

    public static void main(String[] args) throws Exception {
        initFX();
        try {
            var test = new ListViewWithoutSelectionModelTest();
            test.testListViewWithoutSelectionModel();
        } catch (Throwable e) {
            e.printStackTrace();
        } finally {
            tearDown();
        }
    }

    @Test
    public void testListViewWithoutSelectionModel() throws InterruptedException {
        PrintStream defaultErrorStream = System.err;
        ByteArrayOutputStream out = new ByteArrayOutputStream();
        System.setErr(new PrintStream(out, true));
        Thread.sleep(1000);
        Util.runAndWait(() -> {
            Robot robot = new Robot();
            robot.mouseMove(listView.localToScreen(new Point2D(10, 10)));
            robot.mouseClick(MouseButton.PRIMARY);
            robot.keyType(KeyCode.ENTER);
            robot.keyType(KeyCode.SPACE);
            robot.keyType(KeyCode.F2);
        });
        Thread.sleep(2000);
        System.setErr(defaultErrorStream);
        Assert.assertEquals("No error should be thrown", "", out.toString());
    }

    public static class TestApp extends Application {

        @Override
        public void start(Stage primaryStage) throws Exception {
            listView = new ListView<>();
            listView.setSelectionModel(null);
            listView.setItems(FXCollections.observableArrayList("a", "b", "c"));
            primaryStage.addEventHandler(WindowEvent.WINDOW_SHOWN, event -> startupLatch.countDown());
            primaryStage.setScene(new Scene(listView));
            primaryStage.show();
            listView.requestFocus();

        }
    }

    @BeforeClass
    public static void initFX() throws Exception {
        startupLatch = new CountDownLatch(1);
        new Thread(() -> Application.launch(TestApp.class, (String[]) null)).start();
        Assert.assertTrue("Timeout waiting for FX runtime to start",
                startupLatch.await(15, TimeUnit.SECONDS));
    }

    @AfterClass
    public static void tearDown() {
        Platform.exit();
    }
}

[vlaaad]

I can make a PR for this, but it requires an issue to be present in official issue tracker, and I don't have an account there. I signed OCA though.

[sghpjuikit]

I have just realized the bug is very different from the one I'm experiencing...

I can reproduce the issue with the test application.

[nlisker]

@vlaaad You can report it at https://bugreport.java.com/bugreport/ and it will be moved to JBS.

[vlaaad]

Submitted report for moderation, id 9062014

[nlisker]

A quick look at the code shows the following locations not handling null:

• com.sun.javafx.PlatformUtil.ListViewBehavior:

  • activate()
  • clearSelection()
  • mousePressed(MouseEvent e)
  • selectedIndicesListener (in the lambda)
  • selectFirstRow()
  • getSelectionModel()
  • selectPreviousRow()

• Several places inside javafx.scene.control.skin.ComboBoxListViewSkin, though since the ListView is an internal implementation detail it might be possible to prove that a state where its selection model is null is not reachable.

• javafx.scene.control.skin.ListViewSkin in the queryAccessibleAttribute method in the SELECTED_ITEMS case. It could be that at this point the selection model can't be null already.

The documentation says "A ListView has at most one instance [of selection model]", but what if it has none? I assume it means that no selections can be made (a la setSelectable(false)). It seems like it was a mistake to permit it and an option of NONE should have been used. I think that it might still be better to add the NONE selection model and synonymize it with null, if such a behavior even makes sense. Regardless, the documentation should cover this case.

[kevinrushforth]

Submitted report for moderation, id 9062014

Thanks. I transferred it into the JDK project: JDK-8230098.

[sghpjuikit]

Indeed no selection model should still be a selection model. I support adding the NONE model.

This probably affects all components with selection model. Could/should be mirrored for focus models as well. But this is a nice-to-have feature and should be separate from the bug.

[vlaaad]

Cool.

I assume it means that no selections can be made (a la setSelectable(false))

Is there such a thing somewhere? I can't find it but it's exactly my use case: I want a list view where all user can do is pan/scroll.

Are you interested in PR adding null checks in ListViewBehavior and friends? I can implement NoneSelectionModel as well, feels cleaner to me too.

[nlisker]

I found JDK-8090060, which could be a duplicate.

Is there such a thing somewhere?

No, it was just to clarify what I meant.

Are you interested at PR adding null checks in ListViewBehavior and friends? I can implement NoneSelectionModel as well, feels cleaner to me too.

I'm not too familiar with this API, but I think that a proper fix would be to add to javafx.scene.control.SelectionMode an new constant NONE. Whenever the selection model is set to null it is set instead to a selection model that fits that behavior. What that selection model is exactly will have to be thought about. This will require changes in other controls like TreeView and TreeTableView.

[kleopatra]

Several places inside javafx.scene.control.skin.ComboBoxListViewSkin, though since the ListView is an internal implementation detail it might be possible to prove that a state where its selection model is null is not reachable.

the listView is reachable through skin.getPopupContent

The documentation says "A ListView has at most one instance [of selection model]", but what if it has none? Regardless, the documentation should cover this case.

I think that the "at most" does cover the null case :) Null is a perfectly valid value - arguably not the best api, but that's what we have, not only in listView but in all others that have a selectionModel. Thought they had been fixed quite a while ago, but aren't, obviously (what was fixed is the NPE when having null items)

[nlisker]

I think that the "at most" does cover the null case

But the behavior of selection in such a case is not documented anywhere that I could find.

[kleopatra]

ahh .. now I understand what you meant. Okay, would be better, indeed (though I think that null == no selection is rather near-lying ;)

BTW, as to the NONE mode: wouldn't that break compatibility? All code checking against null will fail. In production might or not be important (simply run through until hitting the nothing-selected), tests that verify a null model will fail. Not very frequent, but still ..

[nlisker]

BTW, as to the NONE mode: wouldn't that break compatibility?

That's why I suggested:

I think that it might still be better to add the NONE selection model and synonymize it with null

It's not the first time such a thing is done. For example, the peer of PhongMaterial, NGPhongMaterial, does something like these:

if (diffuseColor != null) {
    material.setDiffuseColor(r, g, b, a);
} else {
    material.setDiffuseColor(0, 0, 0, 0);
}

if (diffuseMap.getImage() == null) {
    diffuseMap.setImage(WHITE_1X1);
}

and LighBase does:

peer.setColor((getColor() == null) ?
                    Toolkit.getPaintAccessor().getPlatformPaint(Color.WHITE)
                    : Toolkit.getPaintAccessor().getPlatformPaint(getColor()));

Though these examples are somewhat inconsistent as to where null is handled, the idea is the same.

It seems that JavaFX chose to allow null as a valid value for ObjectPropertys, which might not have been the best idea (though not a terrible one either), so they must be documented.

[kleopatra]

hmm ... feels like I couldn't make my point clearly (or maybe still not understand what you are suggesting ;), so trying again:

Consider the following test code:

assertNull(list.getSelectionModel())

now it is passing (assuming we have configured a listView to not support selection by setting the model to null), with your suggestion the test will fail, or not?

[nlisker]

It should pass.

[kleopatra]

okay, than I don't understand what you suggest - maybe too much debugging today ;)