8276056: Control.skin.setSkin(Skin) fails to call dispose() on discarded Skin

Co-authored-by: Jeanette Winzenburg <fastegal@openjdk.org>
Reviewed-by: aghaisas

Author: 2 people

modules/javafx.controls/src/main/java/javafx/scene/control/Control.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
@@ -237,19 +237,6 @@ private static Class<?> loadClass(final String className, final Object instance)
         // a reference to the old value.
         private Skin<?> oldValue;
 
-        @Override
-        //This code is basically a kind of optimization that prevents a Skin that is equal but not instance equal.
-        //Although it's not kosher from the property perspective (bindings won't pass through set), it should not do any harm.
-        //But it should be evaluated in the future.
-        public void set(Skin<?> v) {
-            if (v == null
-                ? oldValue == null
-                : oldValue != null && v.getClass().equals(oldValue.getClass()))
-                return;
-
-            super.set(v);
-        }
-
         @Override protected void invalidated() {
             Skin<?> skin = get();
             // Collect the name of the currently installed skin class. We do this

modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/SkinMemoryLeakTest.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2020, 2021, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2020, 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
@@ -77,6 +77,40 @@ public class SkinMemoryLeakTest {
 
 //--------- tests
 
+    /**
+     * default skin -> set another instance of default skin
+     */
+    @Test
+    public void testMemoryLeakSameSkinClass() {
+        installDefaultSkin(control);
+        Skin<?> skin = control.getSkin();
+        installDefaultSkin(control);
+
+        WeakReference<?> weakRef = new WeakReference<>(skin);
+        skin = null;
+        attemptGC(weakRef);
+        assertNull("Unused Skin must be gc'ed", weakRef.get());
+    }
+
+    @Test
+    public void testControlChildrenSameSkinClass() {
+        installDefaultSkin(control);
+        int childCount = control.getChildrenUnmodifiable().size();
+        installDefaultSkin(control);
+        assertEquals("Old skin should dispose children when a new skin is set",
+                childCount, control.getChildrenUnmodifiable().size());
+    }
+
+    @Test
+    public void testSetSkinOfSameClass() {
+        installDefaultSkin(control);
+        Skin<?> oldSkin = control.getSkin();
+        installDefaultSkin(control);
+        Skin<?> newSkin = control.getSkin();
+
+        assertNotEquals("New skin was not set", oldSkin, newSkin);
+    }
+
     /**
      * default skin -> set alternative
      */