RFR: 8240506: TextFieldSkin/Behavior: misbehavior on switching skin

[Ambarish Rapte]

On Thu, 17 Jun 2021 12:41:56 GMT, Jeanette Winzenburg <fastegal at 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