[Rev 04] RFR: 8236259: MemoryLeak in ProgressIndicator

[Ambarish Rapte]

On Tue, 10 Mar 2020 14:35:59 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.
>
> 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