8245282: Button/Combo Behavior: memory leak on dispose

Reviewed-by: arapte
openjdk · Jun 2, 2020 · 853ac78 · 853ac78
1 parent a78b3fb
commit 853ac78
Show file tree

Hide file tree

Showing 3 changed files with 12 additions and 37 deletions.
diff --git a/...s/javafx.controls/src/main/java/com/sun/javafx/scene/control/behavior/ButtonBehavior.java b/...s/javafx.controls/src/main/java/com/sun/javafx/scene/control/behavior/ButtonBehavior.java
@@ -26,6 +26,8 @@
 
 import com.sun.javafx.PlatformUtil;
 import com.sun.javafx.scene.control.inputmap.KeyBinding;
+
+import javafx.beans.InvalidationListener;
 import javafx.beans.Observable;
 import javafx.scene.control.ButtonBase;
 import com.sun.javafx.scene.control.inputmap.InputMap;
@@ -56,6 +58,7 @@ public class ButtonBehavior<C extends ButtonBase> extends BehaviorBase<C> {
      */
     private boolean keyDown;
 
+    private InvalidationListener focusListener = this::focusChanged;
 
 
     /***************************************************************************
@@ -89,7 +92,7 @@ public ButtonBehavior(C control) {
         );
 
         // Button also cares about focus
-        control.focusedProperty().addListener(this::focusChanged);
+        control.focusedProperty().addListener(focusListener);
     }
 
 
@@ -105,10 +108,9 @@ public ButtonBehavior(C control) {
     }
 
     @Override public void dispose() {
+        // TODO specify contract of dispose and post-condition for getNode()
+        getNode().focusedProperty().removeListener(focusListener);
         super.dispose();
-
-        // TODO
-        getNode().focusedProperty().removeListener(this::focusChanged);
     }
 
 

diff --git a/...fx.controls/src/main/java/com/sun/javafx/scene/control/behavior/ComboBoxBaseBehavior.java b/...fx.controls/src/main/java/com/sun/javafx/scene/control/behavior/ComboBoxBaseBehavior.java
@@ -26,6 +26,8 @@
 package com.sun.javafx.scene.control.behavior;
 
 import com.sun.javafx.scene.control.inputmap.InputMap;
+
+import javafx.beans.InvalidationListener;
 import javafx.beans.Observable;
 import javafx.event.EventHandler;
 import javafx.event.EventTarget;
@@ -47,6 +49,7 @@
 public class ComboBoxBaseBehavior<T> extends BehaviorBase<ComboBoxBase<T>> {
 
     private final InputMap<ComboBoxBase<T>> inputMap;
+    private InvalidationListener focusListener = this::focusChanged;
 
     /***************************************************************************
      *                                                                         *
@@ -102,7 +105,7 @@ public ComboBoxBaseBehavior(final ComboBoxBase<T> comboBox) {
         enterReleased.setAutoConsume(false);
 
         // ComboBoxBase also cares about focus
-        comboBox.focusedProperty().addListener(this::focusChanged);
+        comboBox.focusedProperty().addListener(focusListener);
 
         // Only add this if we're on an embedded platform that supports 5-button navigation
         if (Utils.isTwoLevelFocus()) {
@@ -112,7 +115,7 @@ public ComboBoxBaseBehavior(final ComboBoxBase<T> comboBox) {
 
     @Override public void dispose() {
         if (tlFocus != null) tlFocus.dispose();
-        getNode().focusedProperty().removeListener(this::focusChanged);
+        getNode().focusedProperty().removeListener(focusListener);
         super.dispose();
     }
 

diff --git a/...rols/src/test/java/test/com/sun/javafx/scene/control/behavior/BehaviorMemoryLeakTest.java b/...rols/src/test/java/test/com/sun/javafx/scene/control/behavior/BehaviorMemoryLeakTest.java
@@ -41,22 +41,12 @@
 import static org.junit.Assert.*;
 import static test.com.sun.javafx.scene.control.infrastructure.ControlSkinFactory.*;
 
-import javafx.scene.control.Button;
-import javafx.scene.control.CheckBox;
-import javafx.scene.control.ColorPicker;
-import javafx.scene.control.ComboBox;
 import javafx.scene.control.Control;
-import javafx.scene.control.DatePicker;
-import javafx.scene.control.Hyperlink;
 import javafx.scene.control.ListView;
-import javafx.scene.control.MenuButton;
 import javafx.scene.control.PasswordField;
-import javafx.scene.control.RadioButton;
-import javafx.scene.control.SplitMenuButton;
 import javafx.scene.control.TableView;
 import javafx.scene.control.TextArea;
 import javafx.scene.control.TextField;
-import javafx.scene.control.ToggleButton;
 import javafx.scene.control.TreeTableView;
 import javafx.scene.control.TreeView;
 import test.com.sun.javafx.scene.control.infrastructure.ControlSkinFactory;
@@ -86,39 +76,19 @@ public void testMemoryLeakDisposeBehavior() {
     //---------------- parameterized
 
     // Note: name property not supported before junit 4.11
-    @Parameterized.Parameters //(name = "{index}: {0} ")
+    @Parameterized.Parameters // (name = "{index}: {0} ")
     public static Collection<Object[]> data() {
         List<Class<Control>> controlClasses = getControlClassesWithBehavior();
         // FIXME as part of JDK-8241364
         // The behaviors of these controls are leaking
         // 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(
-                // @Ignore("8245282")
-                Button.class,
-                // @Ignore("8245282")
-                CheckBox.class,
-                // @Ignore("8245282")
-                ColorPicker.class,
-                // @Ignore("8245282")
-                ComboBox.class,
-                // @Ignore("8245282")
-                DatePicker.class,
-                // @Ignore("8245282")
-                Hyperlink.class,
                 ListView.class,
-                // @Ignore("8245282")
-                MenuButton.class,
                 PasswordField.class,
-                // @Ignore("8245282")
-                RadioButton.class,
-                // @Ignore("8245282")
-                SplitMenuButton.class,
                 TableView.class,
                 TextArea.class,
                 TextField.class,
-                // @Ignore("8245282")
-                ToggleButton.class,
                 TreeTableView.class,
                 TreeView.class
          );