On Tue, 10 Mar 2020 14:35:59 GMT, Florian Kirmaier <fkirma...@openjdk.org> wrote:
>> Hi everyone, >> >> ticket: https://bugs.openjdk.java.net/browse/JDK-8236259 >> >> The fix itself is quite straight forward. >> It basically just removed the listener which causes the leak. >> >> The unit-test for the fix is a bit more complicated. >> >> I added a library JMemoryBuddy https://github.com/Sandec/JMemoryBuddy >> (written by myself), which simplifies testing for >> memory leaks. I think there are many places in the JavaFX-codebase that >> could highly benefit from this library. >> It could also simplify many of the already existing unit tests. >> It makes testing for memory-leaks readably and reliable. >> It would also be possible to just copy the code of the library into the >> JavaFX-codebase. >> It only contains a single class. >> >> I also had to make a method public, to write the test. I'm open to ideas, >> how I could solve it differently. > > Florian Kirmaier has updated the pull request incrementally with one > additional commit since the last revision: > > JDK-8200224 > cleaned up code based on code review Requested few changes in test and a suggestion for fix. tests/system/src/test/java/test/javafx/scene/control/ProgressIndicatorLeakTest.java line 45: > 44: import java.util.concurrent.TimeUnit; > 45: > 46: import junit.framework.Assert; Please remove the unused imports. tests/system/src/test/java/test/javafx/scene/control/ProgressIndicatorLeakTest.java line 78: > 77: primaryStage.show(); > 78: } > 79: } Use `stage` or `primaryStage` uniformly to make `Stage` calls in the `start()` method. tests/system/src/test/java/test/javafx/scene/control/ProgressIndicatorLeakTest.java line 76: > 75: }); > 76: }); > 77: primaryStage.show(); This call can be changed as, primaryStage.setOnShown(l -> { Platform.runLater(() -> startupLatch.countDown()); }); tests/system/src/test/java/test/javafx/scene/control/ProgressIndicatorLeakTest.java line 112: > 111: throw new AssertionError("Content of WeakReference was not > collected. content: " + > weakReference.get()); 112: } > 113: } This can be replaced using `Assert.assertNull()` tests/system/src/test/java/test/javafx/scene/control/ProgressIndicatorLeakTest.java line 131: > 130: > 131: } You can get rid of some empty lines like 130, 89, 85. tests/system/src/test/java/test/javafx/scene/control/ProgressIndicatorLeakTest.java line 114: > 113: } > 114: public static void createGarbage() { > 115: LinkedList list = new LinkedList<Integer>(); I think the test should consistently pass or fail without using this method. Can you please check and remove this method if can be. I tested once and it passes. modules/javafx.controls/src/main/java/javafx/scene/control/skin/ProgressIndicatorSkin.java line 550: > 549: } > 550: > 551: private void setFillOverride(Paint fillOverride) { The issue can be alternatively fixed using below change at line#510 text.fontProperty().addListener(new WeakChangeListener<>((observable, oldValue, newValue) -> { doneTextWidth = Utils.computeTextWidth(text.getFont(), DONE, 0); doneTextHeight = Utils.computeTextHeight(text.getFont(), DONE, 0, TextBoundsType.LOGICAL_VERTICAL_CENTER); })); I am not sure which one is better. As there are no objections, I am good with the any. ------------- Changes requested by arapte (Reviewer). PR: https://git.openjdk.java.net/jfx/pull/71
