RFR: 8232824: Removing TabPane with strong referenced content causes memory leak from weak one

[Kevin Rushforth]

On Wed, 22 Jan 2020 17:53:38 GMT, Ambarish Rapte <arapte at openjdk.org> wrote:

>> The fix looks good. I'll take a closer look at the unit test later.
>> 
>> Speaking of tests...since the addition of the `TabPane` reordering logic was a victim of the already-existing leak in the `viewOrderChildren` list in `Parent`, it should be possible to write a test case using a Group node and a few Shape nodes, using setViewOrder directly on the Group node (this would be in addition to the system test you wrote). Can you take a look at adding one? It might even be possible to do it as a `javafx.graphics` module unit test rather than a system test, although you would need to see if the bug reproduced there (I suspect it will).
> 
>> 
>> 
>> The fix looks good. I'll take a closer look at the unit test later.
>> 
>> Speaking of tests...since the addition of the `TabPane` reordering logic was a victim of the already-existing leak in the `viewOrderChildren` list in `Parent`, it should be possible to write a test case using a Group node and a few Shape nodes, using setViewOrder directly on the Group node (this would be in addition to the system test you wrote). Can you take a look at adding one? It might even be possible to do it as a `javafx.graphics` module unit test rather than a system test, although you would need to see if the bug reproduced there (I suspect it will).
> 
> Hello Kevin,
> The bug can be reproduced with system test written using `Group` and `Shape` which is very similar to `TabPaneHeaderLeakTest` test. but it seems the bug is not reproducible with unit test. I tried a unit test very similar to the newly added system test `ShapeViewOrderLeakTest`, but looks like `Parent.viewOrderChildren` list does not get populated and so the issue does not occur.

> The bug can be reproduced with system test written using `Group` and `Shape` which is very similar to `TabPaneHeaderLeakTest` test. but it seems the bug is not reproducible with unit test. I tried a unit test very similar to the newly added system test `ShapeViewOrderLeakTest`, but looks like `Parent.viewOrderChildren` list does not get populated and so the issue does not occur.

Did you run a pulse? That would be needed in order to sync the changes down to the peer. In any event, it is fine to use a system test if you can't get it to fail with a unit test.

I have three cleanup comments that are apply to both of the new tests. The first is the most important of these.

1. Test classes should not extend from `javafx.application.Application`. You should use a nested class that extends Application. See [this comment on PR #34](https://github.com/openjdk/jfx/pull/34#pullrequestreview-322619657) for at least one reason why.

2. The initFX method can be simplified 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)

3. Most tests run `startupLatch::countDown` in a `Platform.runLater` call.

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

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