[Rev 01] RFR: 8236259: MemoryLeak in ProgressIndicator

[Kevin Rushforth]

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

I haven't looked at the fix itself, but there are two things that _must_ be fixed before this PR can be considered:
1. You need to remove the additional 3rd-party test library dependency.
2. You need to revert the change that makes a test method public API.

build.gradle line 2514:

> 2513:         compile project(':graphics')
> 2514:         testCompile "de.sandec:JMemoryBuddy:0.1.3"
> 2515:     }

This is a new third-party dependency, which would need legal approval. You need to remove this.

modules/javafx.graphics/src/main/java/javafx/scene/Parent.java line 1929:

> 1928:      */
> 1929:     public List<Node> test_getRemoved() {
> 1930:         return removed;

This is not an acceptable change. We do not add public API to support tests. Take a look at the Shim classes for an example of how to handle this.

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

Changes requested by kcr (Lead).

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