8295506: ButtonBarSkin: memory leak when changing skin

Reviewed-by: aghaisas
openjdk · Dec 2, 2022 · 6ab65a9 · 6ab65a9 · openjdk-notifier · Dec 2, 2022
1 parent 4a19fe7
commit 6ab65a9
Show file tree

Hide file tree

Showing 2 changed files with 21 additions and 13 deletions.
diff --git a/modules/javafx.controls/src/main/java/javafx/scene/control/skin/ButtonBarSkin.java b/modules/javafx.controls/src/main/java/javafx/scene/control/skin/ButtonBarSkin.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2014, 2021, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2014, 2022, 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
@@ -29,10 +29,8 @@
 import java.util.List;
 import java.util.Map;
 
-import com.sun.javafx.scene.control.Properties;
 import javafx.beans.InvalidationListener;
 import javafx.beans.property.ObjectProperty;
-import javafx.collections.ListChangeListener;
 import javafx.geometry.Pos;
 import javafx.scene.Node;
 import javafx.scene.control.Button;
@@ -45,6 +43,9 @@
 import javafx.scene.layout.Priority;
 import javafx.scene.layout.Region;
 
+import com.sun.javafx.scene.control.ListenerHelper;
+import com.sun.javafx.scene.control.Properties;
+
 /**
  * Default skin implementation for the {@link ButtonBar} control.
  *
@@ -106,17 +107,19 @@ public ButtonBarSkin(final ButtonBar control) {
 
         layoutButtons();
 
+        ListenerHelper lh = ListenerHelper.get(this);
+
         updateButtonListeners(control.getButtons(), true);
-        control.getButtons().addListener((ListChangeListener<Node>) c -> {
+        lh.addListChangeListener(control.getButtons(), (c) -> {
             while (c.next()) {
                 updateButtonListeners(c.getRemoved(), false);
                 updateButtonListeners(c.getAddedSubList(), true);
             }
             layoutButtons();
         });
 
-        registerChangeListener(control.buttonOrderProperty(), e -> layoutButtons());
-        registerChangeListener(control.buttonMinWidthProperty(), e -> resizeButtons());
+        lh.addChangeListener(control.buttonOrderProperty(), (ev) -> layoutButtons());
+        lh.addChangeListener(control.buttonMinWidthProperty(), (ev) -> resizeButtons());
     }
 
 
@@ -127,6 +130,18 @@ public ButtonBarSkin(final ButtonBar control) {
      *
      **************************************************************************/
 
+    @Override
+    public void dispose() {
+        if (getSkinnable() == null) {
+            return;
+        }
+
+        updateButtonListeners(getSkinnable().getButtons(), false);
+        getChildren().remove(layout);
+
+        super.dispose();
+    }
+
     private void updateButtonListeners(List<? extends Node> list, boolean buttonsAdded) {
         if (list != null) {
             for (Node n : list) {

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
@@ -162,19 +162,12 @@ public static Collection<Object[]> data() {
         // step 2: fix and remove from list
         List<Class<? extends Control>> leakingClasses = List.of(
                 Accordion.class,
-                ButtonBar.class,
                 ColorPicker.class,
                 ComboBox.class,
                 DatePicker.class,
                 MenuBar.class,
-                //MenuButton.class,
-                //Pagination.class,
                 PasswordField.class,
-                //ScrollBar.class,
-                //ScrollPane.class,
-                // @Ignore("8245145")
                 Spinner.class,
-                //SplitMenuButton.class,
                 SplitPane.class,
                 TableView.class,
                 TreeTableView.class