RFR: 8244110: NPE in MenuButtonSkinBase change listener

[Kevin Rushforth]

On Mon, 4 May 2020 09:07:53 GMT, Ajit Ghaisas <aghaisas at openjdk.org> wrote:

> Issue :
> https://bugs.openjdk.java.net/browse/JDK-8244110
> 
> Root Cause :
> Fix of [JDK-8175358](https://bugs.openjdk.java.net/browse/JDK-8175358) added code to remove accelerators from a scene
> in Scene property listener of MenuButtonSkinBase. That fix uses getSkinnable() in listener. It turned out that it can
> be null as found out by failing system test.  Fix :
> As pointed out by @kleopatra on the JBS, getSkinnable() should never be null for a valid skin. Hence, the correct fix
> is to remove listener cleanly in dispose method. See JBS comments for more info. This also addresses
> [JDK-8244081](https://bugs.openjdk.java.net/browse/JDK-8244081).  Testing :
> 1) Failing system test - passes after the fix
> 2) 3 unit tests which used to log NPE message silently and pass have been made to fail without this fix - these tests
> passes after the fix
> Special Thanks to @kleopatra for most of the test digging and fix guidance.

Inline comments below. I still need to test the fix, but will wait until you answer the questions I raised.

modules/javafx.controls/src/main/java/javafx/scene/control/skin/MenuButtonSkinBase.java line 230:

> 229:         // Remove listeners
> 230:         getSkinnable().sceneProperty().removeListener(sceneChangeListener);
> 231:         getSkinnable().getItems().removeListener(itemsChangedListener);

If scene is set to null (which can happen if this control or its parent is removed from the scene graph) before dispose
is called, this will throw an NPE. I think it should be moved inside the above if statement.

This raises the question: when the scene changes, are both the accelerators and this listener removed from the old
scene?

modules/javafx.controls/src/main/java/javafx/scene/control/skin/MenuButtonSkinBase.java line 77:

> 76:     private ListChangeListener<MenuItem> itemsChangedListener;
> 77:     private ChangeListener<? super Scene> sceneChangeListener;
> 78:

Can this be made final?

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

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