8246195: ListViewSkin/Behavior: misbehavior on switching skin

modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/behavior/ListViewBehavior.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2013, 2016, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2013, 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
@@ -213,16 +213,20 @@ public ListViewBehavior(ListView<T> control) {
         ListView<T> control = getNode();
 
         ListCellBehavior.removeAnchor(control);
+        control.selectionModelProperty().removeListener(weakSelectionModelListener);
+        if (control.getSelectionModel() != null) {
+            control.getSelectionModel().getSelectedIndices().removeListener(weakSelectedIndicesListener);
+        }
+        control.itemsProperty().removeListener(weakItemsListener);
+        if (control.getItems() != null) {
+            control.getItems().removeListener(weakItemsListListener);
+        }
+
         if (tlFocus != null) tlFocus.dispose();
+        control.removeEventFilter(KeyEvent.ANY, keyEventListener);
         super.dispose();
-
-        control.removeEventHandler(KeyEvent.ANY, keyEventListener);
     }
 
-
-
-
-
     /**************************************************************************
      *                         State and Functions                            *
      *************************************************************************/

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

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2010, 2018, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2010, 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
@@ -37,6 +37,7 @@
 import javafx.collections.ObservableList;
 import javafx.collections.ObservableMap;
 import javafx.collections.WeakListChangeListener;
+import javafx.collections.WeakMapChangeListener;
 import javafx.event.EventHandler;
 import javafx.geometry.Orientation;
 import javafx.scene.AccessibleAction;
@@ -104,7 +105,6 @@ public class ListViewSkin<T> extends VirtualContainerBase<ListView<T>, ListCell<
     private Node placeholderNode;
 
     private ObservableList<T> listViewItems;
-    private final InvalidationListener itemsChangeListener = observable -> updateListViewItems();
 
     private boolean needCellsRebuilt = true;
     private boolean needCellsReconfigured = false;
@@ -129,6 +129,9 @@ public class ListViewSkin<T> extends VirtualContainerBase<ListView<T>, ListCell<
         }
     };
 
+    private WeakMapChangeListener<Object, Object> weakPropertiesMapListener =
+            new WeakMapChangeListener<>(propertiesMapListener);
+
     private final ListChangeListener<T> listViewItemsListener = new ListChangeListener<T>() {
         @Override public void onChanged(Change<? extends T> c) {
             while (c.next()) {
@@ -166,6 +169,12 @@ public class ListViewSkin<T> extends VirtualContainerBase<ListView<T>, ListCell<
             new WeakListChangeListener<T>(listViewItemsListener);
 
 
+    private final InvalidationListener itemsChangeListener = observable -> updateListViewItems();
+
+    private WeakInvalidationListener
+                weakItemsChangeListener = new WeakInvalidationListener(itemsChangeListener);
+
+    private EventHandler<MouseEvent> ml;
 
     /***************************************************************************
      *                                                                         *
@@ -208,7 +217,7 @@ public ListViewSkin(final ListView<T> control) {
         flow.setFixedCellSize(control.getFixedCellSize());
         getChildren().add(flow);
 
-        EventHandler<MouseEvent> ml = event -> {
+        ml = event -> {
             // RT-15127: cancel editing on scroll. This is a bit extreme
             // (we are cancelling editing on touching the scrollbars).
             // This can be improved at a later date.
@@ -230,11 +239,11 @@ public ListViewSkin(final ListView<T> control) {
 
         updateItemCount();
 
-        control.itemsProperty().addListener(new WeakInvalidationListener(itemsChangeListener));
+        control.itemsProperty().addListener(weakItemsChangeListener);
 
         final ObservableMap<Object, Object> properties = control.getProperties();
         properties.remove(Properties.RECREATE);
-        properties.addListener(propertiesMapListener);
+        properties.addListener(weakPropertiesMapListener);
 
         // Register listeners
         registerChangeListener(control.itemsProperty(), o -> updateListViewItems());
@@ -263,6 +272,20 @@ public ListViewSkin(final ListView<T> control) {
 
     /** {@inheritDoc} */
     @Override public void dispose() {
+        if (getSkinnable() == null) return;
+        // listener cleanup fixes side-effects (NPE on refresh, setItems, modifyItems)
+        getSkinnable().getProperties().removeListener(weakPropertiesMapListener);
+        getSkinnable().itemsProperty().removeListener(weakItemsChangeListener);
+        if (listViewItems != null) {
+            listViewItems.removeListener(weakListViewItemsListener);
+            listViewItems = null;
+        }
+        // flow related cleanup
+        // leaking without nulling factory
+        flow.setCellFactory(null);
+        // for completeness - but no effect with/out?
+        flow.getVbar().removeEventFilter(MouseEvent.MOUSE_PRESSED, ml);
+        flow.getHbar().removeEventFilter(MouseEvent.MOUSE_PRESSED, ml);
         super.dispose();
 
         if (behavior != null) {

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

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2010, 2017, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2010, 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
@@ -25,6 +25,7 @@
 
 package javafx.scene.control.skin;
 
+import javafx.event.EventHandler;
 import javafx.scene.control.Control;
 import javafx.scene.control.IndexedCell;
 import javafx.scene.control.ScrollToEvent;
@@ -53,6 +54,7 @@ public abstract class VirtualContainerBase<C extends Control, I extends IndexedC
      */
     private final VirtualFlow<I> flow;
 
+    private EventHandler<? super ScrollToEvent<Integer>> scrollToEventHandler;
 
 
     /***************************************************************************
@@ -69,7 +71,7 @@ public VirtualContainerBase(final C control) {
         super(control);
         flow = createVirtualFlow();
 
-        control.addEventHandler(ScrollToEvent.scrollToTopIndex(), event -> {
+        scrollToEventHandler = event -> {
             // Fix for RT-24630: The row count in VirtualFlow was incorrect
             // (normally zero), so the scrollTo call was misbehaving.
             if (itemCountDirty) {
@@ -78,7 +80,8 @@ public VirtualContainerBase(final C control) {
                 itemCountDirty = false;
             }
             flow.scrollToTop(event.getScrollTarget());
-        });
+        };
+        control.addEventHandler(ScrollToEvent.scrollToTopIndex(), scrollToEventHandler);
     }
 
 
@@ -123,6 +126,17 @@ protected VirtualFlow<I> createVirtualFlow() {
         return new VirtualFlow<>();
     }
 
+    /**
+     * {@inheritDoc} <p>
+     * Overridden to remove EventHandler.
+     */
+    @Override
+    public void dispose() {
+        if (getSkinnable() == null) return;
+        getSkinnable().removeEventHandler(ScrollToEvent.scrollToTopIndex(), scrollToEventHandler);
+        super.dispose();
+    }
+
     /**
      * Get the virtualized container.
      * Subclasses can invoke this method to get the VirtualFlow instance.

modules/javafx.controls/src/test/java/test/com/sun/javafx/scene/control/behavior/BehaviorCleanupTest.java

@@ -0,0 +1,160 @@
+/*
+ * 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.com.sun.javafx.scene.control.behavior;
+
+import java.lang.ref.WeakReference;
+
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+
+import com.sun.javafx.scene.control.behavior.BehaviorBase;
+import com.sun.javafx.scene.control.behavior.ListCellBehavior;
+
+import static javafx.collections.FXCollections.*;
+import static org.junit.Assert.*;
+import static test.com.sun.javafx.scene.control.infrastructure.ControlSkinFactory.*;
+
+import javafx.scene.control.ListView;
+
+/**
+ * Test for misbehavior of individual implementations that turned
+ * up in binch testing.
+ *
+ */
+public class BehaviorCleanupTest {
+
+// ---------- ListView
+
+    /**
+     * Test cleanup of listener to itemsProperty.
+     */
+    @Test
+    public void testListViewBehaviorDisposeSetItems() {
+        ListView<String> listView = new ListView<>(observableArrayList("one", "two"));
+        WeakReference<BehaviorBase<?>> weakRef = new WeakReference<>(createBehavior(listView));
+        weakRef.get().dispose();
+        int last = 1;
+        ListCellBehavior.setAnchor(listView, last, false);
+        listView.setItems(observableArrayList("other", "again"));
+        assertEquals("sanity: anchor unchanged", last, listView.getProperties().get("anchor"));
+        listView.getItems().remove(0);
+        assertEquals("anchor must not be updated on items modification when disposed",
+                last, listView.getProperties().get("anchor"));
+    }
+
+    @Test
+    public void testListViewBehaviorSetItems() {
+        ListView<String> listView = new ListView<>(observableArrayList("one", "two"));
+        createBehavior(listView);
+        int last = 1;
+        ListCellBehavior.setAnchor(listView, last, false);
+        listView.setItems(observableArrayList("other", "again"));
+        assertEquals("sanity: anchor unchanged", last, listView.getProperties().get("anchor"));
+        listView.getItems().remove(0);
+        assertEquals("anchor must be updated on items modification",
+                last -1, listView.getProperties().get("anchor"));
+   }
+
+    /**
+     * Test cleanup of itemsList listener in ListViewBehavior.
+     */
+    @Test
+    public void testListViewBehaviorDisposeRemoveItem() {
+        ListView<String> listView = new ListView<>(observableArrayList("one", "two"));
+        WeakReference<BehaviorBase<?>> weakRef = new WeakReference<>(createBehavior(listView));
+        weakRef.get().dispose();
+        int last = 1;
+        ListCellBehavior.setAnchor(listView, last, false);
+        listView.getItems().remove(0);
+        assertEquals("anchor must not be updated on items modification when disposed",
+                last,
+                listView.getProperties().get("anchor"));
+    }
+
+    @Test
+    public void testListViewBehaviorRemoveItem() {
+        ListView<String> listView = new ListView<>(observableArrayList("one", "two"));
+        createBehavior(listView);
+        int last = 1;
+        ListCellBehavior.setAnchor(listView, last, false);
+        assertEquals("behavior must set anchor on select", last, listView.getProperties().get("anchor"));
+        listView.getItems().remove(0);
+        assertEquals("anchor must be updated on items modification",
+                last -1, listView.getProperties().get("anchor"));
+    }
+
+    /**
+     * Test cleanup of selection listeners in ListViewBehavior.
+     */
+    @Test
+    public void testListViewBehaviorDisposeSelect() {
+        ListView<String> listView = new ListView<>(observableArrayList("one", "two"));
+        WeakReference<BehaviorBase<?>> weakRef = new WeakReference<>(createBehavior(listView));
+        listView.getSelectionModel().select(1);
+        weakRef.get().dispose();
+        listView.getSelectionModel().select(0);
+        assertNull("anchor must remain cleared on selecting when disposed",
+                listView.getProperties().get("anchor"));
+    }
+
+    @Test
+    public void testListViewBehaviorSelect() {
+        ListView<String> listView = new ListView<>(observableArrayList("one", "two"));
+        createBehavior(listView);
+        int last = 1;
+        listView.getSelectionModel().select(last);
+        assertEquals("anchor must be set", last, listView.getProperties().get("anchor"));
+    }
+
+    @Test
+    public void testListViewBehaviorDispose() {
+        ListView<String> listView = new ListView<>(observableArrayList("one", "two"));
+        WeakReference<BehaviorBase<?>> weakRef = new WeakReference<>(createBehavior(listView));
+        listView.getSelectionModel().select(1);
+        weakRef.get().dispose();
+        assertNull("anchor must be cleared after dispose", listView.getProperties().get("anchor"));
+    }
+
+  //------------------ setup/cleanup
+
+    @After
+    public void cleanup() {
+        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/com/sun/javafx/scene/control/behavior/BehaviorMemoryLeakTest.java

@@ -84,7 +84,6 @@ public static Collection<Object[]> data() {
         // step 1: file issues (where not yet done), add informal ignore to entry
         // step 2: fix and remove from list
         List<Class<? extends Control>> leakingClasses = List.of(
-                ListView.class,
                 PasswordField.class,
                 TableView.class,
                 TextArea.class,