8256821: TreeViewSkin/Behavior: misbehavior on switching skin

Reviewed-by: arapte

Author: Jeanette Winzenburg

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

@@ -154,14 +154,6 @@ public class TreeViewBehavior<T> extends BehaviorBase<TreeView<T>> {
     public TreeViewBehavior(TreeView<T> control) {
         super(control);
 
-//        // Fix for RT-16565
-//        getNode().selectionModelProperty().addListener(weakSelectionModelListener);
-//        if (control.getSelectionModel() != null) {
-//            control.getSelectionModel().getSelectedIndices().addListener(weakSelectedIndicesListener);
-//        }
-
-
-
         // create a map for treeView-specific mappings
         treeViewInputMap = createInputMap();
 
@@ -255,6 +247,12 @@ public TreeViewBehavior(TreeView<T> control) {
     }
 
     @Override public void dispose() {
+        getNode().selectionModelProperty().removeListener(weakSelectionModelListener);
+        MultipleSelectionModel<TreeItem<T>> sm = getNode().getSelectionModel();
+        if (sm != null) {
+            sm.getSelectedIndices().removeListener(weakSelectedIndicesListener);
+        }
+        getNode().removeEventFilter(KeyEvent.ANY, keyEventListener);
         TreeCellBehavior.removeAnchor(getNode());
         super.dispose();
     }

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

@@ -136,6 +136,10 @@ public class TreeViewSkin<T> extends VirtualContainerBase<TreeView<T>, TreeCell<
 
 
 
+    private EventHandler<MouseEvent> ml;
+
+
+
     /***************************************************************************
      *                                                                         *
      * Constructors                                                            *
@@ -165,7 +169,7 @@ public TreeViewSkin(final TreeView control) {
 
         setRoot(getSkinnable().getRoot());
 
-        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.
@@ -226,6 +230,17 @@ public TreeViewSkin(final TreeView control) {
 
     /** {@inheritDoc} */
     @Override public void dispose() {
+        if (getSkinnable() == null) return;
+
+        getSkinnable().getProperties().removeListener(propertiesMapListener);
+        setRoot(null);
+        // leaking without nulling factory
+        flow.setCellFactory(null);
+        // for completeness - but no effect with/out? Same as in ListViewSkin
+        // don't without seeing any effect - it's not on the skinnable, but on a child, so shouldn't
+        flow.getVbar().removeEventFilter(MouseEvent.MOUSE_PRESSED, ml);
+        flow.getHbar().removeEventFilter(MouseEvent.MOUSE_PRESSED, ml);
+
         super.dispose();
 
         if (behavior != null) {

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

@@ -39,6 +39,8 @@
 import static test.com.sun.javafx.scene.control.infrastructure.ControlSkinFactory.*;
 
 import javafx.scene.control.ListView;
+import javafx.scene.control.TreeItem;
+import javafx.scene.control.TreeView;
 
 /**
  * Test for misbehavior of individual implementations that turned
@@ -47,6 +49,51 @@
  */
 public class BehaviorCleanupTest {
 
+//----------- TreeView
+
+    /**
+     * Test cleanup of selection listeners in TreeViewBehavior.
+     */
+    @Test
+    public void testTreeViewBehaviorDisposeSelect() {
+        TreeView<String> treeView = new TreeView<>(createRoot());
+        WeakReference<BehaviorBase<?>> weakRef = new WeakReference<>(createBehavior(treeView));
+        treeView.getSelectionModel().select(1);
+        weakRef.get().dispose();
+        treeView.getSelectionModel().select(0);
+        assertNull("anchor must remain cleared on selecting when disposed",
+                treeView.getProperties().get("anchor"));
+    }
+
+    @Test
+    public void testTreeViewBehaviorSelect() {
+        TreeView<String> treeView = new TreeView<>(createRoot());
+        createBehavior(treeView);
+        int last = 1;
+        treeView.getSelectionModel().select(last);
+        assertEquals("anchor must be set", last, treeView.getProperties().get("anchor"));
+    }
+
+    @Test
+    public void testTreeViewBehaviorDispose() {
+        TreeView<String> treeView = new TreeView<>(createRoot());
+        WeakReference<BehaviorBase<?>> weakRef = new WeakReference<>(createBehavior(treeView));
+        treeView.getSelectionModel().select(1);
+        weakRef.get().dispose();
+        assertNull("anchor must be cleared after dispose", treeView.getProperties().get("anchor"));
+    }
+
+    /**
+     * Creates and returns an expanded treeItem with two children.
+     */
+    private TreeItem<String> createRoot() {
+        TreeItem<String> root = new TreeItem<>("root");
+        root.setExpanded(true);
+        root.getChildren().addAll(new TreeItem<>("child one"), new TreeItem<>("child two"));
+        return root;
+    }
+
+
 // ---------- ListView
 
     /**

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

@@ -46,7 +46,6 @@
 import javafx.scene.control.TextArea;
 import javafx.scene.control.TextField;
 import javafx.scene.control.TreeTableView;
-import javafx.scene.control.TreeView;
 
 /**
  * Test for memory leaks in Behavior implementations.
@@ -86,8 +85,7 @@ public static Collection<Object[]> data() {
                 TableView.class,
                 TextArea.class,
                 TextField.class,
-                TreeTableView.class,
-                TreeView.class
+                TreeTableView.class
          );
         // remove the known issues to make the test pass
         controlClasses.removeAll(leakingClasses);

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

@@ -25,9 +25,10 @@
 
 package test.javafx.scene.control.skin;
 
+import java.lang.ref.WeakReference;
+
 import org.junit.After;
 import org.junit.Before;
-import org.junit.Ignore;
 import org.junit.Test;
 
 import static javafx.collections.FXCollections.*;
@@ -43,6 +44,7 @@
 import javafx.scene.control.ListView;
 import javafx.scene.control.ToolBar;
 import javafx.scene.control.TreeCell;
+import javafx.scene.control.TreeItem;
 import javafx.scene.control.TreeView;
 import javafx.scene.layout.Pane;
 import javafx.scene.layout.VBox;
@@ -58,6 +60,77 @@ public class SkinCleanupTest {
     private Stage stage;
     private Pane root;
 
+  //---------------- TreeView
+
+    /**
+     * Sanity: replacing the root has no side-effect, listener to rootProperty
+     * is registered with skin api
+     */
+    @Test
+    public void testTreeViewSetRoot() {
+        TreeView<String> treeView = new TreeView<>(createRoot());
+        installDefaultSkin(treeView);
+        replaceSkin(treeView);
+        treeView.setRoot(createRoot());
+    }
+
+    /**
+     * NPE from event handler to treeModification of root.
+     */
+    @Test
+    public void testTreeViewAddRootChild() {
+        TreeView<String> treeView = new TreeView<>(createRoot());
+        installDefaultSkin(treeView);
+        replaceSkin(treeView);
+        treeView.getRoot().getChildren().add(createRoot());
+    }
+
+    /**
+     * NPE from event handler to treeModification of root.
+     */
+    @Test
+    public void testTreeViewReplaceRootChildren() {
+        TreeView<String> treeView = new TreeView<>(createRoot());
+        installDefaultSkin(treeView);
+        replaceSkin(treeView);
+        treeView.getRoot().getChildren().setAll(createRoot().getChildren());
+    }
+
+    /**
+     * NPE due to properties listener not removed
+     */
+    @Test
+    public void testTreeViewRefresh() {
+        TreeView<String> treeView = new TreeView<>();
+        installDefaultSkin(treeView);
+        replaceSkin(treeView);
+        treeView.refresh();
+    }
+
+    /**
+     * Sanity: guard against potential memory leak from root property listener.
+     */
+    @Test
+    public void testMemoryLeakAlternativeSkinWithRoot() {
+        TreeView<String> treeView = new TreeView<>(createRoot());
+        installDefaultSkin(treeView);
+        WeakReference<?> weakRef = new WeakReference<>(replaceSkin(treeView));
+        assertNotNull(weakRef.get());
+        attemptGC(weakRef);
+        assertEquals("Skin must be gc'ed", null, weakRef.get());
+    }
+
+    /**
+     * Creates and returns an expanded treeItem with two children
+     */
+    private TreeItem<String> createRoot() {
+        TreeItem<String> root = new TreeItem<>("root");
+        root.setExpanded(true);
+        root.getChildren().addAll(new TreeItem<>("child one"), new TreeItem<>("child two"));
+        return root;
+    }
+
+
 // ------------------ TreeCell
 
     @Test

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

@@ -61,7 +61,6 @@
 import javafx.scene.control.TextField;
 import javafx.scene.control.TreeTableRow;
 import javafx.scene.control.TreeTableView;
-import javafx.scene.control.TreeView;
 
 /**
  * Test memory leaks in Skin implementations.
@@ -123,8 +122,7 @@ public static Collection<Object[]> data() {
                 // @Ignore("8240506")
                 TextField.class,
                 TreeTableRow.class,
-                TreeTableView.class,
-                TreeView.class
+                TreeTableView.class
         );
         // remove the known issues to make the test pass
         controlClasses.removeAll(leakingClasses);