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

[Johan Vos]

On Tue, 19 Dec 2023 09:59:32 GMT, John Hendrikx <jhendrikx at openjdk.org> wrote:

>> Johan Vos has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Fix more memoryleaks due to listeners never being unregistered.
>
> Would it be an idea to do deterministic clean-up?
> 
> The JIRA tickets involved are light on details, and I don't have a Mac, so I can't test the leak (I think?), but it seems to me that you will want to remove the listeners when the menu bar isn't visible (and re-add them if it becomes visible again?).
> 
> From what I understand, these are normal JavaFX controls wrapped into adapters, and so any listeners installed through the adapter interface will end up on controls.  The `GlobalMenuAdapter` seems to be tracking the `showing` property, and calls `show` / `hide` accordingly.
> 
> The ticket says:
> 
>> when an application loses focus, the SystemMenu is removed. When it gets focus again, the SystemMenu is "back".
> 
> My question is, is there any event that can be used to trigger clean-up when this happens?  Do the controls go invisible, are they no longer showing (is `hide` called?)  There must be something.  If there is something, then some kind of disposal can be triggered for deterministic clean-up (ie. add `dispose` method).
> 
> If you can go further and distill this information into a boolean property then a pattern like this can be used:
> 
>         mb.textProperty()
>              .when(showingProperty)  // assumes showing property becomes false when focus is lost...
>              .addListener(valueModel -> glassMenu.setTitle(parseText(mb)));
> 
> The `showingProperty` can be fed from the hide/show event handlers to set it to the right value, perhaps combined with focus information.

@hjohn You're absolutely right. It's another reason why I moved this PR to [WIP]. Instead of guessing and trying, I want to understand the deterministic path that leads to the issue with listeners being added and not removed. The one fixed in the first commit is clear, and well understood. Still, I saw uncollected listeners, with the GC root trace pointing to those other listeners (textProperty, mnemonicParsingProperty etc). Rather than trying to "fix" this with weak listeners, it is much better to find the real cause for this.
In the process of finding out, I'm adding a few shim classes and unit tests so that we can clearly test for regression.

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

PR Comment: https://git.openjdk.org/jfx/pull/1283#issuecomment-1862465132