8295500: AccordionSkin: memory leak when changing skin

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

Hide file tree

Showing 2 changed files with 41 additions and 15 deletions.
diff --git a/modules/javafx.controls/src/main/java/javafx/scene/control/skin/AccordionSkin.java b/modules/javafx.controls/src/main/java/javafx/scene/control/skin/AccordionSkin.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2010, 2021, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2010, 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
@@ -25,9 +25,11 @@
 
 package javafx.scene.control.skin;
 
-import com.sun.javafx.scene.control.behavior.BehaviorBase;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
 import javafx.beans.value.ChangeListener;
-import javafx.collections.ListChangeListener;
 import javafx.scene.Node;
 import javafx.scene.control.Accordion;
 import javafx.scene.control.Control;
@@ -36,11 +38,9 @@
 import javafx.scene.control.TitledPane;
 import javafx.scene.shape.Rectangle;
 
+import com.sun.javafx.scene.control.ListenerHelper;
 import com.sun.javafx.scene.control.behavior.AccordionBehavior;
-
-import java.util.HashMap;
-import java.util.List;
-import java.util.Map;
+import com.sun.javafx.scene.control.behavior.BehaviorBase;
 
 /**
  * Default skin implementation for the {@link Accordion} control.
@@ -95,9 +95,10 @@ public AccordionSkin(final Accordion control) {
 
         // install default input map for the accordion control
         behavior = new AccordionBehavior(control);
-//        control.setInputMap(behavior.getInputMap());
 
-        control.getPanes().addListener((ListChangeListener<TitledPane>) c -> {
+        ListenerHelper lh = ListenerHelper.get(this);
+
+        lh.addListChangeListener(control.getPanes(), (c) -> {
             if (firstTitledPane != null) {
                 firstTitledPane.getStyleClass().remove("first-titled-pane");
             }
@@ -131,28 +132,37 @@ public AccordionSkin(final Accordion control) {
         getChildren().setAll(control.getPanes());
         getSkinnable().requestLayout();
 
-        registerChangeListener(getSkinnable().widthProperty(), e -> clipRect.setWidth(getSkinnable().getWidth()));
-        registerChangeListener(getSkinnable().heightProperty(), e -> {
+        lh.addChangeListener(getSkinnable().widthProperty(), (ev) -> {
+            clipRect.setWidth(getSkinnable().getWidth());
+        });
+
+        lh.addChangeListener(getSkinnable().heightProperty(), (ev) -> {
             clipRect.setHeight(getSkinnable().getHeight());
             relayout = true;
         });
     }
 
 
-
     /* *************************************************************************
      *                                                                         *
      * Public API                                                              *
      *                                                                         *
      **************************************************************************/
 
     /** {@inheritDoc} */
-    @Override public void dispose() {
-        super.dispose();
+    @Override
+    public void dispose() {
+        if (getSkinnable() == null) {
+            return;
+        }
+
+        removeTitledPaneListeners(getSkinnable().getPanes());
 
         if (behavior != null) {
             behavior.dispose();
         }
+
+        super.dispose();
     }
 
     /** {@inheritDoc} */

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
@@ -161,15 +161,31 @@ public static Collection<Object[]> data() {
         // 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(
-                Accordion.class,
+                //
                 ColorPicker.class,
+
+                //
                 ComboBox.class,
+
+                //
                 DatePicker.class,
+
+                //
                 MenuBar.class,
+
+                //
                 PasswordField.class,
+
+                //
                 Spinner.class,
+
+                //
                 SplitPane.class,
+
+                //
                 TableView.class,
+
+                //
                 TreeTableView.class
         );
         // remove the known issues to make the test pass