RFR: 8236259: MemoryLeak in ProgressIndicator

[Jeanette Winzenburg]

On Thu, 19 Dec 2019 12:35:56 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.

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

> 520:             registerChangeListener(text.fontProperty(), (Consumer<ObservableValue<?>>) fontListener);
> 521: 
> 522:             // The circular background for the progress pie piece

wondering why you add a (strong) field reference here? 

Thought that registerChangeListener(observableValue, consumer) takes care of handling storage/cleanup - provided its counter-method unregisterChangeListener(observableValue) is called, which is missing, so seems to be the only part that has to be added. Or not?

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

> 554:         }
> 555: 
> 556:         private void setFillOverride(Paint fillOverride) {

just a side-note: this will remove the whole chain of listeners to that property. Not a problem here, because the indicator seems to be the only participant listening. A bit strange using the skin's LambdaMultiplePropertyChangeListenerHandler ...

-------------

PR: https://git.openjdk.java.net/jfx/pull/71