8198402: ToggleButton.setToggleGroup causes memory leak when button i…

…s removed via ToggleGroup.getToggles() Reviewed-by: kcr, arapte
openjdk · Apr 30, 2020 · 3130fc8 · 3130fc8
1 parent 8ad5805
commit 3130fc8
Show file tree

Hide file tree

Showing 2 changed files with 104 additions and 15 deletions.
diff --git a/modules/javafx.controls/src/main/java/javafx/scene/control/ToggleGroup.java b/modules/javafx.controls/src/main/java/javafx/scene/control/ToggleGroup.java
@@ -71,19 +71,27 @@ public final ObservableList<Toggle> getToggles() {
     private final ObservableList<Toggle> toggles = new VetoableListDecorator<Toggle>(new TrackableObservableList<Toggle>() {
         @Override protected void onChanged(Change<Toggle> c) {
             while (c.next()) {
-                // Look through the removed toggles, and if any of them was the
-                // one and only selected toggle, then we will clear the selected
-                // toggle property.
+                final List<Toggle> addedToggles = c.getAddedSubList();
+
+                // Look through the removed toggles.
                 for (Toggle t : c.getRemoved()) {
+                    // If any of them was the one and only selected toggle,
+                    // then we will clear the selected toggle property.
                     if (t.isSelected()) {
                         selectToggle(null);
                     }
+
+                    // If the toggle is not added again (below) remove
+                    // the group association.
+                    if (!addedToggles.contains(t)) {
+                        t.setToggleGroup(null);
+                    }
                 }
 
                 // A Toggle can only be in one group at any one time. If the
                 // group is changed, then the toggle is removed from the old group prior to
                 // being added to the new group.
-                for (Toggle t: c.getAddedSubList()) {
+                for (Toggle t: addedToggles) {
                     if (!ToggleGroup.this.equals(t.getToggleGroup())) {
                         if (t.getToggleGroup() != null) {
                             t.getToggleGroup().getToggles().remove(t);
@@ -95,7 +103,7 @@ public final ObservableList<Toggle> getToggles() {
                 // Look through all the added toggles and the very first selected
                 // toggle we encounter will become the one we make the selected
                 // toggle for this group.
-                for (Toggle t : c.getAddedSubList()) {
+                for (Toggle t : addedToggles) {
                     if (t.isSelected()) {
                         selectToggle(t);
                         break;

diff --git a/modules/javafx.controls/src/test/java/test/javafx/scene/control/ToggleButtonTest.java b/modules/javafx.controls/src/test/java/test/javafx/scene/control/ToggleButtonTest.java
@@ -30,6 +30,7 @@
 import test.com.sun.javafx.pgstub.StubToolkit;
 import com.sun.javafx.logging.PlatformLogger;
 import com.sun.javafx.tk.Toolkit;
+import java.lang.ref.WeakReference;
 import javafx.event.ActionEvent;
 import javafx.event.EventType;
 import javafx.geometry.Pos;
@@ -142,6 +143,80 @@ public class ToggleButtonTest {
         assertPseudoClassDoesNotExist(toggle, "selected");
     }
 
+    /*********************************************************************
+     * Toggle group Tests                                                *
+     ********************************************************************/
+    @Test public void setToggleGroupAndSeeValueIsReflectedInModel() {
+        toggle.setToggleGroup(toggleGroup);
+        assertSame(toggle.toggleGroupProperty().getValue(), toggleGroup);
+    }
+
+    @Test public void setToggleGroupAndSeeValue() {
+        toggle.setToggleGroup(toggleGroup);
+        assertSame(toggle.getToggleGroup(), toggleGroup);
+    }
+
+    @Test public void toggleGroupViaGroupAddAndRemoveClearsReference() {
+        WeakReference<ToggleButton> ref = new WeakReference<>(toggle);
+
+        toggleGroup.getToggles().add(toggle);
+        toggleGroup.getToggles().clear();
+
+        toggle = null;
+        attemptGC(ref, 5);
+
+        assertNull(ref.get());
+    }
+
+    @Test public void toggleGroupViaToggleSetClearsReference() {
+        WeakReference<ToggleButton> ref = new WeakReference<>(toggle);
+
+        toggle.setToggleGroup(toggleGroup);
+        toggle.setToggleGroup(null);
+
+        toggle = null;
+        attemptGC(ref, 5);
+
+        assertNull(ref.get());
+    }
+
+    @Test public void toggleGroupViaToggleThenGroupClearsReference() {
+        WeakReference<ToggleButton> ref = new WeakReference<>(toggle);
+
+        toggle.setToggleGroup(toggleGroup);
+        toggleGroup.getToggles().clear();
+
+        toggle = null;
+        attemptGC(ref, 5);
+
+        assertNull(ref.get());
+    }
+
+    @Test public void toggleGroupViaGroupThenToggleClearsReference() {
+        WeakReference<ToggleButton> ref = new WeakReference<>(toggle);
+
+        toggleGroup.getToggles().add(toggle);
+        toggle.setToggleGroup(null);
+
+        toggle = null;
+        attemptGC(ref, 5);
+
+        assertNull(ref.get());
+    }
+
+    @Test public void toggleGroupSwitchingClearsReference() {
+        WeakReference<ToggleButton> ref = new WeakReference<>(toggle);
+
+        ToggleGroup anotherToggleGroup = new ToggleGroup();
+        toggle.setToggleGroup(toggleGroup);
+        toggle.setToggleGroup(anotherToggleGroup);
+        toggle.setToggleGroup(null);
+
+        toggle = null;
+        attemptGC(ref, 5);
+
+        assertNull(ref.get());
+    }
 
     /*********************************************************************
      * Miscellaneous Tests                                         *
@@ -156,16 +231,6 @@ public class ToggleButtonTest {
         assertFalse(toggle.isSelected());
     }
 
-    @Test public void setToggleGroupAndSeeValueIsReflectedInModel() {
-        toggle.setToggleGroup(toggleGroup);
-        assertSame(toggle.toggleGroupProperty().getValue(), toggleGroup);
-    }
-
-    @Test public void setToggleGroupAndSeeValue() {
-        toggle.setToggleGroup(toggleGroup);
-        assertSame(toggle.getToggleGroup(), toggleGroup);
-    }
-
     @Test public void fireAndCheckSelectionToggled() {
         toggle.fire();
         assertTrue(toggle.isSelected());
@@ -190,4 +255,20 @@ public class ToggleButtonTest {
         assertTrue("fire() doesnt emit ActionEvent!", flag[0]);
     }
 
+    private void attemptGC(WeakReference<? extends Object> weakRef, int n) {
+        // Attempt gc n times
+        for (int i = 0; i < n; i++) {
+            System.gc();
+            System.runFinalization();
+
+            if (weakRef.get() == null) {
+                break;
+            }
+            try {
+                Thread.sleep(500);
+            } catch (InterruptedException e) {
+                fail("InterruptedException occurred during Thread.sleep()");
+            }
+        }
+    }
 }