[Rev 01] RFR: 8236840: Memory leak when switching ButtonSkin

[Ambarish Rapte]

On Mon, 30 Mar 2020 10:36:13 GMT, Jeanette Winzenburg <fastegal at openjdk.org> wrote:

>> Thanks for the test case, I did minor changes to it and included in the next commit.
>> The NPE can occur even without `button.setDefaultButton(true);`.
>> Please take a look
>
> good catch! You are right, without explicit removal of the listener, the NPE happens always.
> 
> Actually, I had been sloppy and got distracted by the NPE from my actual goal which was to dig into Kevin's "not
> cleaning up after itself" and .. finally found a (concededly extreme corner-case :) where that's happening: when
> setting the skin to null. Two failing tests:
>     @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());
>         button.setSkin(null);
>         
>         attemptGC(defSkinRef);
>         assertNull("skin must be gc'ed", defSkinRef.get());
>     }
>     
>     @Test
>     public void testDefaultButtonNullSkinAcceleratorRemoved() {
>         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();
>         KeyCodeCombination key = new KeyCodeCombination(KeyCode.ENTER);
> 
>         assertNotNull(scene.getAccelerators().get(key));
>         button.setSkin(null);
>         assertNull(scene.getAccelerators().get(key));
>         
>     }
> 
> An explicitly cleanup in dispose makes them pass:
> 
>     @Override
>     public void dispose() {
>         setDefaultButton(false);
>         setCancelButton(false);
>         getSkinnable().sceneProperty().removeListener(weakChangeListener);

I have included this change and the test, with slight modification to include same test for Cancel button.

-------------

PR: https://git.openjdk.java.net/jfx/pull/147