[Rev 01] RFR: 8241737: TabPaneSkin memory leak on replacing selectionModel

Jeanette Winzenburg fastegal at openjdk.java.net
Wed Apr 15 11:17:03 UTC 2020

On Wed, 15 Apr 2020 10:17:41 GMT, Ambarish Rapte <arapte at 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

More information about the openjfx-dev mailing list