On Mon, 30 Mar 2020 07:21:38 GMT, Ambarish Rapte <ara...@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
