RFR: 8294589: MenuBarSkin: memory leak when changing skin

[Michael Strauß]

On Mon, 3 Oct 2022 23:07:08 GMT, Andy Goryachev <angorya at openjdk.org> wrote:

>>> You are right, @kleopatra - this executeOnceWhenPropertyIsNonNull() is installing a listener. perhaps we should add a similar functionality to LambdaMultiplePropertyChangeListenerHandler and actually install a WeakListener instead.
>> 
>> I'd advise against doing this in an ad-hoc manner. Skins leak memory by adding (but not removing) all kinds of listeners or children to their controls. Some skins try to address this with weak listeners, or by manually removing listeners in the dispose() method. SkinBase attempts to make it easier for skins to not leak listeners with the `registerInvalidationListener` and `registerChangeListener` APIs, which remove an added listener when the skin is disposed (however, both of these methods only accept Consumers, not actual `InvalidationListener`/`ChangeListener` instances, making the API less useful than it could be).
>> 
>> But there are more places where memory is leaked:
>> 1. adding, but not removing children to the control
>> 2. adding EventHandlers/EventFilters to the control or to other objects (for example, MenuBarSkin adds EventHandlers to the control's scene)
>> 3. hidden listeners like the one you discovered: Utils.executeOnceWhenPropertyIsNonNull
>> 4. Label.setLabeLFor(control)
>> 5. ListChangeListener, etc.
>> 
>> Most skins exhibit one or several of these memory leaks, which causes them to remain strongly referenced by the control even after being disposed. The problem here is that JavaFX doesn't offer good tools and APIs to address these challenges, so it's incredible easy to create a skin that leaks memory. In fact, it's so easy that most built-in skins do it (and these were created by the developers of JavaFX itself, so obviously it's a major problem if even those people get it wrong so often).
>> 
>> For me, this is a clear indication that we're dealing with an API problem first and foremost. I think there should be a better API that makes it easy for skins to interact with their associated controls, instead of hand-wiring hundreds of little skin-control dependencies (and sure enough, getting it wrong every so often).
>
> You are absolutely right!  There are so many places where memory leaks are being created.  My plan is to go through all the skins and take care of them all.
> 
> LambdaMultiplePropertyChangeListenerHandler is, I believe, a step in the right direction.  I wish there were a more uniform and public way to add various listeners, implemented as a public class - something that can be used to yank all listeners with a single method.
> 
> I've done something like that on the application level, see https://github.com/andy-goryachev/FxTextEditor/blob/master/src/goryachev/fx/FxDisconnector.java
> 
> What we could do (later) is to keep enhancing LambdaMultiplePropertyChangeListenerHandler by adding more ways of adding listeners, including full-blown change/invalidation listeners, and then rename it, move to a public util package and make it a part of the general purpose API.

Sure, but I think this should be done with a clear vision of where we're going, and not just as a collection of ad-hoc helper methods. There are even more problems that a new API should at least consider:
Some skins change their behavior depending on the configuration of their associated control at the time when the skin is installed. For example, `MenuButtonSkinBase` adds a `MOUSE_PRESSED` handler only if the control didn't specify one for its onMousePressed property:

    if (control.getOnMousePressed() == null) {
        control.addEventHandler(MouseEvent.MOUSE_PRESSED, ...

But what happens if the `onMousePressed` property is set after the skin is installed?
Rather than a one-time check, cases like these should probably be a dynamic expression:

    when control.onMousePressedProperty().isNotNull() and skin.isInstalled()
        control.addEventHandler
    otherwise
        control.removeEventHandler

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

PR: https://git.openjdk.org/jfx/pull/906