On Mon, 2 Mar 2020 10:09:57 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. > > The pull request has been updated with 1 additional commit. The fix looks good to me. I've verified that it fixes the leak. The newly added test looks good as well, with some comments left inline (formatting, a missing copyright header, and a couple other suggestions). modules/javafx.controls/src/main/java/javafx/scene/control/skin/ProgressIndicatorSkin.java line 331: > 330: // clean up the old determinateIndicator > 331: if(determinateIndicator != null) { > 332: determinateIndicator.unregisterListener(); Minor: add a space after the `if`. tests/system/src/test/java/test/javafx/scene/control/ProgressIndicatorLeakTest.java line 1: > 1: package test.javafx.scene.control; > 2: You need a copyright header for this file. modules/javafx.controls/src/main/java/javafx/scene/control/skin/ProgressIndicatorSkin.java line 60: > 59: import javafx.scene.shape.Circle; > 60: import javafx.scene.text.Font; > 61: import javafx.scene.text.Text; You don't use any of the 3 newly added imports in this patch, so they can be removed (reverted). tests/system/src/test/java/test/javafx/scene/control/ProgressIndicatorLeakTest.java line 42: > 41: indicator.setProgress(1.0); > 42: Assert.assertTrue("size was: " + > indicator.getChildrenUnmodifiable().size(), > indicator.getChildrenUnmodifiable().size() == 1); > 43: detIndicator = new > WeakReference<Node>(indicator.getChildrenUnmodifiable().get(0)); This assertion can be done as: assertEquals("size is wrong", 1, indicator.getChildrenUnmodifiable().size()); It will be easier to read and more straight-forward. tests/system/src/test/java/test/javafx/scene/control/ProgressIndicatorLeakTest.java line 57: > 56: @BeforeClass > 57: public static void initFX() { > 58: startupLatch = new CountDownLatch(1); The `initFX` method can be simplified a bit using a pattern we've adopted in our newer tests. See [QuadraticCssTimeTest.java](https://github.com/openjdk/jfx/blob/jfx14/tests/system/src/test/java/test/javafx/scene/QuadraticCssTimeTest.java#L84). tests/system/src/test/java/test/javafx/scene/control/ProgressIndicatorLeakTest.java line 72: > 71: public void memoryTest() throws > NoSuchFieldException,IllegalAccessException { > 72: System.out.println("detIndicator: " + detIndicator.get()); > 73: assertCollectable(detIndicator); I recommend to comment out this debugging statement. tests/system/src/test/java/test/javafx/scene/control/ProgressIndicatorLeakTest.java line 80: > 79: createGarbage(); > 80: System.gc(); > 81: We recommend also calling `System.runFinalization();` for GC related tests. tests/system/src/test/java/test/javafx/scene/control/ProgressIndicatorLeakTest.java line 85: > 84: Thread.sleep(100); > 85: } catch (InterruptedException e) {} > 86: counter = counter + 1; You can skip the try / catch if you declare the test method as `throws Exception` tests/system/src/test/java/test/javafx/scene/control/ProgressIndicatorLeakTest.java line 82: > 81: > 82: while(counter < 10 && weakReference.get() != null) { > 83: try { Please add a space after the `while`. tests/system/src/test/java/test/javafx/scene/control/ProgressIndicatorLeakTest.java line 91: > 90: > 91: if(weakReference.get() != null) { > 92: throw new AssertionError("Content of WeakReference was not > collected. content: " + weakReference.get()); Add a space after the `if` tests/system/src/test/java/test/javafx/scene/control/ProgressIndicatorLeakTest.java line 95: > 94: } > 95: public static void createGarbage() { > 96: LinkedList list = new LinkedList<Integer>(); I think this is fine, but I'm curious as to whether you've actually found this (creating garbage) to be necessary. tests/system/src/test/java/test/javafx/scene/control/ProgressIndicatorLeakTest.java line 98: > 97: int counter = 0; > 98: while(counter < 999999) { > 99: counter += 1; Space after the `while` tests/system/src/test/java/test/javafx/scene/control/ProgressIndicatorLeakTest.java line 88: > 87: createGarbage(); > 88: System.gc(); > 89: } Same comment as earlier about also calling `System.runFinalization()`; ------------- PR: https://git.openjdk.java.net/jfx/pull/71
