RFR: 8294589: MenuBarSkin: memory leak when changing skin

[Andy Goryachev]

On Mon, 3 Oct 2022 22:54:38 GMT, Michael Strauß <mstrauss at openjdk.org> wrote:

>>> Perhaps the test is too artificial, something is not being done correctly or exactly as in the real application? Using StageLoader or showControl() hooks up the missing dependencies.
>> 
>> one last time: there is _no_ such thing as a "too artificial" test - a class must _always_ fulfil its contract in whatever valid context. It's not enough to do so for some (or even the majority) of use-cases. Plus: logically, any assumption (like: there are no memory leaks) is invalidated by a single counter-example (like the valid test).
>> 
>> Have a nice weekend, I'm off now :)
>
>> 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.

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

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