8264677 MemoryLeak: Progressindicator leaks, when treeShowing is false #455

FlorianKirmaier · 2021-04-03T15:35:19Z

Fixing leak in ProgressIndicator when treeShowing is false

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8264677: MemoryLeak: Progressindicator leaks, when treeShowing is false

Reviewers

Kevin Rushforth (@kevinrushforth - Reviewer)
Ambarish Rapte (@arapte - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jfx pull/455/head:pull/455
$ git checkout pull/455

Update a local copy of the PR:
$ git checkout pull/455
$ git pull https://git.openjdk.java.net/jfx pull/455/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 455

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jfx/pull/455.diff

Fixing leak in ProgressIndicator when treeShowing is false

bridgekeeper · 2021-04-03T15:35:35Z

👋 Welcome back fkirmaier! 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.

mlbridge · 2021-04-03T15:39:29Z

Webrevs

kevinrushforth · 2021-04-03T15:39:43Z

/reviewers 2

openjdk · 2021-04-03T15:39:53Z

@kevinrushforth
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

kevinrushforth · 2021-04-03T15:41:18Z

Is this a recent regression, or has this case been failing for a long time?

FlorianKirmaier · 2021-04-03T15:51:11Z

I think it's failing since a long time. But i haven't tested it.
It must be, because before the other leak should "shadow" this leak.

FlorianKirmaier · 2021-04-03T15:56:12Z

Before, ProgressIndicator was basically leaking every time, this is a very rare special case.
Because very few people add invisible ProgressIndicators.
I think in the application, it was put slightly delayed visible, but because the task was finished even faster, it never became really visible. So in the affected project it was basically triggered by a rare timing.

kevinrushforth · 2021-04-05T22:39:28Z

I see. Thanks.

/reviewers 2

openjdk · 2021-04-05T22:40:14Z

@kevinrushforth
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

kevinrushforth

I haven't run it yet, but noticed a couple things during a quick code review.

modules/javafx.controls/src/main/java/javafx/scene/control/skin/ProgressIndicatorSkin.java

changes based on code review

FlorianKirmaier · 2021-04-06T12:21:39Z

I've done the 2 changes from the code review!

arapte

Suggested a minor change.
The test file has an existing test memoryTest(). The AfterClass and BeforeClass methods are specific to this test, and not related to the new test. It looks little unstructured with addition of this new test. Not sure if it is worth to change the helper methods and existing test.
otherwise the fix and test looks good.

tests/system/src/test/java/test/javafx/scene/control/ProgressIndicatorLeakTest.java

Updated naming of the test, reworked the old test. It now has a much smaller scope and is easier to reason about, the initialization of JavaFX is now seperated from the test itself.

FlorianKirmaier · 2021-04-09T11:35:01Z

I've now reworked the old test. The initialization of JavaFX is now seperated of the old test.
The old test is now also much simpler. It no longer "jumps around" between different methods.
The waiting for the Stage to be shown is also no longer necessary, which simplifies it even more.

FlorianKirmaier · 2021-04-09T11:36:26Z

Also no longer WeakReferences are needed for the test due to the new memory tests.
No more worrying whether it can become null at the wrong moment.

kevinrushforth

The fix looks good. So does the newly added test, and I like the refactoring of the tests that you've done. I left one question about existing test.

kevinrushforth · 2021-04-09T12:37:07Z

tests/system/src/test/java/test/javafx/scene/control/ProgressIndicatorLeakTest.java

+                indicator.setProgress(-1.0);
+                indicator.setProgress(1.0);
+                checker.assertCollectable(detIndicator);
+                stage.show();


Now that you are no longer waiting for Stage::onShown, isn't it possible for the test to miss a potential leak? Can you explain why you think it's not needed?

That's a good question.
OnShown is usually called in about 1 Frame.
The memory-leak test takes some times, usually longer as 1 Frame.
In the current configuration, it takes up to 1 seconds, with up to 10 checks.
Because the check is so much longer, waiting for onShown doesn't really make a difference.

My worry isn't the test might fail and give a false positive. The checking 10 times will be sufficient for that.

My concern is that since this bug fix is fixing a leak that only happens when the stage is showing, the test might miss a potential leak if a regression in this area were ever reintroduced. I think it's best to restore the wait for onShown.

I've readded the wait onShown. I don't really see a Scenario were this makes a difference, but i guess it's more correct, because it now tests it at the moment, where the object should be free, and not relying on the fact, that the check takes some time.

Readded countdownlatch for onShown based on code review

kevinrushforth

Thanks. Looks good now.

arapte

looks good to me.

openjdk · 2021-04-26T12:46:09Z

@FlorianKirmaier 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:

8264677: MemoryLeak: Progressindicator leaks, when treeShowing is false

Reviewed-by: kcr, arapte

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 35 new commits pushed to the master branch:

6b63bf5: 8265669: AccumCell should not be visible
ed080c8: 8262276: Debug build of WebKit fails
db30e71: 8265513: Openjfx graphics build broken (Eclipse)
b50ce94: 8264952: [TestBug] Controls unit tests - ControlTest and SpinnerTest fail for non US Locale
27e57d3: 8265469: Allow to build media and webkit for Linux-AArch64
dfda00d: 8265514: Openjfx controls running tests broken (Eclipse)
1b407cc: 8239880: CSS tests should cleanup any global state they modify
fab638a: 8265425: Hard failure when building OpenJFX for Linux AArch64
dffdc6f: 8265758: [TestBug] Remove ignored unit test from CustomMenuItemTest
4a9c892: 8264064: Cross compile JavaFX graphics and controls modules for Windows AArch64 (ARM64)
... and 25 more: https://git.openjdk.java.net/jfx/compare/ed5cfe72e0da4e4b90eb83cac7c50fe761f28c04...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@kevinrushforth, @arapte) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

FlorianKirmaier · 2021-04-27T08:42:11Z

/integrate

openjdk · 2021-04-27T08:43:52Z

@FlorianKirmaier
Your change (at version ba02397) is now ready to be sponsored by a Committer.

kevinrushforth · 2021-04-27T11:10:25Z

/sponsor

openjdk · 2021-04-27T11:11:58Z

@kevinrushforth @FlorianKirmaier Since your change was applied there have been 35 commits pushed to the master branch:

6b63bf5: 8265669: AccumCell should not be visible
ed080c8: 8262276: Debug build of WebKit fails
db30e71: 8265513: Openjfx graphics build broken (Eclipse)
b50ce94: 8264952: [TestBug] Controls unit tests - ControlTest and SpinnerTest fail for non US Locale
27e57d3: 8265469: Allow to build media and webkit for Linux-AArch64
dfda00d: 8265514: Openjfx controls running tests broken (Eclipse)
1b407cc: 8239880: CSS tests should cleanup any global state they modify
fab638a: 8265425: Hard failure when building OpenJFX for Linux AArch64
dffdc6f: 8265758: [TestBug] Remove ignored unit test from CustomMenuItemTest
4a9c892: 8264064: Cross compile JavaFX graphics and controls modules for Windows AArch64 (ARM64)
... and 25 more: https://git.openjdk.java.net/jfx/compare/ed5cfe72e0da4e4b90eb83cac7c50fe761f28c04...master

Your commit was automatically rebased without conflicts.

Pushed as commit 483f171.

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

8264677

8e539d3

Fixing leak in ProgressIndicator when treeShowing is false

openjdk bot added the rfr Ready for review label Apr 3, 2021

kevinrushforth reviewed Apr 5, 2021

View reviewed changes

modules/javafx.controls/src/main/java/javafx/scene/control/skin/ProgressIndicatorSkin.java Outdated Show resolved Hide resolved

modules/javafx.controls/src/main/java/javafx/scene/control/skin/ProgressIndicatorSkin.java Show resolved Hide resolved

8264677

1a49a35

changes based on code review

kevinrushforth requested review from kevinrushforth and arapte April 6, 2021 23:20

arapte reviewed Apr 7, 2021

View reviewed changes

tests/system/src/test/java/test/javafx/scene/control/ProgressIndicatorLeakTest.java Outdated Show resolved Hide resolved

8264677

15fae37

Updated naming of the test, reworked the old test. It now has a much smaller scope and is easier to reason about, the initialization of JavaFX is now seperated from the test itself.

kevinrushforth reviewed Apr 9, 2021

View reviewed changes

kevinrushforth self-requested a review April 15, 2021 23:26

8264677

ba02397

Readded countdownlatch for onShown based on code review

kevinrushforth approved these changes Apr 23, 2021

View reviewed changes

arapte approved these changes Apr 26, 2021

View reviewed changes

openjdk bot added the ready Ready to be integrated label Apr 26, 2021

openjdk bot added the sponsor Ready to sponsor label Apr 27, 2021

openjdk bot closed this Apr 27, 2021

openjdk bot added integrated Pull request has been integrated and removed sponsor Ready to sponsor ready Ready to be integrated rfr Ready for review labels Apr 27, 2021

8264677 MemoryLeak: Progressindicator leaks, when treeShowing is false #455

8264677 MemoryLeak: Progressindicator leaks, when treeShowing is false #455

FlorianKirmaier commented Apr 3, 2021 •

edited by openjdk bot

bridgekeeper bot commented Apr 3, 2021

mlbridge bot commented Apr 3, 2021 •

edited

kevinrushforth commented Apr 3, 2021

openjdk bot commented Apr 3, 2021

kevinrushforth commented Apr 3, 2021

FlorianKirmaier commented Apr 3, 2021

FlorianKirmaier commented Apr 3, 2021

kevinrushforth commented Apr 5, 2021

openjdk bot commented Apr 5, 2021

kevinrushforth left a comment

FlorianKirmaier commented Apr 6, 2021

arapte left a comment

FlorianKirmaier commented Apr 9, 2021

FlorianKirmaier commented Apr 9, 2021

kevinrushforth left a comment

kevinrushforth Apr 9, 2021

FlorianKirmaier Apr 11, 2021

kevinrushforth Apr 22, 2021

FlorianKirmaier Apr 23, 2021

kevinrushforth left a comment

arapte left a comment

openjdk bot commented Apr 26, 2021 •

edited

FlorianKirmaier commented Apr 27, 2021

openjdk bot commented Apr 27, 2021

kevinrushforth commented Apr 27, 2021

openjdk bot commented Apr 27, 2021

8264677 MemoryLeak: Progressindicator leaks, when treeShowing is false #455

8264677 MemoryLeak: Progressindicator leaks, when treeShowing is false #455

Conversation

FlorianKirmaier commented Apr 3, 2021 • edited by openjdk bot

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Apr 3, 2021

mlbridge bot commented Apr 3, 2021 • edited

Webrevs

kevinrushforth commented Apr 3, 2021

openjdk bot commented Apr 3, 2021

kevinrushforth commented Apr 3, 2021

FlorianKirmaier commented Apr 3, 2021

FlorianKirmaier commented Apr 3, 2021

kevinrushforth commented Apr 5, 2021

openjdk bot commented Apr 5, 2021

kevinrushforth left a comment

Choose a reason for hiding this comment

FlorianKirmaier commented Apr 6, 2021

arapte left a comment

Choose a reason for hiding this comment

FlorianKirmaier commented Apr 9, 2021

FlorianKirmaier commented Apr 9, 2021

kevinrushforth left a comment

Choose a reason for hiding this comment

kevinrushforth Apr 9, 2021

Choose a reason for hiding this comment

FlorianKirmaier Apr 11, 2021

Choose a reason for hiding this comment

kevinrushforth Apr 22, 2021

Choose a reason for hiding this comment

FlorianKirmaier Apr 23, 2021

Choose a reason for hiding this comment

kevinrushforth left a comment

Choose a reason for hiding this comment

arapte left a comment

Choose a reason for hiding this comment

openjdk bot commented Apr 26, 2021 • edited

FlorianKirmaier commented Apr 27, 2021

openjdk bot commented Apr 27, 2021

kevinrushforth commented Apr 27, 2021

openjdk bot commented Apr 27, 2021

FlorianKirmaier commented Apr 3, 2021 •

edited by openjdk bot

mlbridge bot commented Apr 3, 2021 •

edited

openjdk bot commented Apr 26, 2021 •

edited