RFR: 8294589: MenuBarSkin: memory leak when changing skin

[Jeanette Winzenburg]

On Tue, 4 Oct 2022 14:20:46 GMT, Jeanette Winzenburg <fastegal at openjdk.org> wrote:

>>> Thanks again, @kleopatra With your permission, I'll add tests with and without scene property set. Or do we want to keep the original set?
>> 
>> SkinMemoryLeakTest already has both methods, that is testing the replacement of a skin with/out residing in a scene .. no need for adding anything ;)
>
>> 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).
>> 
> 
> well, just guessing games here but: mine is that they didn't put too many thoughts into the possibility of replacing a skin and if they did, they actively prevented it - as seen in the implementation of TextAreaSkin.dispose when we started the current cleanup round:
> 
>     /** {@inheritDoc} */
>     @Override public void dispose() {
>         super.dispose();
> 
>         if (behavior != null) {
>             behavior.dispose();
>         }
> 
>         // TODO Unregister listeners on text editor, paragraph list
>         throw new UnsupportedOperationException();
>     }

> 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.

- yes, we need weakListeners (to wait for a not-null value to add the event handler) _and_ remove both the weakListener and the event handlers in dispose
- adding functionality to the LambdaHandler sounds like a good idea, but needs some thinking and should be done in a separate issue which also adds delegate methods to expose the new functionality in SkinBase for use in sub classes

For this issue, you might simply inline the utility method and register the listeners using skinbase api (didn't try, it's your issue :), see TableRowSkin for a similar pattern 

This conditional adding of handlers/listeners is needed if we have to support path properties (also f.i. when listening to selectedXX in any of the skins with selectionModels as properties). Which basically should be supported by base - but probably without any chance to ever get them ;)

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

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