On Thu, 17 Jun 2021 12:41:56 GMT, Jeanette Winzenburg <faste...@openjdk.org>
wrote:
> The issue is about memory leaks and side-effects (like NPEs) when switching
> skins.
>
> Details (copied from issue for convenience):
>
> memory leak in TextInputControlBehavior:
> - listener accidentally added twice (removed once)
> - keyPad mappings not properly installed/disposed
>
> memory leak TextFieldBehavior:
> - listeners to scene/focusOwner property not removed,
>
> memory leak in TextInputControlSkin:
> - event handlers related to inputMethods not removed
>
> issues in TextFieldSkin:
> - memory leak due to behavior leaking
> - memory leaks due to manually added change/invalidation listeners that are
> not removed
> - memory leaks due to not removing children with strong references to skin
> - side-effects (f.i. NPEs) due to listeners/bindings that are still active
> after dispose
>
> Fix was to properly install/remove those listeners/handlers. Added tests that
> failed/passed before/after the fix, respectively, also added tests passing
> before that must pass after the fix.
The fix looks good to me, provided few minor comments.
modules/javafx.controls/src/main/java/javafx/scene/control/skin/TextInputControlSkin.java
line 370:
> 368: public void dispose() {
> 369: if (getSkinnable() == null) return;
> 370: // the inputMethodEvent handler installed by this skin must be
> removed prevent a memory leak
minor typo: missing `to`: -> must be removed `to` prevent
modules/javafx.controls/src/test/java/test/com/sun/javafx/scene/control/behavior/BehaviorCleanupTest.java
line 80:
> 78: Button button = new Button("dummy");
> 79: showControl(button, true);
> 80: assertFalse("caret must be blinking on focus gained",
> isCaretBlinking(control));
Should the message be not like, `Caret must not be blinking when TextField
looses focus` ?
modules/javafx.controls/src/test/java/test/com/sun/javafx/scene/control/behavior/BehaviorCleanupTest.java
line 174:
> 172: assertFalse("sanity: inputMap has child maps",
> inputMap.getChildInputMaps().isEmpty());
> 173: behavior.dispose();
> 174: assertEquals("default child maps be cleared", 0,
> inputMap.getChildInputMaps().size());
minor typo: default child maps `must` be cleared
modules/javafx.controls/src/test/java/test/com/sun/javafx/scene/control/behavior/BehaviorCleanupTest.java
line 404:
> 402: }
> 403: if (!root.getChildren().contains(control)) {
> 404: root.getChildren().add(control);
The controls added to root are not removed. I think we should clear the
scenegraph after execution of each test.
suggesting to add following call in the cleanup method,
if (root != null) {
root.getChildren().removeAll();
}
-------------
Changes requested by arapte (Reviewer).
PR: https://git.openjdk.java.net/jfx/pull/534
