diff --git a/modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/behavior/ListViewBehavior.java b/modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/behavior/ListViewBehavior.java index b70fae93ae2..cfc5c1ab7d3 100644 --- a/modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/behavior/ListViewBehavior.java +++ b/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 control) { ListView 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 * *************************************************************************/ diff --git a/modules/javafx.controls/src/main/java/javafx/scene/control/skin/ListViewSkin.java b/modules/javafx.controls/src/main/java/javafx/scene/control/skin/ListViewSkin.java index a0cc6dfc9d5..5c7d0f7c762 100644 --- a/modules/javafx.controls/src/main/java/javafx/scene/control/skin/ListViewSkin.java +++ b/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 extends VirtualContainerBase, ListCell< private Node placeholderNode; private ObservableList listViewItems; - private final InvalidationListener itemsChangeListener = observable -> updateListViewItems(); private boolean needCellsRebuilt = true; private boolean needCellsReconfigured = false; @@ -129,6 +129,9 @@ public class ListViewSkin extends VirtualContainerBase, ListCell< } }; + private WeakMapChangeListener weakPropertiesMapListener = + new WeakMapChangeListener<>(propertiesMapListener); + private final ListChangeListener listViewItemsListener = new ListChangeListener() { @Override public void onChanged(Change c) { while (c.next()) { @@ -166,6 +169,12 @@ public class ListViewSkin extends VirtualContainerBase, ListCell< new WeakListChangeListener(listViewItemsListener); + private final InvalidationListener itemsChangeListener = observable -> updateListViewItems(); + + private WeakInvalidationListener + weakItemsChangeListener = new WeakInvalidationListener(itemsChangeListener); + + private EventHandler ml; /*************************************************************************** * * @@ -208,7 +217,7 @@ public ListViewSkin(final ListView control) { flow.setFixedCellSize(control.getFixedCellSize()); getChildren().add(flow); - EventHandler 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 control) { updateItemCount(); - control.itemsProperty().addListener(new WeakInvalidationListener(itemsChangeListener)); + control.itemsProperty().addListener(weakItemsChangeListener); final ObservableMap 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 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) { diff --git a/modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualContainerBase.java b/modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualContainerBase.java index 5dc17aeb2a5..1b383eed17d 100644 --- a/modules/javafx.controls/src/main/java/javafx/scene/control/skin/VirtualContainerBase.java +++ b/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 flow; + private EventHandler> 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 createVirtualFlow() { return new VirtualFlow<>(); } + /** + * {@inheritDoc}

+ * 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. diff --git a/modules/javafx.controls/src/test/java/test/com/sun/javafx/scene/control/behavior/BehaviorCleanupTest.java b/modules/javafx.controls/src/test/java/test/com/sun/javafx/scene/control/behavior/BehaviorCleanupTest.java new file mode 100644 index 00000000000..c6d10dc8892 --- /dev/null +++ b/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 listView = new ListView<>(observableArrayList("one", "two")); + WeakReference> 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 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 listView = new ListView<>(observableArrayList("one", "two")); + WeakReference> 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 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 listView = new ListView<>(observableArrayList("one", "two")); + WeakReference> 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 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 listView = new ListView<>(observableArrayList("one", "two")); + WeakReference> 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); + } + }); + } + +} diff --git a/modules/javafx.controls/src/test/java/test/com/sun/javafx/scene/control/behavior/BehaviorMemoryLeakTest.java b/modules/javafx.controls/src/test/java/test/com/sun/javafx/scene/control/behavior/BehaviorMemoryLeakTest.java index f8c1ea2be5e..028ff7511a5 100644 --- a/modules/javafx.controls/src/test/java/test/com/sun/javafx/scene/control/behavior/BehaviorMemoryLeakTest.java +++ b/modules/javafx.controls/src/test/java/test/com/sun/javafx/scene/control/behavior/BehaviorMemoryLeakTest.java @@ -84,7 +84,6 @@ public static Collection data() { // step 1: file issues (where not yet done), add informal ignore to entry // step 2: fix and remove from list List> leakingClasses = List.of( - ListView.class, PasswordField.class, TableView.class, TextArea.class, diff --git a/modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/SkinCleanupTest.java b/modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/SkinCleanupTest.java index 8693361fd0f..3c3f485fb1b 100644 --- a/modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/SkinCleanupTest.java +++ b/modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/SkinCleanupTest.java @@ -27,8 +27,10 @@ import org.junit.After; import org.junit.Before; +import org.junit.Ignore; import org.junit.Test; +import static javafx.collections.FXCollections.*; import static javafx.scene.control.ControlShim.*; import static org.junit.Assert.*; import static test.com.sun.javafx.scene.control.infrastructure.ControlSkinFactory.*; @@ -37,6 +39,7 @@ import javafx.scene.control.Button; import javafx.scene.control.ChoiceBox; import javafx.scene.control.Control; +import javafx.scene.control.ListView; import javafx.scene.control.ToolBar; import javafx.scene.layout.Pane; import javafx.scene.layout.VBox; @@ -52,6 +55,47 @@ public class SkinCleanupTest { private Stage stage; private Pane root; + //-------------- listView + + @Test + public void testListViewAddItems() { + ListView listView = new ListView<>(); + installDefaultSkin(listView); + replaceSkin(listView); + listView.getItems().add("addded"); + } + + @Test + public void testListViewRefresh() { + ListView listView = new ListView<>(); + installDefaultSkin(listView); + replaceSkin(listView); + listView.refresh(); + } + + @Test + public void testListViewSetItems() { + ListView listView = new ListView<>(); + installDefaultSkin(listView); + replaceSkin(listView); + listView.setItems(observableArrayList()); + } + +//-------- choiceBox, toolBar + + /** + * FIXME: Left-over from ChoiceBox fix. + * NPE on sequence setItems -> modify items after skin is replaced. + */ + @Test @Ignore("8246202") + public void testChoiceBoxSetItems() { + ChoiceBox box = new ChoiceBox<>(); + installDefaultSkin(box); + replaceSkin(box); + box.setItems(observableArrayList("one")); + box.getItems().add("added"); + } + /** * NPE when adding items after skin is replaced */ diff --git a/modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/SkinMemoryLeakTest.java b/modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/SkinMemoryLeakTest.java index 1a7b5996757..dc510d6301d 100644 --- a/modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/SkinMemoryLeakTest.java +++ b/modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/SkinMemoryLeakTest.java @@ -111,7 +111,6 @@ public static Collection data() { ComboBox.class, DatePicker.class, ListCell.class, - ListView.class, MenuBar.class, MenuButton.class, Pagination.class,