On Wed, 15 Apr 2020 10:17:41 GMT, Ambarish Rapte <ara...@openjdk.org> wrote:
>> 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() { >> 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) > >> B) basically same test, but not firing the pulse - it fails without removing >> and passes with removing the listener > > Seems like this test randomly crashes (not always) any other test from > `TabPaneTest`. It might be crashing the test > executed just after this one OR the next test which does `tk.firePulse()`. > Also not including `tk.firePulse()` would > make the test incomplete/ unreliable. >> Personally, I would tend to keep and ignore that test with doc: > > That is better to include the test now, else very doubtful of adding it in > future. Including the test in next commit > with @Ignore("JDK-8242621"). can't reproduce the random crashing (but then, it's random :) And don't quite understand why you think the test being incomplete/unreliable without a firePulse (there are tons of tests without) - as long as we don't want to test the result of a layout pass, we should be fine. anyway, that's a different story, to me your fix and tests look fine ------------- PR: https://git.openjdk.java.net/jfx/pull/175