8236840: Memory leak when switching ButtonSkin

Reviewed-by: fastegal, kcr
openjdk · Apr 7, 2020 · 418675a · 418675a
1 parent 247a65d
commit 418675a
Show file tree

Hide file tree

Showing 2 changed files with 189 additions and 20 deletions.
diff --git a/modules/javafx.controls/src/main/java/javafx/scene/control/skin/ButtonSkin.java b/modules/javafx.controls/src/main/java/javafx/scene/control/skin/ButtonSkin.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2010, 2019, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2010, 2020, 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
@@ -28,6 +28,8 @@
 import com.sun.javafx.scene.NodeHelper;
 import com.sun.javafx.scene.control.behavior.BehaviorBase;
 import com.sun.javafx.scene.control.skin.Utils;
+import javafx.beans.value.ChangeListener;
+import javafx.beans.value.WeakChangeListener;
 import javafx.scene.Scene;
 import javafx.scene.control.Button;
 import javafx.scene.control.ContextMenu;
@@ -78,6 +80,25 @@ public class ButtonSkin extends LabeledSkinBase<Button> {
         }
     };
 
+    ChangeListener<Scene> sceneChangeListener = (ov, oldScene, newScene) -> {
+        if (oldScene != null) {
+            if (getSkinnable().isDefaultButton()) {
+                setDefaultButton(oldScene, false);
+            }
+            if (getSkinnable().isCancelButton()) {
+                setCancelButton(oldScene, false);
+            }
+        }
+        if (newScene != null) {
+            if (getSkinnable().isDefaultButton()) {
+                setDefaultButton(newScene, true);
+            }
+            if (getSkinnable().isCancelButton()) {
+                setCancelButton(newScene, true);
+            }
+        }
+    };
+    WeakChangeListener<Scene> weakSceneChangeListener = new WeakChangeListener<>(sceneChangeListener);
 
 
     /***************************************************************************
@@ -124,24 +145,7 @@ public ButtonSkin(Button control) {
                 }
             }
         });
-        control.sceneProperty().addListener((ov, oldScene, newScene) -> {
-            if (oldScene != null) {
-                if (getSkinnable().isDefaultButton()) {
-                    setDefaultButton(oldScene, false);
-                }
-                if (getSkinnable().isCancelButton()) {
-                    setCancelButton(oldScene, false);
-                }
-            }
-            if (newScene != null) {
-                if (getSkinnable().isDefaultButton()) {
-                    setDefaultButton(newScene, true);
-                }
-                if (getSkinnable().isCancelButton()) {
-                    setCancelButton(newScene, true);
-                }
-            }
-        });
+        control.sceneProperty().addListener(weakSceneChangeListener);
 
         // set visuals
         if (getSkinnable().isDefaultButton()) {
@@ -171,6 +175,13 @@ public ButtonSkin(Button control) {
 
     /** {@inheritDoc} */
     @Override public void dispose() {
+        if (getSkinnable().isDefaultButton()) {
+            setDefaultButton(false);
+        }
+        if (getSkinnable().isCancelButton()) {
+            setCancelButton(false);
+        }
+        getSkinnable().sceneProperty().removeListener(weakSceneChangeListener);
         super.dispose();
 
         if (behavior != null) {

diff --git a/modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/ButtonSkinTest.java b/modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/ButtonSkinTest.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2011, 2016, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2011, 2020, 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
@@ -26,6 +26,9 @@
 package test.javafx.scene.control.skin;
 
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.fail;
 
 import javafx.beans.value.ObservableValue;
 import javafx.geometry.Insets;
@@ -34,6 +37,8 @@
 import javafx.scene.Group;
 import javafx.scene.Scene;
 import javafx.stage.Stage;
+import javafx.scene.input.KeyCode;
+import javafx.scene.input.KeyCodeCombination;
 import javafx.scene.input.Mnemonic;
 import javafx.collections.ObservableList;
 import javafx.scene.input.KeyCombination;
@@ -44,8 +49,11 @@
 import javafx.scene.shape.Rectangle;
 
 import org.junit.Before;
+import org.junit.After;
 import org.junit.Test;
 
+import java.lang.ref.WeakReference;
+
 /**
  */
 public class ButtonSkinTest {
@@ -59,7 +67,17 @@ public class ButtonSkinTest {
         // computed but wasn't expected will be caught.
         button.setPadding(new Insets(10, 10, 10, 10));
         button.setSkin(skin);
+        Thread.currentThread().setUncaughtExceptionHandler((thread, throwable) -> {
+            if (throwable instanceof RuntimeException) {
+                throw (RuntimeException)throwable;
+            } else {
+                Thread.currentThread().getThreadGroup().uncaughtException(thread, throwable);
+            }
+        });
+    }
 
+    @After public void cleanup() {
+        Thread.currentThread().setUncaughtExceptionHandler(null);
     }
 
     @Test public void maxWidthTracksPreferred() {
@@ -152,6 +170,146 @@ public void testMnemonicDoesntDuplicateOnGraphicsChange() {
         }
     }
 
+    class ButtonSkin1 extends ButtonSkin {
+        ButtonSkin1(Button btn) {
+            super(btn);
+        }
+    }
+
+    class ButtonSkin2 extends ButtonSkin {
+        ButtonSkin2(Button btn) {
+            super(btn);
+        }
+    }
+
+    @Test
+    public void testOldSkinShouldGC() {
+        Button button = new Button();
+        Group root = new Group(button);
+        Scene scene = new Scene(root);
+        Stage stage = new Stage();
+        stage.setScene(scene);
+        stage.show();
+
+        WeakReference<ButtonSkin> defSkinRef = new WeakReference<>((ButtonSkin)button.getSkin());
+        ButtonSkin skin = new ButtonSkin1(button);
+        WeakReference<ButtonSkin> oldSkinRef = new WeakReference<>(skin);
+        button.setSkin(skin);
+        skin = new ButtonSkin2(button);
+        WeakReference<ButtonSkin> currSkinRef = new WeakReference<>(skin);
+        button.setSkin(skin);
+        skin = null;
+
+        attemptGC(oldSkinRef);
+        assertNull("Old ButtonSkin must be GCed.", oldSkinRef.get());
+        assertNull("Default ButtonSkin must be GCed.", defSkinRef.get());
+        assertNotNull("Current ButtonSkin must NOT be GCed.", currSkinRef.get());
+    }
+
+    @Test
+    public void testUnusedSkinShouldGC() {
+        Button button = new Button();
+        Group root = new Group(button);
+        Scene scene = new Scene(root);
+        Stage stage = new Stage();
+        stage.setScene(scene);
+        stage.show();
+
+        WeakReference<ButtonSkin> defSkinRef = new WeakReference<>((ButtonSkin)button.getSkin());
+        ButtonSkin skin = new ButtonSkin1(button);
+        WeakReference<ButtonSkin> skinRef1 = new WeakReference<>(skin);
+        skin = new ButtonSkin2(button);
+        WeakReference<ButtonSkin> skinRef2 = new WeakReference<>(skin);
+        skin = null;
+
+        attemptGC(skinRef1);
+        assertNull("Unused ButtonSkin must be GCed.", skinRef1.get());
+        assertNull("Unused ButtonSkin must be GCed.", skinRef2.get());
+        assertNotNull("Default ButtonSkin must NOT be GCed.", defSkinRef.get());
+    }
+
+    @Test
+    public void testButtonAndSkinShouldGC() {
+        Button button = new Button();
+        ButtonSkin skin = new ButtonSkin1(button);
+        WeakReference<Button> buttonRef = new WeakReference<>(button);
+        WeakReference<ButtonSkin> skinRef = new WeakReference<>(skin);
+        button.setSkin(skin);
+        button = null;
+        skin = null;
+
+        attemptGC(skinRef);
+        assertNull("Button must be GCed.", buttonRef.get());
+        assertNull("ButtonSkin must be GCed.", skinRef.get());
+    }
+
+    @Test
+    public void testNPEOnSwitchSkinAndRemoveButton() {
+        Button button = new Button();
+        Group root = new Group(button);
+        Scene scene = new Scene(root);
+        Stage stage = new Stage();
+        stage.setScene(scene);
+        stage.show();
+
+        button.setSkin(new ButtonSkin1(button));
+        root.getChildren().remove(button);
+    }
+
+    @Test
+    public void testDefaultButtonNullSkinReleased() {
+        Button button = new Button();
+        button.setDefaultButton(true);
+        Group root = new Group(button);
+        Scene scene = new Scene(root);
+        Stage stage = new Stage();
+        stage.setScene(scene);
+        stage.show();
+        WeakReference<ButtonSkin> defSkinRef = new WeakReference<>((ButtonSkin)button.getSkin());
+        KeyCodeCombination key = new KeyCodeCombination(KeyCode.ENTER);
+        assertNotNull(scene.getAccelerators().get(key));
+        button.setSkin(null);
+        assertNull(scene.getAccelerators().get(key));
+
+        attemptGC(defSkinRef);
+        assertNull("ButtonSkin must be GCed", defSkinRef.get());
+    }
+
+    @Test
+    public void testCancelButtonNullSkinReleased() {
+        Button button = new Button();
+        button.setCancelButton(true);
+        Group root = new Group(button);
+        Scene scene = new Scene(root);
+        Stage stage = new Stage();
+        stage.setScene(scene);
+        stage.show();
+        WeakReference<ButtonSkin> defSkinRef = new WeakReference<>((ButtonSkin)button.getSkin());
+        KeyCodeCombination key = new KeyCodeCombination(KeyCode.ESCAPE);
+        assertNotNull(scene.getAccelerators().get(key));
+        button.setSkin(null);
+        assertNull(scene.getAccelerators().get(key));
+
+        attemptGC(defSkinRef);
+        assertNull("ButtonSkin must be GCed", defSkinRef.get());
+    }
+
+    private void attemptGC(WeakReference<ButtonSkin> weakRef) {
+        for (int i = 0; i < 10; i++) {
+            System.gc();
+            System.runFinalization();
+
+            if (weakRef.get() == null) {
+                break;
+            }
+            try {
+                Thread.sleep(50);
+            } catch (InterruptedException e) {
+                fail("InterruptedException occurred during Thread.sleep()");
+            }
+        }
+    }
+
     public static final class ButtonSkinMock extends ButtonSkin {
         boolean propertyChanged = false;
         int propertyChangeCount = 0;