On Mon, 30 Mar 2020 10:36:13 GMT, Jeanette Winzenburg <faste...@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
