RFR: 8242621: TabPane: Memory leak when switching skin [v3]

[Ambarish Rapte]

On Tue, 5 Jan 2021 16:19:47 GMT, Jeanette Winzenburg <fastegal at openjdk.org> wrote:

> Fix looks good, verified tests failing (except one, commented) before and passing after the fix.


Thanks for the review. I have updated the PR according to all comments. Please take a look. :)

> modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/SkinCleanupTest.java line 289:
> 
>> 287:       assertNull(tabPane.getSkin());
>> 288:       assertEquals(0, tabPane.getChildrenUnmodifiable().size());
>> 289:     }
> 
> Same as above: what's the reason to actually show?
> 
> And just curious:  why is this passing already before the fix?

Corrected to use installDefaultSkin().
It works without this fix is because the children are cleared([here](https://github.com/openjdk/jfx/blob/e74f679fda6f03ee8449836147815fdaafb5d626/modules/javafx.controls/src/main/java/javafx/scene/control/Control.java#L300)) in Control class when the skin is set to null.

> modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/SkinCleanupTest.java line 302:
> 
>> 300:     @Test
>> 301:     public void testNPEWhenRemTabAfterSkinIsReplaced() {
>> 302:       TabPane tabPane = new TabPane();
> 
> Personally I don't like abbreviation in method names, not even test methods. `testNPEWhenRemoveTabAfterSkinIsReplaced` isn't that much longer :)

Corrected.

> modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/SkinCleanupTest.java line 311:
> 
>> 309:     @Test
>> 310:     public void testAddRemTabsAfterSkinIsReplaced() {
>> 311:       TabPane tabPane = new TabPane();
> 
> see above

Corrected

-------------

PR: https://git.openjdk.java.net/jfx/pull/318