[Rev 01] RFR: 8244110: NPE in MenuButtonSkinBase change listener

[Kevin Rushforth]

On Tue, 5 May 2020 10:35:12 GMT, Ajit Ghaisas <aghaisas at openjdk.org> wrote:

>> 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?
>
> If MenuButton is removed from a scene :
> Valid scene -> null scene : sceneChangeListener is invoked to remove accelerators registered with old valid scene.
> If the skin is disposed after this - we have a guard to prevent removal of accelerators from a null Scene. Also,
> removing of the listener using sceneProperty() does not result in NPE.
> To your specific question :
> When the Scene changes, only the accelerators need to be removed from the old Scene without removing
> sceneChangeListener from skin. This is due to the fact that the Control (removed from old Scene) can be added to
> another Scene without recreating the skin.  Please let me know if you have further questions.

Yes, you are of course right about `sceneProperty()` not being null just because scene might be. An my other question
was based on a too-quick read of the code: since the listener is on the scene property of the control, and is meant to
track the case where the control moves from one scene to another, it would not be the right thing to do to remove it
when changing scenes.

So the fix looks fine to me.

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

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