8269081: Tree/ListViewSkin: must remove flow on dispose

Reviewed-by: arapte

Author: Jeanette Winzenburg

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

@@ -175,8 +175,6 @@ public class ListViewSkin<T> extends VirtualContainerBase<ListView<T>, ListCell<
     private WeakInvalidationListener
                 weakItemsChangeListener = new WeakInvalidationListener(itemsChangeListener);
 
-    private EventHandler<MouseEvent> ml;
-
     /* *************************************************************************
      *                                                                         *
      * Constructors                                                            *
@@ -218,7 +216,7 @@ public ListViewSkin(final ListView<T> control) {
         flow.setFixedCellSize(control.getFixedCellSize());
         getChildren().add(flow);
 
-        ml = event -> {
+        EventHandler<MouseEvent> 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.
@@ -281,12 +279,7 @@ public ListViewSkin(final ListView<T> control) {
             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);
+        getChildren().remove(flow);
         super.dispose();
 
         if (behavior != null) {

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

@@ -136,11 +136,6 @@ public class TreeViewSkin<T> extends VirtualContainerBase<TreeView<T>, TreeCell<
     private WeakEventHandler<TreeModificationEvent<T>> weakRootListener;
 
 
-
-    private EventHandler<MouseEvent> ml;
-
-
-
     /* *************************************************************************
      *                                                                         *
      * Constructors                                                            *
@@ -170,7 +165,7 @@ public TreeViewSkin(final TreeView control) {
 
         setRoot(getSkinnable().getRoot());
 
-        ml = event -> {
+        EventHandler<MouseEvent> 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.
@@ -235,13 +230,7 @@ public TreeViewSkin(final TreeView control) {
 
         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);
-
+        getChildren().remove(flow);
         super.dispose();
 
         if (behavior != null) {

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

@@ -35,10 +35,13 @@
 import org.junit.runner.RunWith;
 import org.junit.runners.Parameterized;
 
+import com.sun.javafx.tk.Toolkit;
+
 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.Accordion;
 import javafx.scene.control.ButtonBar;
 import javafx.scene.control.ColorPicker;
@@ -51,13 +54,18 @@
 import javafx.scene.control.PasswordField;
 import javafx.scene.control.ScrollBar;
 import javafx.scene.control.ScrollPane;
+import javafx.scene.control.Separator;
+import javafx.scene.control.Skin;
 import javafx.scene.control.Spinner;
 import javafx.scene.control.SplitMenuButton;
 import javafx.scene.control.SplitPane;
 import javafx.scene.control.TableRow;
 import javafx.scene.control.TableView;
 import javafx.scene.control.TreeTableRow;
 import javafx.scene.control.TreeTableView;
+import javafx.scene.layout.Pane;
+import javafx.scene.layout.VBox;
+import javafx.stage.Stage;
 
 /**
  * Test memory leaks in Skin implementations.
@@ -78,12 +86,39 @@ public class SkinMemoryLeakTest {
     @Test
     public void testMemoryLeakAlternativeSkin() {
         installDefaultSkin(control);
+        // FIXME: JDK-8265406 - fragile test pattern
         WeakReference<?> weakRef = new WeakReference<>(replaceSkin(control));
         assertNotNull(weakRef.get());
         attemptGC(weakRef);
         assertEquals("Skin must be gc'ed", null, weakRef.get());
     }
 
+    /**
+     * default skin -> set alternative while showing
+     */
+    @Test
+    public void testMemoryLeakAlternativeSkinShowing() {
+        showControl(control, true);
+        Skin<?> replacedSkin = replaceSkin(control);
+        WeakReference<?> weakRef = new WeakReference<>(replacedSkin);
+        assertNotNull(weakRef.get());
+        // beware: this is important - we might get false reds without!
+        Toolkit.getToolkit().firePulse();
+        replacedSkin = null;
+        attemptGC(weakRef);
+        assertEquals("Skin must be gc'ed", null, weakRef.get());
+    }
+
+    @Test
+    public void testControlChildren() {
+        installDefaultSkin(control);
+        int childCount = control.getChildrenUnmodifiable().size();
+        String skinClass = control.getSkin().getClass().getSimpleName();
+        replaceSkin(control);
+        assertEquals(skinClass + " must remove direct children that it has added",
+                childCount, control.getChildrenUnmodifiable().size());
+    }
+
 //------------ parameters
 
     // Note: name property not supported before junit 4.11
@@ -106,6 +141,8 @@ public static Collection<Object[]> data() {
                 PasswordField.class,
                 ScrollBar.class,
                 ScrollPane.class,
+                // @Ignore("8273071")
+                Separator.class,
                 // @Ignore("8245145")
                 Spinner.class,
                 SplitMenuButton.class,
@@ -126,6 +163,36 @@ public SkinMemoryLeakTest(Class<Control> controlClass) {
 
 //------------ setup
 
+    private Scene scene;
+    private Stage stage;
+    private Pane root;
+
+   /**
+     * 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());
+        }
+    }
+
     @Before
     public void setup() {
         Thread.currentThread().setUncaughtExceptionHandler((thread, throwable) -> {
@@ -141,6 +208,7 @@ public void setup() {
 
     @After
     public void cleanup() {
+        if (stage != null) stage.hide();
         Thread.currentThread().setUncaughtExceptionHandler(null);
     }