RFR: 8319779: SystemMenu: memory leak due to listener never being removed [v6]

[Johan Vos]

On Tue, 9 Jan 2024 16:21:39 GMT, Andy Goryachev <angorya at openjdk.org> wrote:

>> Johan Vos has updated the pull request incrementally with two additional commits since the last revision:
>> 
>>  - Cleanup test
>>  - Add shim class so that we can access the references to com.sun.glass.ui.Menu instances.
>>    Add a test to make sure those references are gone.
>
> modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/GlassSystemMenu.java line 98:
> 
>> 96:             active.set(false);
>> 97:         }
>> 98:         active = new SimpleBooleanProperty(true);
> 
> should this be
> 
> if(active == null) {
>   active = new ...
> } else {
>  active.set(false);
> }
> 
> instead?

No, the explicit goal of this construction is to set active to false (in case it exists) so that existing listeners can be released; followed by creating a new active property that is by default set to true until `setMenus` is called again (e.g. after unfocus/focus events).
I noticed however that it is not as simple as that, and the current PR introduces regression, as the `SystemMenuBarTest.testMemoryLeak` test is now failing. Listeners are not only registered when `setMenus` is called, but also when menu(Item)s are added to menus. I'm not sure if a single `active` property can address this, as we don't want to remove all listeners for all menuitems in case a particular menuitem is added to a particular menu.
@hjohn does that make sense?

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

PR Review Comment: https://git.openjdk.org/jfx/pull/1283#discussion_r1447895647