On Tue, 23 Mar 2021 14:30:56 GMT, Ambarish Rapte <ara...@openjdk.org> wrote:
>> The method `ControlAcceleratorSupport.doAcceleratorInstall(final List<?
>> extends MenuItem> items, final Scene scene)` adds a `ChangeListener` on
>> `MenuItem.acceleratorProperty()`. This listener is not removed when the
>> MenuItem is removed from scenegraph.
>> Adding and removing a MenuItem results in multiple copies of the listener
>> added to MenuItem.acceleratorProperty().
>>
>> Fix is to remove the listener when MenuItem is removed.
>> Fix can be verified by checking the number of instances of
>> `ControlAcceleratorSupport.lambda` using _jvisualvm_.
>> Without this fix, the number of `ControlAcceleratorSupport.lambda` increase
>> in multiple of number of MenuItems being removed and added back.
>> With fix, the count is always same as number of MenuItems in scenegraph.
>>
>> Also there is another ListChangeListener added to a
>> `ObservableList<MenuItem> items` in the method
>> `ControlAcceleratorSupport.doAcceleratorInstall(final
>> ObservableList<MenuItem> items, final Scene scene)`. There was a TODO note
>> to remove this listener.
>> This listener is added on `MenuBarButton.getItems()` and not on
>> `Menu.getItems()`. This `MenuBarButton` is created by `MenuBarSkin` to show
>> a `Menu`. This `MenuBarButton` gets disposed when the related `Menu` is
>> removed from scenegraph, and so the added `ListChangeListener` gets GCed.
>> Hence it is not required to explicitly remove the listener.
>> Added a comment explaining this behavior in place of the TODO.
>
> Ambarish Rapte has updated the pull request incrementally with one additional
> commit since the last revision:
>
> correct test
The fix looks good.
The test looks OK as far as it goes, although I note that it is testing for the
leak in an indirect way, since it looks at the `changeListenerMap` rather than
checking the listeners of `menuitem.acceleratorProperty()` which is what we
really care about. Is there a way to test it more directly? If not then I think
this is fine.
modules/javafx.controls/src/test/java/test/javafx/scene/control/ControlAcceleratorSupportTest.java
line 46:
> 44: public static void setup() throws Exception {
> 45: for (int i = 0; i < 4; i++) {
> 46: System.gc();
Maybe call `System.runFinalization()`, too?
-------------
Marked as reviewed by kcr (Lead).
PR: https://git.openjdk.java.net/jfx/pull/429
