Re: RFR: 8240506: TextFieldSkin/Behavior: misbehavior on switching skin [v2]
On Wed, 7 Jul 2021 09:48:20 GMT, Jeanette Winzenburg 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. > > Jeanette Winzenburg has updated the pull request incrementally with one > additional commit since the last revision: > > addressed review issues Sorry for the delay. Looks good to me. :) - Marked as reviewed by mhanl (Author). PR: https://git.openjdk.java.net/jfx/pull/534
Re: RFR: 8240506: TextFieldSkin/Behavior: misbehavior on switching skin [v2]
On Mon, 28 Jun 2021 13:44:16 GMT, Marius Hanl wrote: >> Jeanette Winzenburg has updated the pull request incrementally with one >> additional commit since the last revision: >> >> addressed review issues > > Just a formal review, I left some comments inline @Maran23 did resolve all we discussed - anything I missed? - PR: https://git.openjdk.java.net/jfx/pull/534
Re: RFR: 8240506: TextFieldSkin/Behavior: misbehavior on switching skin [v2]
On Wed, 7 Jul 2021 09:48:20 GMT, Jeanette Winzenburg 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. > > Jeanette Winzenburg has updated the pull request incrementally with one > additional commit since the last revision: > > addressed review issues Looks good to me. - Marked as reviewed by arapte (Reviewer). PR: https://git.openjdk.java.net/jfx/pull/534
Re: RFR: 8240506: TextFieldSkin/Behavior: misbehavior on switching skin [v2]
On Wed, 7 Jul 2021 10:02:52 GMT, Jeanette Winzenburg
wrote:
>> 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();
>> }
>
> Hmm ... don't quite understand: the cleanup follows the same pattern used
> across many controls/skin tests
>
> @After
> public void cleanup() {
> if (stage != null) {
> stage.hide();
> }
>
> The stage is created at most once per test method, and allows to add more
> controls in that same test method, it's hidden after running each test.
> Running the next text, there's no reference to the old .. why should we
> remove its children also? Or maybe I misunderstand what you are suggesting :)
Oops, the current code is proper. I missed the cleanup somehow. This looks good.
-
PR: https://git.openjdk.java.net/jfx/pull/534
Re: RFR: 8240506: TextFieldSkin/Behavior: misbehavior on switching skin [v2]
On Tue, 6 Jul 2021 20:00:16 GMT, Ambarish Rapte wrote:
>> Jeanette Winzenburg has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> addressed review issues
>
> 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();
> }
Hmm ... don't quite understand: the cleanup follows the same pattern used
across many controls/skin tests
@After
public void cleanup() {
if (stage != null) {
stage.hide();
}
The stage is created at most once per test method, and allows to add more
controls in that same test method, it's hidden after running each test. Running
the next text, there's no reference to the old .. why should we remove its
children also? Or maybe I misunderstand what you are suggesting :)
-
PR: https://git.openjdk.java.net/jfx/pull/534
Re: RFR: 8240506: TextFieldSkin/Behavior: misbehavior on switching skin [v2]
On Tue, 6 Jul 2021 20:21:09 GMT, Ambarish Rapte wrote: >> I'm also interested in the opinion from others. I think we are a bit more >> safer with weak listener and there are used often as well. >> But as you correctly mentioned a lot of times (still) a listener is created >> inline. But I think on most occurences where it is not it is wrapped in an >> weak one. > > The main idea here is that we must remove the added listeners. > A Strong listener can cause leak and a WeakListener will not cause leak, but > if not removed then a WeakListener would be executed before it gets GCed. > As long as we are removing them properly on disposal, they both look similar > to me, but yes we can be consistent in choosing appropriate listener. > IMO, we can keep it out of scope of this PR, It can be taken as a follow up > issue and make a consistent change in bulk. okay, leaving as-is - if we'll have too much time in future we can start a overall cleansing round :) - PR: https://git.openjdk.java.net/jfx/pull/534
Re: RFR: 8240506: TextFieldSkin/Behavior: misbehavior on switching skin [v2]
> 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. Jeanette Winzenburg has updated the pull request incrementally with one additional commit since the last revision: addressed review issues - Changes: - all: https://git.openjdk.java.net/jfx/pull/534/files - new: https://git.openjdk.java.net/jfx/pull/534/files/2adf136f..df1ebc09 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jfx=534=01 - incr: https://webrevs.openjdk.java.net/?repo=jfx=534=00-01 Stats: 5 lines in 2 files changed: 0 ins; 1 del; 4 mod Patch: https://git.openjdk.java.net/jfx/pull/534.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/534/head:pull/534 PR: https://git.openjdk.java.net/jfx/pull/534
Re: RFR: 8240506: TextFieldSkin/Behavior: misbehavior on switching skin
On Thu, 17 Jun 2021 12:41:56 GMT, Jeanette Winzenburg
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
Re: RFR: 8240506: TextFieldSkin/Behavior: misbehavior on switching skin
On Fri, 2 Jul 2021 22:58:02 GMT, Marius Hanl wrote: >> Okay, went through listener registrations in all behaviors - and they are >> indeed inconsistent: >> >> - some listen to control properties like focused (f.i. Button, Combo): >> adding strong, often inline listeners >> - some listen to control path properties like selection/Model/Indices (f.i. >> ListView, TreeView): adding weak (inline or field) wrappers >> - cleanup for all guarantees to make those listeners removable (without >> touching their type) and actually remove them in dispose >> >> So I tend to follow that approach here as well, opinions? > > I'm also interested in the opinion from others. I think we are a bit more > safer with weak listener and there are used often as well. > But as you correctly mentioned a lot of times (still) a listener is created > inline. But I think on most occurences where it is not it is wrapped in an > weak one. The main idea here is that we must remove the added listeners. A Strong listener can cause leak and a WeakListener will not cause leak, but if not removed then a WeakListener would be executed before it gets GCed. As long as we are removing them properly on disposal, they both look similar to me, but yes we can be consistent in choosing appropriate listener. IMO, we can keep it out of scope of this PR, It can be taken as a follow up issue and make a consistent change in bulk. - PR: https://git.openjdk.java.net/jfx/pull/534
Re: RFR: 8240506: TextFieldSkin/Behavior: misbehavior on switching skin
On Tue, 29 Jun 2021 12:36:50 GMT, Jeanette Winzenburg wrote: >> we are a bit inconsistent in wrapping (or not) listeners into their weak >> counterparts - behaviors tend to not :) That's okay - and less crowded by >> additional code - as long as they are removed properly, IMO. But just >> seeing: good question, as the focusOwner listener is wrapped, need to >> consult my notes. Thanks for the heads up! >> >> As to the single vs. multiple lines: ersonally, I tend to not change more >> than absolutely needed - it had the brackets before the fix so I kept them. > > Okay, went through listener registrations in all behaviors - and they are > indeed inconsistent: > > - some listen to control properties like focused (f.i. Button, Combo): adding > strong, often inline listeners > - some listen to control path properties like selection/Model/Indices (f.i. > ListView, TreeView): adding weak (inline or field) wrappers > - cleanup for all guarantees to make those listeners removable (without > touching their type) and actually remove them in dispose > > So I tend to follow that approach here as well, opinions? I'm also interested in the opinion from others. I think we are a bit more safer with weak listener and there are used often as well. But as you correctly mentioned a lot of times (still) a listener is created inline. But I think on most occurences where it is not it is wrapped in an weak one. - PR: https://git.openjdk.java.net/jfx/pull/534
Re: RFR: 8240506: TextFieldSkin/Behavior: misbehavior on switching skin
On Mon, 28 Jun 2021 15:42:47 GMT, Jeanette Winzenburg
wrote:
>> modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/behavior/TextFieldBehavior.java
>> line 75:
>>
>>> 73: }
>>> 74:
>>> 75: focusListener = (observable, oldValue, newValue) -> {
>>
>> Shouldn't this focusListener also be wrapped in a weakListener? Also this
>> lambda expression can be a one-liner (no braces needed)
>
> we are a bit inconsistent in wrapping (or not) listeners into their weak
> counterparts - behaviors tend to not :) That's okay - and less crowded by
> additional code - as long as they are removed properly, IMO. But just seeing:
> good question, as the focusOwner listener is wrapped, need to consult my
> notes. Thanks for the heads up!
>
> As to the single vs. multiple lines: ersonally, I tend to not change more
> than absolutely needed - it had the brackets before the fix so I kept them.
Okay, went through listener registrations in all behaviors - and they are
indeed inconsistent:
- some listen to control properties like focused (f.i. Button, Combo): adding
strong, often inline listeners
- some listen to control path properties like selection/Model/Indices (f.i.
ListView, TreeView): adding weak (inline or field) wrappers
- cleanup for all guarantees to make those listeners removable (without
touching their type) and actually remove them in dispose
So I tend to follow that approach here as well, opinions?
-
PR: https://git.openjdk.java.net/jfx/pull/534
Re: RFR: 8240506: TextFieldSkin/Behavior: misbehavior on switching skin
On Mon, 28 Jun 2021 13:18:38 GMT, Marius Hanl 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.
>
> modules/javafx.controls/src/main/java/javafx/scene/control/skin/TextFieldSkin.java
> line 390:
>
>> 388: @Override public void dispose() {
>> 389: if (getSkinnable() == null) return;
>> 390: getChildren().removeAll(textGroup, handleGroup);
>
> Also curious: Most of the skins don't remove their children in **dispose()**.
> Are they all wrong, or is this a special case here?
they are all wrong ;) When starting with the cleanup, we (me at least ;)
weren't yet entirely certain - but not removing them let them hang in the
hierarchy, reachable by strong references from their parent. Which in itself
isn't pretty (and might lead to unwanted side-effects) - but if they in turn
have any references to the skin (even not so obvious - for me again - like
being a not static nested class of the skin ;) the skin is strongly reachable
as well, making it leaky.
> modules/javafx.controls/src/test/java/test/com/sun/javafx/scene/control/behavior/BehaviorCleanupTest.java
> line 162:
>
>> 160: secondStage.hide();
>> 161: }
>> 162:
>
> minor: this empty line can be removed
yeah, can do .. but do we have conventions about vertical whitespace? Tend to
keep what suits my eyes :)
-
PR: https://git.openjdk.java.net/jfx/pull/534
Re: RFR: 8240506: TextFieldSkin/Behavior: misbehavior on switching skin
On Mon, 28 Jun 2021 12:55:47 GMT, Marius Hanl 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.
>
> modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/behavior/TextFieldBehavior.java
> line 75:
>
>> 73: }
>> 74:
>> 75: focusListener = (observable, oldValue, newValue) -> {
>
> Shouldn't this focusListener also be wrapped in a weakListener? Also this
> lambda expression can be a one-liner (no braces needed)
we are a bit inconsistent in wrapping (or not) listeners into their weak
counterparts - behaviors tend to not :) That's okay - and less crowded by
additional code - as long as they are removed properly, IMO. But just seeing:
good question, as the focusOwner listener is wrapped, need to consult my notes.
Thanks for the heads up!
As to the single vs. multiple lines: ersonally, I tend to not change more than
absolutely needed - it had the brackets before the fix so I kept them.
> modules/javafx.controls/src/main/java/javafx/scene/control/skin/TextFieldSkin.java
> line 389:
>
>> 387: /** {@inheritDoc} */
>> 388: @Override public void dispose() {
>> 389: if (getSkinnable() == null) return;
>
> Just curious: Can the skinnable be null here? And is it fine then to never
> call **super.dispose()** ?
dispose has no precondition - it can be called multiple times (explicitly
specified in its javadoc), so has to guard itself against having cleaned
already. And super is called the first time.
-
PR: https://git.openjdk.java.net/jfx/pull/534
Re: RFR: 8240506: TextFieldSkin/Behavior: misbehavior on switching skin
On Thu, 17 Jun 2021 12:41:56 GMT, Jeanette Winzenburg
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.
Just a formal review, I left some comments inline
modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/behavior/TextFieldBehavior.java
line 75:
> 73: }
> 74:
> 75: focusListener = (observable, oldValue, newValue) -> {
Shouldn't this focusListener also be wrapped in a weakListener? Also this
lambda expression can be a one-liner (no braces needed)
modules/javafx.controls/src/main/java/javafx/scene/control/skin/TextFieldSkin.java
line 389:
> 387: /** {@inheritDoc} */
> 388: @Override public void dispose() {
> 389: if (getSkinnable() == null) return;
Just curious: Can the skinnable be null here? And is it fine then to never call
**super.dispose()** ?
modules/javafx.controls/src/main/java/javafx/scene/control/skin/TextFieldSkin.java
line 390:
> 388: @Override public void dispose() {
> 389: if (getSkinnable() == null) return;
> 390: getChildren().removeAll(textGroup, handleGroup);
Also curious: Most of the skins don't remove their children in **dispose()**.
Are they all wrong, or is this a special case here?
modules/javafx.controls/src/test/java/test/com/sun/javafx/scene/control/behavior/BehaviorCleanupTest.java
line 162:
> 160: secondStage.hide();
> 161: }
> 162:
minor: this empty line can be removed
-
Changes requested by mhanl (Author).
PR: https://git.openjdk.java.net/jfx/pull/534
RFR: 8240506: TextFieldSkin/Behavior: misbehavior on switching skin
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. - Commit messages: - 8240506: TextFieldSkin/Behavior: misbehavior on switching skin Changes: https://git.openjdk.java.net/jfx/pull/534/files Webrev: https://webrevs.openjdk.java.net/?repo=jfx=534=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8240506 Stats: 753 lines in 10 files changed: 687 ins; 38 del; 28 mod Patch: https://git.openjdk.java.net/jfx/pull/534.diff Fetch: git fetch https://git.openjdk.java.net/jfx pull/534/head:pull/534 PR: https://git.openjdk.java.net/jfx/pull/534
