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

[Jeanette Winzenburg]

On Mon, 30 Mar 2020 07:21:38 GMT, Ambarish Rapte <arapte at openjdk.org> wrote:

>> modules/javafx.controls/src/main/java/javafx/scene/control/skin/ButtonSkin.java line 178:
>> 
>>> 177:     @Override public void dispose() {
>>> 178:         getSkinnable().sceneProperty().removeListener(weakChangeListener);
>>> 179:         super.dispose();
>> 
>> +1! Actually, looks like that manual remove is really needed - otherwise we get an NPE when removing the default button
>> from its parent after the skin has been switched:
>>     @Test
>>     public void testDefaultButtonSwitchSkinAndRemove() {
>>         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();
>>         
>>         button.setSkin(new ButtonSkin1(button));
>>         root.getChildren().remove(button);
>>     }
>> 
>> Note: to see this NPE as failing test (vs. its printout on sysout), we need to re-wire the uncaughtExceptionHandler,
>> see ComboBoxTest setup/cleanup for an example.
>
> 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);

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

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