RFR: 8241737: TabPaneSkin memory leak on replacing selectionModel

Jeanette Winzenburg fastegal at openjdk.java.net
Tue Apr 14 13:03:36 UTC 2020

On Tue, 14 Apr 2020 12:20:16 GMT, Ambarish Rapte <arapte at openjdk.org> wrote:

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

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?


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

More information about the openjfx-dev mailing list