On Tue, 14 Apr 2020 12:20:16 GMT, Ambarish Rapte <ara...@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
