[Rev 03] RFR: 8236259: MemoryLeak in ProgressIndicator

[Kevin Rushforth]

On Mon, 2 Mar 2020 10:09:57 GMT, Florian Kirmaier <fkirmaier at 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