RFR: 8294589: MenuBarSkin: memory leak when changing skin

[Jeanette Winzenburg]

On Tue, 4 Oct 2022 14:06:59 GMT, Jeanette Winzenburg <fastegal 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 :)
>
>> 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();
    }

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

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