RFR: 8241737: TabPaneSkin memory leak on replacing selectionModel

[Ambarish Rapte]

On Mon, 13 Apr 2020 15:47:00 GMT, Jeanette Winzenburg <fastegal at openjdk.org> wrote:

>> `TabPaneSkin` adds a listener to `SelectionModel.selectedItemProperty()` which holds the `SelectionModel` from being
>> GCed. Fix is to add and remove the listener when a `SelectionModel` is changed.
>> If the fix looks good, We can change all the `getSkinnable().getSelectionModel()` calls to use the new class member
>> `selectionModel`.
>> The fix seems safe to cause any regression. Enabled an ignored test that was added as part of fix for
>> [JDK-8241455](https://bugs.openjdk.java.net/browse/JDK-8241455).
>
> modules/javafx.controls/src/main/java/javafx/scene/control/skin/TabPaneSkin.java line 270:
> 
>> 269:         }
>> 270:
>> 271:         super.dispose();
> 
> wondering if the removal is really needed? The memory leak seems to be fixed without explicit removing. Did you
> experience any side-effects if not?
> If so, a unit test covering that would be cool. There are other (mostly) skins that have a weak "path listener"  which
> is not removed on dispose, they need a re-visit to see if the side-effect applies to them as well. Or the other way
> round: without a detectable side-effect, not doing it might be okay.   Whatever the outcome, this looks like a pattern
> to remember when skins are listening weakly :)

This is needed to future safe the code from an NPE from scenarios as in below test case,

    class TabPaneSkin1 extends TabPaneSkin {
        TabPaneSkin1(TabPane tabPane) {
            super(tabPane);
        }
    }

    @Test
    public void testNPEOnSwitchSkinAndRemoveTabPane() {
        TabPane testPane = new TabPane(new Tab("tab0"), new Tab("tab1"));
        Group root = new Group(testPane);
        Scene scene = new Scene(root);
        Stage stage = new Stage();
        stage.setScene(scene);
        stage.show();
        tk.firePulse();

        testPane.setSkin(new TabPaneSkin1(testPane));
        tk.firePulse();
        testPane.getSelectionModel().select(1);
        tk.firePulse();
    }

But currently this throws an NPE at different code,
java.lang.NullPointerException
        at javafx.controls/javafx.scene.control.skin.TabPaneSkin.isHorizontal(TabPaneSkin.java:699)
        at javafx.controls/javafx.scene.control.skin.TabPaneSkin$TabHeaderArea.layoutChildren(TabPaneSkin.java:1134)
        at javafx.graphics/javafx.scene.Parent.layout(Parent.java:1207)

This is part of an another problem that skins should remove all listeners when switched.
So I think we can not add this test with this fix.

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

PR: https://git.openjdk.java.net/jfx/pull/175