8244657: ChoiceBox/ToolBarSkin: misbehavior on switching skin

Reviewed-by: aghaisas

Author: Jeanette Winzenburg

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

@@ -206,6 +206,17 @@ public ChoiceBoxSkin(ChoiceBox<T> control) {
 
     /** {@inheritDoc} */
     @Override public void dispose() {
+         // removing the content listener fixes NPE from listener
+        if (choiceBoxItems != null) {
+            choiceBoxItems.removeListener(weakChoiceBoxItemsListener);
+            choiceBoxItems = null;
+        }
+        // removing the path listener fixes the memory leak on replacing skin
+        if (selectionModel != null) {
+            selectionModel.selectedIndexProperty().removeListener(selectionChangeListener);
+            selectionModel = null;
+        }
+
         super.dispose();
 
         if (behavior != null) {

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

@@ -105,7 +105,7 @@ public class ToolBarSkin extends SkinBase<ToolBar> {
     private final ParentTraversalEngine engine;
     private final BehaviorBase<ToolBar> behavior;
 
-
+    private ListChangeListener<Node> itemsListener;
 
     /***************************************************************************
      *                                                                         *
@@ -228,8 +228,8 @@ public Node selectLast(TraversalContext context) {
         });
         ParentHelper.setTraversalEngine(getSkinnable(), engine);
 
-        control.focusedProperty().addListener((observable, oldValue, newValue) -> {
-            if (newValue) {
+        registerChangeListener(control.focusedProperty(), ov -> {
+            if (getSkinnable().isFocused()) {
                 // TODO need to detect the focus direction
                 // to selected the first control in the toolbar when TAB is pressed
                 // or select the last control in the toolbar when SHIFT TAB is pressed.
@@ -241,7 +241,7 @@ public Node selectLast(TraversalContext context) {
             }
         });
 
-        control.getItems().addListener((ListChangeListener<Node>) c -> {
+        itemsListener = (ListChangeListener<Node>) c -> {
             while (c.next()) {
                 for (Node n: c.getRemoved()) {
                     box.getChildren().remove(n);
@@ -250,7 +250,8 @@ public Node selectLast(TraversalContext context) {
             }
             needsUpdate = true;
             getSkinnable().requestLayout();
-        });
+        };
+        control.getItems().addListener(itemsListener);
     }
 
 
@@ -365,6 +366,8 @@ public CssMetaData<ToolBar,Pos> getCssMetaData() {
 
     /** {@inheritDoc} */
     @Override public void dispose() {
+        if (getSkinnable() == null) return;
+        getSkinnable().getItems().removeListener(itemsListener);
         super.dispose();
 
         if (behavior != null) {

modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/SkinCleanupTest.java

@@ -0,0 +1,136 @@
+/*
+ * Copyright (c) 2020, 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
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.  Oracle designates this
+ * particular file as subject to the "Classpath" exception as provided
+ * by Oracle in the LICENSE file that accompanied this code.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+package test.javafx.scene.control.skin;
+
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+
+import static javafx.scene.control.ControlShim.*;
+import static org.junit.Assert.*;
+import static test.com.sun.javafx.scene.control.infrastructure.ControlSkinFactory.*;
+
+import javafx.scene.Scene;
+import javafx.scene.control.Button;
+import javafx.scene.control.ChoiceBox;
+import javafx.scene.control.Control;
+import javafx.scene.control.ToolBar;
+import javafx.scene.layout.Pane;
+import javafx.scene.layout.VBox;
+import javafx.scene.shape.Rectangle;
+import javafx.stage.Stage;
+
+/**
+ * Tests around the cleanup task JDK-8241364.
+ */
+public class SkinCleanupTest {
+
+    private Scene scene;
+    private Stage stage;
+    private Pane root;
+
+    /**
+     * NPE when adding items after skin is replaced
+     */
+    @Test
+    public void testChoiceBoxAddItems() {
+        ChoiceBox<String> box = new ChoiceBox<>();
+        installDefaultSkin(box);
+        replaceSkin(box);
+        box.getItems().add("added");
+    }
+
+    @Test
+    public void testToolBarAddItems() {
+        ToolBar bar = new ToolBar();
+        installDefaultSkin(bar);
+        replaceSkin(bar);
+        bar.getItems().add(new Rectangle());
+    }
+
+    /**
+     * Sanity test - fix changed listening to focusProperty, ensure
+     * that it's still working as before.
+     */
+    @Test
+    public void testToolBarFocus() {
+        ToolBar bar = new ToolBar();
+        bar.getItems().addAll(new Button("dummy"), new Button("other"));
+        showControl(bar, false);
+        Button outside = new Button("outside");
+        showControl(outside, true);
+        bar.requestFocus();
+        assertEquals("first item in toolbar must be focused", bar.getItems().get(0), scene.getFocusOwner());
+    }
+
+//---------------- setup and initial
+
+    /**
+     * Ensures the control is shown in an active scenegraph. Requests
+     * focus on the control if focused == true.
+     *
+     * @param control the control to show
+     * @param focused if true, requests focus on the added control
+     */
+    protected void showControl(Control control, boolean focused) {
+        if (root == null) {
+            root = new VBox();
+            scene = new Scene(root);
+            stage = new Stage();
+            stage.setScene(scene);
+        }
+        if (!root.getChildren().contains(control)) {
+            root.getChildren().add(control);
+        }
+        stage.show();
+        if (focused) {
+            stage.requestFocus();
+            control.requestFocus();
+            assertTrue(control.isFocused());
+            assertSame(control, scene.getFocusOwner());
+        }
+    }
+
+    @After
+    public void cleanup() {
+        if (stage != null) stage.hide();
+        Thread.currentThread().setUncaughtExceptionHandler(null);
+    }
+
+    @Before
+    public void setup() {
+        Thread.currentThread().setUncaughtExceptionHandler((thread, throwable) -> {
+            if (throwable instanceof RuntimeException) {
+                throw (RuntimeException)throwable;
+            } else {
+                Thread.currentThread().getThreadGroup().uncaughtException(thread, throwable);
+            }
+        });
+    }
+
+}
+
+

modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/SkinMemoryLeakTest.java

@@ -107,8 +107,6 @@ public static Collection<Object[]> data() {
         List<Class<? extends Control>> leakingClasses = List.of(
                 Accordion.class,
                 ButtonBar.class,
-                // @Ignore("8244657")
-                ChoiceBox.class,
                 ColorPicker.class,
                 ComboBox.class,
                 DatePicker.class,
@@ -132,7 +130,6 @@ public static Collection<Object[]> data() {
                 TextArea.class,
                 // @Ignore("8240506")
                 TextField.class,
-                ToolBar.class,
                 TreeCell.class,
                 TreeTableRow.class,
                 TreeTableView.class,