8296409: Multiple copies of accelerator change listeners are added to…

… MenuItems, but only 1 is removed Reviewed-by: angorya, arapte
openjdk · Dec 19, 2022 · bac8ee8 · bac8ee8
1 parent ae86ed3
commit bac8ee8
Show file tree

Hide file tree

Showing 2 changed files with 54 additions and 15 deletions.
diff --git a/...javafx.controls/src/main/java/com/sun/javafx/scene/control/ControlAcceleratorSupport.java b/...javafx.controls/src/main/java/com/sun/javafx/scene/control/ControlAcceleratorSupport.java
@@ -75,24 +75,18 @@ public static void addAcceleratorsIntoScene(ObservableList<MenuItem> items, Node
             throw new IllegalArgumentException("Anchor cannot be null");
         }
 
-        final Scene scene = anchor.getScene();
-        if (scene != null) {
-            doAcceleratorInstall(items, scene);
-        }
-        // Scene change listener is added to the anchor for scenarios like,
-        // 1. Installing accelerators when Control is added to Scene
-        // 2. Removing accelerators when Control is removed from Scene
-        // Remove previously added listener if any
+
         WeakReference<ChangeListener<Scene>> listenerW = sceneChangeListenerMap.get(anchor);
-        if (listenerW != null) {
-            ChangeListener<Scene> listener = listenerW.get();
-            if (listener != null) {
-                anchor.sceneProperty().removeListener(listener);
+        if (listenerW == null || listenerW.get() == null) {
+            final Scene scene = anchor.getScene();
+            if (scene != null) {
+                doAcceleratorInstall(items, scene);
             }
-            sceneChangeListenerMap.remove(anchor);
+            // Scene change listener is added to the anchor for scenarios like,
+            // 1. Installing accelerators when Control is added to Scene
+            // 2. Removing accelerators when Control is removed from Scene
+            anchor.sceneProperty().addListener(getSceneChangeListener(anchor, items));
         }
-        // Add a new listener
-        anchor.sceneProperty().addListener(getSceneChangeListener(anchor, items));
     }
 
     private static void addAcceleratorsIntoScene(ObservableList<MenuItem> items, Object anchor) {

diff --git a/...avafx.controls/src/test/java/test/javafx/scene/control/ControlAcceleratorSupportTest.java b/...avafx.controls/src/test/java/test/javafx/scene/control/ControlAcceleratorSupportTest.java
@@ -25,18 +25,22 @@
 
 package test.javafx.scene.control;
 
+import javafx.beans.value.ChangeListener;
 import javafx.scene.control.MenuBar;
 import javafx.scene.control.Menu;
+import javafx.scene.control.MenuButton;
 import javafx.scene.control.MenuItem;
 import javafx.scene.layout.BorderPane;
 import javafx.scene.layout.StackPane;
 
+import test.com.sun.javafx.binding.ExpressionHelperUtility;
 import test.com.sun.javafx.scene.control.infrastructure.StageLoader;
 import test.util.memory.JMemoryBuddy;
 import static test.com.sun.javafx.scene.control.infrastructure.ControlTestUtils.*;
 
 import org.junit.Test;
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotEquals;
 
 public class ControlAcceleratorSupportTest {
 
@@ -114,4 +118,45 @@ public void testMemoryLeak_JDK_8274022() {
             checker.assertCollectable(menuItem);
         });
     }
+
+    @Test
+    public void testMemoryButtonSkinDoesntAddAdditionalListeners() {
+        // JDK-8296409
+        MenuItem menuItem = new MenuItem("Menu Item");
+        MenuButton menuButton = new MenuButton("Menu Button", null, menuItem);
+        StackPane root = new StackPane(menuButton);
+        StageLoader sl = new StageLoader(root);
+        assertEquals(1, getListenerCount(menuItem.acceleratorProperty()));
+        root.getChildren().remove(menuButton);
+        assertEquals(0, getListenerCount(menuItem.acceleratorProperty()));
+        root.getChildren().add(menuButton);
+        assertEquals(1, getListenerCount(menuItem.acceleratorProperty()));
+        sl.dispose();
+    }
+
+    @Test
+    public void testMemoryButtonSkinDoesntAddAdditionalListenersOnSceneChange() {
+        // JDK-8296409
+        MenuItem menuItem = new MenuItem("Menu Item");
+        MenuButton menuButton = new MenuButton("Menu Button", null, menuItem);
+        StackPane root = new StackPane(menuButton);
+        StackPane root2 = new StackPane();
+        StageLoader sl1 = new StageLoader(root);
+        StageLoader sl2 = new StageLoader(root2);
+        assertEquals(1, getListenerCount(menuItem.acceleratorProperty()));
+        ChangeListener originalChangeListener =
+                ExpressionHelperUtility.getChangeListeners(menuItem.acceleratorProperty()).get(0);
+        root2.getChildren().add(menuButton);
+        assertEquals(1, getListenerCount(menuItem.acceleratorProperty()));
+        ChangeListener secondChangeListener =
+                ExpressionHelperUtility.getChangeListeners(menuItem.acceleratorProperty()).get(0);
+        assertNotEquals(originalChangeListener, secondChangeListener);
+        root.getChildren().add(menuButton);
+        assertEquals(1, getListenerCount(menuItem.acceleratorProperty()));
+        ChangeListener thirdChangeListener =
+                ExpressionHelperUtility.getChangeListeners(menuItem.acceleratorProperty()).get(0);
+        assertNotEquals(secondChangeListener,thirdChangeListener);
+        sl1.dispose();
+        sl2.dispose();
+    }
 }