8295809: TreeTableViewSkin: memory leak when changing skin #931

andy-goryachev-oracle · 2022-10-24T19:06:26Z

as determined by SkinMemoryLeakTest (remove line 180) and a leak tester
https://github.com/andy-goryachev-oracle/Test/blob/main/src/goryachev/apps/LeakTest.java

caused by:

adding and not removing listeners
adding and not removing event handlers/filters
adding and not removing cell factory
holding unnecessary instance in TreeTableViewSkin.treeTableViewSkin // test-only

NOTES:

this fix requires JDK-8294809 ListenerHelper and JDK-8295806 TableViewSkin.
there were more memory leaks in this skin than detected by SkinMemoryLeakTest, probably because of cell factory (?). A manual test using LeakTest shows that there are no memory leaks after a) replacing skin, b) moving the components to a new window, and c) removing all the components from the window.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8295809: TreeTableViewSkin: memory leak when changing skin

Reviewers

Ajit Ghaisas (@aghaisas - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx pull/931/head:pull/931
$ git checkout pull/931

Update a local copy of the PR:
$ git checkout pull/931
$ git pull https://git.openjdk.org/jfx pull/931/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 931

View PR using the GUI difftool:
$ git pr show -t 931

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/931.diff

…lper

This reverts commit 2df4a85.

…skin

…lper

…95806.table.view.skin

bridgekeeper · 2022-10-24T19:08:29Z

👋 Welcome back angorya! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2022-10-24T19:10:08Z

⚠️ @andy-goryachev-oracle This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

8295806.table.view.skin

andy-goryachev-oracle · 2022-11-28T20:11:08Z

must be reviewed after #929

8295806.table.view.skin

…view.skin

mlbridge · 2022-12-05T17:17:59Z

Webrevs

8295809.tree.table.view.skin

andy-goryachev-oracle · 2022-12-06T17:25:52Z

@aghaisas could you please take a look at this PR?

aghaisas · 2022-12-07T10:16:36Z

modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/SkinMemoryLeakTest.java

@@ -232,10 +232,10 @@ public static Collection<Object[]> data() {
                //MenuBar.class,

                //
-                PasswordField.class,
+                //PasswordField.class,


This seems an unrelated change.

since this is the last PR in the series of skin memory leak fixes, the code block will be removed.

aghaisas · 2022-12-07T10:16:41Z

modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/SkinMemoryLeakTest.java


                //
-                Spinner.class,
+                Spinner.class


This seems an unrelated change.

cleaned up.

aghaisas · 2022-12-07T10:25:50Z

modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/SkinCleanupTest.java

@@ -274,6 +274,8 @@ public void testTreeTableRowFixedCellSizeEnabled() {
        assertTrue("fixed cell size enabled", isFixedCellSizeEnabled(tableRow));
    }

+    @Ignore("JDK-8295809") // TODO probably need to verify the result of listener action,


We need to file a follow-on bug to modify or cleanup these ignored tests in future.
Use the bug ID of the newly filed bug with @Ignore tag for these two tests.

thank you for noticing! the tests are changed to validate functionality rather than implementation detail, i.e. that the tree table row skin tracks the virtual flow width with or without skin replacement.

aghaisas

I have identified the minor changes required.

8295809.tree.table.view.skin

aghaisas

Thanks for fixing the tests. Fix looks good now.

This will have some conflict with #805 fix though.

openjdk · 2022-12-08T14:57:58Z

@andy-goryachev-oracle This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8295809: TreeTableViewSkin: memory leak when changing skin

Reviewed-by: aghaisas

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

andy-goryachev-oracle · 2022-12-08T16:02:06Z

/integrate

openjdk · 2022-12-08T16:04:27Z

Going to push as commit 6abbe08.

openjdk · 2022-12-08T16:05:00Z

@andy-goryachev-oracle Pushed as commit 6abbe08.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

andy-goryachev-oracle added 25 commits October 7, 2022 12:20

8294809: original code

199d0ce

8294809: relicensed by author

019adac

8294809: renamed

78774f4

Merge remote-tracking branch 'origin/master' into 8294809.listener.he…

3b3610a

…lper

8294809: null checks

8ff39da

8294809: override

78dfacf

8294809: cleanup

718e255

8294809: event handlers and filters

1044940

8294809: whitespace

0589e78

8294809: skin base

d52802e

8294809: change listener with callback

cd6cd5c

8294809: remove

39e0bd7

8294809: tests

512a63d

8294809: use weak reference correctly this time

af77693

8294809: removed weak listeners support

2df4a85

Revert "8294809: removed weak listeners support"

c518cbf

This reverts commit 2df4a85.

8294809: is alive

e36d328

8294809: generics

71ca35e

Merge remote-tracking branch 'origin/master' into 8295806.table.view.…

d850dd0

…skin

Merge remote-tracking branch 'origin/master' into 8294809.listener.he…

82c2027

…lper

8294809: map change listener

7a1fa62

Merge remote-tracking branch 'origin/8294809.listener.helper' into 82…

6f790c2

…95806.table.view.skin

8295806: plugged the leak

ea239e3

8295806: placeholder

30f8d00

8295809: plugged the leak

6a4ef95

andy-goryachev-oracle added 3 commits October 24, 2022 14:26

8295809: temporarily disabled some tests

ad9955a

Merge remote-tracking branch 'origin/master' into

85f263a

8295806.table.view.skin

Merge branch '8295806.table.view.skin' into 8295809.tree.table.view.skin

5396a36

8295809: cleanup

ba1154c

andy-goryachev-oracle added 5 commits November 30, 2022 09:23

Merge remote-tracking branch 'origin/master' into

8c5f4df

8295806.table.view.skin

Merge branch '8295806.table.view.skin' into 8295809.tree.table.view.skin

92e75b1

Merge remote-tracking branch 'origin/master' into

ba84ea2

8295806.table.view.skin

Merge branch '8295806.table.view.skin' into 8295809.tree.table.view.skin

b6c10b2

Merge remote-tracking branch 'origin/master' into 8295809.tree.table.…

e6e96d6

…view.skin

andy-goryachev-oracle marked this pull request as ready for review December 5, 2022 17:07

openjdk bot added the rfr Ready for review label Dec 5, 2022

andy-goryachev-oracle added 2 commits December 6, 2022 09:19

Merge remote-tracking branch 'origin/master' into

f9ae740

8295809.tree.table.view.skin

8295809: cleanup

cfa05b7

andy-goryachev-oracle mentioned this pull request Dec 6, 2022

8289357: (Tree)TableView is null in (Tree)TableRowSkin during autosize #805

Closed

3 tasks

aghaisas approved these changes Dec 7, 2022

View reviewed changes

aghaisas suggested changes Dec 7, 2022

View reviewed changes

andy-goryachev-oracle added 2 commits December 7, 2022 08:26

Merge remote-tracking branch 'origin/master' into

4f40923

8295809.tree.table.view.skin

8295809: updated tests

2a153d9

aghaisas approved these changes Dec 8, 2022

View reviewed changes

openjdk bot added the ready Ready to be integrated label Dec 8, 2022

openjdk bot added the integrated Pull request has been integrated label Dec 8, 2022

openjdk bot closed this Dec 8, 2022

openjdk bot removed ready Ready to be integrated rfr Ready for review labels Dec 8, 2022

andy-goryachev-oracle deleted the 8295809.tree.table.view.skin branch December 8, 2022 16:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8295809: TreeTableViewSkin: memory leak when changing skin #931

8295809: TreeTableViewSkin: memory leak when changing skin #931

andy-goryachev-oracle commented Oct 24, 2022 •

edited by openjdk bot

Loading

bridgekeeper bot commented Oct 24, 2022

openjdk bot commented Oct 24, 2022

andy-goryachev-oracle commented Nov 28, 2022

mlbridge bot commented Dec 5, 2022 •

edited

Loading

andy-goryachev-oracle commented Dec 6, 2022

aghaisas Dec 7, 2022

andy-goryachev-oracle Dec 7, 2022

aghaisas Dec 7, 2022

andy-goryachev-oracle Dec 7, 2022

aghaisas Dec 7, 2022

andy-goryachev-oracle Dec 7, 2022

aghaisas left a comment

aghaisas left a comment

openjdk bot commented Dec 8, 2022

andy-goryachev-oracle commented Dec 8, 2022

openjdk bot commented Dec 8, 2022

openjdk bot commented Dec 8, 2022

8295809: TreeTableViewSkin: memory leak when changing skin #931

8295809: TreeTableViewSkin: memory leak when changing skin #931

Conversation

andy-goryachev-oracle commented Oct 24, 2022 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Oct 24, 2022

openjdk bot commented Oct 24, 2022

andy-goryachev-oracle commented Nov 28, 2022

mlbridge bot commented Dec 5, 2022 • edited Loading

Webrevs

andy-goryachev-oracle commented Dec 6, 2022

aghaisas Dec 7, 2022

Choose a reason for hiding this comment

andy-goryachev-oracle Dec 7, 2022

Choose a reason for hiding this comment

aghaisas Dec 7, 2022

Choose a reason for hiding this comment

andy-goryachev-oracle Dec 7, 2022

Choose a reason for hiding this comment

aghaisas Dec 7, 2022

Choose a reason for hiding this comment

andy-goryachev-oracle Dec 7, 2022

Choose a reason for hiding this comment

aghaisas left a comment

Choose a reason for hiding this comment

aghaisas left a comment

Choose a reason for hiding this comment

openjdk bot commented Dec 8, 2022

andy-goryachev-oracle commented Dec 8, 2022

openjdk bot commented Dec 8, 2022

openjdk bot commented Dec 8, 2022

andy-goryachev-oracle commented Oct 24, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Dec 5, 2022 •

edited

Loading