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