On Mon, 13 Apr 2020 15:47:00 GMT, Jeanette Winzenburg <faste...@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
