[Rev 01] RFR: 8241455: Memory leak on replacing selection/focusModel

[Ambarish Rapte]

On Fri, 27 Mar 2020 11:46:30 GMT, Jeanette Winzenburg <fastegal at openjdk.org> wrote:

>> Several controls with selection/focusModels leave memory leaks on replacing the model.
>> 
>> Added tests for all such, those for the misbehaving models fail before and pass after the fix.
>> 
>> for convenience, the bug reference https://bugs.openjdk.java.net/browse/JDK-8241455
>
> Jeanette Winzenburg has updated the pull request incrementally with one additional commit since the last revision:
> 
>   widened issue/fix/test for all controls with selection/focus models

Change looks good to me.
Similar to Skin classes the listeners here are also not explicitly removed.
But I don't see any problematic scenario even if the listeners of an old Model are executed before it gets GCed.
Suggested minor changes in test.

modules/javafx.controls/src/test/java/test/javafx/scene/control/SelectionFocusModelMemoryTest.java line 203:

> 202:         ObservableList<String> data = FXCollections.observableArrayList("Apple", "Orange", "Banana");
> 203:         data.forEach(text -> control.getTabs().add(new Tab("text")));
> 204:         WeakReference<SelectionModel<?>> weakRef = new WeakReference<>(control.getSelectionModel());

`new Tab("text")`  ->  `new Tab(text)`

modules/javafx.controls/src/test/java/test/javafx/scene/control/SelectionFocusModelMemoryTest.java line 200:

> 199:         // has its own issue https://bugs.openjdk.java.net/browse/JDK-8241737
> 200:         if (showBeforeReplaceSM) return;
> 201:         TabPane control = new TabPane();

@Ignore is used to ignore the test that are known to fail, It also becomes easy to track the ignored test with the
@Ignore tag. I think it will be good to tag this scenario with a commented @Ignore inside the if statement
`//@Ignore("JDK-8241737")`

modules/javafx.controls/src/test/java/test/javafx/scene/control/SelectionFocusModelMemoryTest.java line 228:

> 227:         // will be fixed as side-effect of skin cleanup in https://bugs.openjdk.java.net/browse/JDK-8087555
> 228:         if (showBeforeReplaceSM) return;
> 229:         ChoiceBox<String> control = new ChoiceBox<>(FXCollections.observableArrayList("Apple", "Orange",
> "Banana"));

@Ignore inside the if statement,
`//@Ignore("JDK-8087555")`

modules/javafx.controls/src/test/java/test/javafx/scene/control/SelectionFocusModelMemoryTest.java line 285:

> 284:         }
> 285:         root.getChildren().add(node);
> 286:         if (!stage.isShowing()) {

Will it be good to add a call to `root.getChildren().removeAll()` before adding the `node` to `root` ?

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

Changes requested by arapte (Reviewer).

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