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

[Ambarish Rapte]

On Sat, 28 Mar 2020 10:29:39 GMT, Jeanette Winzenburg <fastegal at openjdk.org> wrote:

>> Ambarish Rapte has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Changes for Kevin's review comment
>
> modules/javafx.controls/src/main/java/javafx/scene/control/skin/ButtonSkin.java line 101:
> 
>> 100:     };
>> 101:     WeakChangeListener<Scene> weakChangeListener = new WeakChangeListener<>(sceneChangeListener);
>> 102:
> 
> a loose naming convention for weak listeners seems to be to prepend _weak_ to the name of the strong listener, which
> would be
>     
>     WeakChangeListener<Scene> weakSceneChangeListener = new WeakChangeListener<>(sceneChangeListener);

Corrected in the next commit...

> 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

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

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