8089398: [ChoiceBox, ComboBox] throws NPE on setting value on null selectionModel

Reviewed-by: fastegal, aghaisas

Author: Marius Hanl

modules/javafx.controls/src/main/java/javafx/scene/control/ChoiceBox.java

@@ -151,9 +151,12 @@ public ChoiceBox(ObservableList<T> items) {
         // selection model to indicate that this is the selected item
         valueProperty().addListener((ov, t, t1) -> {
             if (getItems() == null) return;
+            SingleSelectionModel<T> sm = getSelectionModel();
+            if (sm == null) return;
+
             int index = getItems().indexOf(t1);
             if (index > -1) {
-                getSelectionModel().select(index);
+                sm.select(index);
             }
         });
     }

modules/javafx.controls/src/main/java/javafx/scene/control/ComboBox.java

@@ -240,8 +240,9 @@ public ComboBox(ObservableList<T> items) {
         // selection model to indicate that this is the selected item
         valueProperty().addListener((ov, t, t1) -> {
             if (getItems() == null) return;
-
             SelectionModel<T> sm = getSelectionModel();
+            if (sm == null) return;
+
             int index = getItems().indexOf(t1);
 
             if (index == -1) {
@@ -278,7 +279,10 @@ public ComboBox(ObservableList<T> items) {
             if (!isEditable()) {
                 // check if value is in items list
                 if (getItems() != null && !getItems().contains(getValue())) {
-                    getSelectionModel().clearSelection();
+                    SingleSelectionModel<T> selectionModel = getSelectionModel();
+                    if (selectionModel != null) {
+                        selectionModel.clearSelection();
+                    }
                 }
             }
         });

modules/javafx.controls/src/main/java/javafx/scene/control/skin/ComboBoxListViewSkin.java

@@ -53,6 +53,7 @@
 import javafx.scene.control.ListView;
 import javafx.scene.control.SelectionMode;
 import javafx.scene.control.SelectionModel;
+import javafx.scene.control.SingleSelectionModel;
 import javafx.scene.control.TextField;
 import javafx.scene.input.*;
 import javafx.util.Callback;
@@ -321,9 +322,13 @@ public final void setHideOnClick(boolean value) {
         if (listViewSelectionDirty) {
             try {
                 listSelectionLock = true;
-                T item = comboBox.getSelectionModel().getSelectedItem();
-                listView.getSelectionModel().clearSelection();
-                listView.getSelectionModel().select(item);
+                SingleSelectionModel<T> selectionModel = comboBox.getSelectionModel();
+
+                if (selectionModel != null) {
+                    T item = selectionModel.getSelectedItem();
+                    listView.getSelectionModel().clearSelection();
+                    listView.getSelectionModel().select(item);
+                }
             } finally {
                 listSelectionLock = false;
                 listViewSelectionDirty = false;
@@ -396,6 +401,11 @@ private void updateListViewItems() {
     }
 
     private void updateValue() {
+        SingleSelectionModel<T> comboBoxSM = comboBox.getSelectionModel();
+        if (comboBoxSM == null) {
+            return;
+        }
+
         T newValue = comboBox.getValue();
 
         SelectionModel<T> listViewSM = listView.getSelectionModel();
@@ -413,7 +423,7 @@ private void updateValue() {
                 listViewSM.clearSelection();
                 listSelectionLock = false;
             } else {
-                int index = comboBox.getSelectionModel().getSelectedIndex();
+                int index = comboBoxSM.getSelectedIndex();
                 if (index >= 0 && index < comboBoxItems.size()) {
                     T itemsObj = comboBoxItems.get(index);
                     if ((itemsObj != null && itemsObj.equals(newValue)) || (itemsObj == null && newValue == null)) {
@@ -554,15 +564,21 @@ private ListView<T> createListView() {
 
         _listView.getSelectionModel().selectedIndexProperty().addListener(o -> {
             if (listSelectionLock) return;
+            SingleSelectionModel<T> selectionModel = comboBox.getSelectionModel();
+            if (selectionModel == null) return;
+
             int index = listView.getSelectionModel().getSelectedIndex();
-            comboBox.getSelectionModel().select(index);
+            selectionModel.select(index);
             updateDisplayNode();
             comboBox.notifyAccessibleAttributeChanged(AccessibleAttribute.TEXT);
         });
 
-        comboBox.getSelectionModel().selectedItemProperty().addListener(o -> {
-            listViewSelectionDirty = true;
-        });
+        SingleSelectionModel<T> selectionModel = comboBox.getSelectionModel();
+        if (selectionModel != null) {
+            selectionModel.selectedItemProperty().addListener(o -> {
+                listViewSelectionDirty = true;
+            });
+        }
 
         _listView.addEventFilter(MouseEvent.MOUSE_RELEASED, t -> {
             // RT-18672: Without checking if the user is clicking in the

modules/javafx.controls/src/test/java/test/javafx/scene/control/ChoiceBoxTest.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2011, 2020, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2011, 2021, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -26,6 +26,7 @@
 package test.javafx.scene.control;
 
 import javafx.scene.control.Separator;
+import org.junit.After;
 import test.com.sun.javafx.pgstub.StubToolkit;
 import com.sun.javafx.tk.Toolkit;
 
@@ -56,8 +57,6 @@
 import javafx.scene.control.SingleSelectionModel;
 import javafx.scene.layout.StackPane;
 import javafx.stage.Stage;
-import javafx.event.Event;
-import javafx.event.EventHandler;
 
 import org.junit.Before;
 import org.junit.Ignore;
@@ -70,10 +69,22 @@ public class ChoiceBoxTest {
     private Stage stage;
 
     @Before public void setup() {
+        Thread.currentThread().setUncaughtExceptionHandler((thread, throwable) -> {
+            if (throwable instanceof RuntimeException) {
+                throw (RuntimeException) throwable;
+            } else {
+                Thread.currentThread().getThreadGroup().uncaughtException(thread, throwable);
+            }
+        });
+
         //This step is not needed (Just to make sure StubToolkit is loaded into VM)
         tk = (StubToolkit)Toolkit.getToolkit();
     }
 
+    @After public void cleanUp() {
+        Thread.currentThread().setUncaughtExceptionHandler(null);
+    }
+
     protected void startApp(Parent root) {
         scene = new Scene(root,800,600);
         stage = new Stage();
@@ -154,6 +165,19 @@ protected void startApp(Parent root) {
         assertNull(box.getSelectionModel());
     }
 
+    @Test public void testNullSelectionModelDoesNotThrowNPEOnValueChange() {
+        ObservableList<String> items = FXCollections.observableArrayList("ITEM1", "ITEM2");
+
+        box.setSkin(new ChoiceBoxSkin<>(box));
+        box.setItems(items);
+        box.setSelectionModel(null);
+
+        box.setValue(items.get(1));
+
+        String text = ChoiceBoxSkinNodesShim.getChoiceBoxSelectedText((ChoiceBoxSkin<?>) box.getSkin());
+        assertEquals(items.get(1), text);
+    }
+
     @Test public void selectionModelCanBeBound() {
         SingleSelectionModel<String> sm = ChoiceBoxShim.<String>get_ChoiceBoxSelectionModel(box);
         ObjectProperty<SingleSelectionModel<String>> other = new SimpleObjectProperty<SingleSelectionModel<String>>(sm);

modules/javafx.controls/src/test/java/test/javafx/scene/control/ComboBoxTest.java

@@ -285,6 +285,83 @@ public Node getDisplayNode() {
         assertNull(comboBox.getSelectionModel());
     }
 
+    @Test public void testNullSelectionModelDoesNotThrowNPEInSkinOnValueChange() {
+        ObservableList<String> items = FXCollections.observableArrayList("ITEM1", "ITEM2");
+
+        ListCell<String> buttonCell = new ListCell<>() {
+            @Override
+            protected void updateItem(String item, boolean empty) {
+                super.updateItem(item, empty);
+                setText(item);
+            }
+        };
+        comboBox.setButtonCell(buttonCell);
+        comboBox.setItems(items);
+        comboBox.setSelectionModel(null);
+
+        // Should not throw an NPE.
+        comboBox.setValue(items.get(1));
+
+        assertEquals(items.get(1), comboBox.getValue());
+        assertEquals(items.get(1), comboBox.getButtonCell().getText());
+    }
+
+    @Test public void testNullSelectionModelDoesNotThrowNPEInSkinOnLayout() {
+        ObservableList<String> items = FXCollections.observableArrayList("ITEM1", "ITEM2");
+
+        comboBox.setItems(items);
+
+        comboBox.setValue(items.get(1));
+        comboBox.setSelectionModel(null);
+
+        // Should not throw an NPE.
+        comboBox.layout();
+    }
+
+    @Test public void testNullSelectionModelDoesNotThrowNPEOnEditableChange() {
+        ObservableList<String> items = FXCollections.observableArrayList("ITEM1", "ITEM2");
+
+        comboBox.setEditable(true);
+        comboBox.setItems(items);
+        comboBox.setSelectionModel(null);
+
+        // Should not throw an NPE.
+        comboBox.setEditable(false);
+    }
+
+    @Test public void testNullSelectionModelDoesNotThrowNPEOnValueChange() {
+        ObservableList<String> items = FXCollections.observableArrayList("ITEM1", "ITEM2");
+
+        ComboBox<String> comboBox = new ComboBox<>();
+        comboBox.setItems(items);
+        comboBox.setSelectionModel(null);
+
+        // Should not throw an NPE.
+        comboBox.setValue(items.get(1));
+
+        assertEquals(items.get(1), comboBox.getValue());
+    }
+
+    @Test public void testNullSelectionModelDoesNotThrowNPEOnListViewSelect() {
+        ObservableList<String> items = FXCollections.observableArrayList("ITEM1", "ITEM2");
+
+        comboBox.setItems(items);
+        comboBox.setSelectionModel(null);
+        ListView<String> listView = (ListView<String>) ((ComboBoxListViewSkin<String>) comboBox.getSkin())
+                .getPopupContent();
+
+        // Should not throw an NPE.
+        listView.getSelectionModel().select(1);
+    }
+
+    @Test public void testNullSelectionModelDoesNotThrowNPEOnSkinCreation() {
+        ComboBox<String> comboBox = new ComboBox<>();
+        comboBox.setSelectionModel(null);
+
+        // Should not throw an NPE.
+        comboBox.setSkin(new ComboBoxListViewSkin<>(comboBox));
+    }
+
     @Test public void selectionModelCanBeBound() {
         SingleSelectionModel<String> sm = ComboBoxShim.<String>get_ComboBoxSelectionModel(comboBox);
         ObjectProperty<SingleSelectionModel<String>> other = new SimpleObjectProperty<SingleSelectionModel<String>>(sm);