On Tue, 14 Apr 2020 13:01:18 GMT, Jeanette Winzenburg <faste...@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