On Fri, 30 Sep 2022 16:21:32 GMT, Jeanette Winzenburg <faste...@openjdk.org> wrote:
>> Will definitely do! Some tests were failing yesterday, until all is fixed - >> it's a draft PR :-) >> Thank you so much, @kleopatra > >> 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). ------------- PR: https://git.openjdk.org/jfx/pull/906
