RFR: 8241737: TabPaneSkin memory leak on replacing selectionModel

[Jeanette Winzenburg]

On Tue, 14 Apr 2020 13:01:18 GMT, Jeanette Winzenburg <fastegal at openjdk.org> wrote:

>> 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.
>
> thanks for the test :)
> 
> Good catch, and I can reproduce it, though with a twist:
> 
> - with firing the pulse I see the error you cited
> - without firing the pulse, I see the error in the selected item listener (because the skinnable is null) which to me
>   appears to be "nearer" the real reason
> 
> Whatever the error, though, we learned that we must be extremely careful with (even weak) listeners - at least when
> they fall into some categories. So far we had
> - listeners to path properties
> - listeners that change global state (like in the button skin issue)
> - ?? maybe all
> 
> Personally, I would tend to keep and ignore that test with doc: the ignore could point to
> https://bugs.openjdk.java.net/browse/JDK-8242621 (memory leak on switching tab, which might or not be related), the
> removing code could have a code comment explaining why it is needed.  What do you think?

Looked again, and actually seeing two different issues ;)

A) your test - that is with firing the pulse: fails for both not/removing the listener
B) basically same test, but not firing the pulse - it fails without removing and passes with removing the listener

So I think we should include B as it is directly related to this fix (and verifies the need to remove the listener). As
to A: we should keep it somewhere to not forget, but where?

Test code B:

    @Test
    public void testNPEOnSwitchSkinAndSelect() {
        // want to replace the skin - test doesn't make sense without
        if (!showBeforeReplaceSM) return;
        
        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();

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


The exception (seen on the test stack when rewiring the uncaughthandler as usual, else on sysout):

    Exception in thread "main" java.lang.NullPointerException
	at javafx.controls/javafx.scene.control.skin.TabPaneSkin.lambda$0(TabPaneSkin.java:447)
	at javafx.base/javafx.beans.WeakInvalidationListener.invalidated(WeakInvalidationListener.java:83)
	at javafx.base/com.sun.javafx.binding.ExpressionHelper$Generic.fireValueChangedEvent(ExpressionHelper.java:348)
	at javafx.base/com.sun.javafx.binding.ExpressionHelper.fireValueChangedEvent(ExpressionHelper.java:80)
	at
javafx.base/javafx.beans.property.ReadOnlyObjectPropertyBase.fireValueChangedEvent(ReadOnlyObjectPropertyBase.java:74)
	at javafx.base/javafx.beans.property.ReadOnlyObjectWrapper.fireValueChangedEvent(ReadOnlyObjectWrapper.java:102) 	at
javafx.base/javafx.beans.property.ObjectPropertyBase.markInvalid(ObjectPropertyBase.java:113) 	at
javafx.base/javafx.beans.property.ObjectPropertyBase.set(ObjectPropertyBase.java:147) 	at
javafx.controls/javafx.scene.control.SelectionModel.setSelectedItem(SelectionModel.java:105) 	at
javafx.controls/javafx.scene.control.TabPane$TabPaneSelectionModel.select(TabPane.java:736) 	at
javafx.controls/test.javafx.scene.control.SelectionFocusModelMemoryTest.testNPEOnSwitchSkinAndRemoveTabPaneFirePulse(SelectionFocusModelMemoryTest.java:261)

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

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