Memory leak issues related to 2 PRs in OpenJFX

[Ed Kennard]

Hi everyone,

I’ve been migrating our codebase to Java 11 LTS and OpenJFX 14.  One of our GC-related unit tests is failing now, where one of our custom Tab controls is supposed to be GC’d.  The test passes against Java 8.

When our GC-related unit tests fail they output exactly what it is that’s blocking GC, and in this case it’s pointing to Node.windowShowingChangedListener.  If I disable that listener and recompile JavaFX, then the test fails again but this time due to Node.sceneWindowChangedListener.  If I disable that then the test passes.  Also, if I re-enable both of these listeners but wrap them in WeakListeners, then the test passes.

I believe the two PRs which introduced these listeners are:

8090322: Need new tree visible property in Node that consider Scene and Stage visibility [1]
8216377: Fix memoryleak for initial nodes of Window [2]

If I trace when the windowShowingChangedListener is being added or removed for my custom controls, I can see that for one of them (shown as “Grid” below) they’re being consistently added/removed out of sequence.  This seems to result in an attempt to remove a listener which hasn’t been added yet, then later on that missed addition happens, resulting in one listener incorrectly leftover at the end:

invalidatedScenes - Adding window.showing listener for MetadataTableView
invalidatedScenes - Removing window.showing listener for MetadataTableView
*** THIS IS PREMATURE *** invalidatedScenes - Removing window.showing listener for Grid
invalidatedScenes - Adding window.showing listener for MetadataTableView
invalidatedScenes - Adding window.showing listener for Grid
invalidatedScenes - Adding window.showing listener for Grid
invalidatedScenes - Removing window.showing listener for MetadataTableView
invalidatedScenes - Removing window.showing listener for Grid

I’ve been trying hard to reproduce the issue in a simple test along the same lines as the existing one in systemTests…InitialNodesMemoryLeakTest, but so far haven’t managed to.  There’s quite a bit going on in our custom controls, with some places where it’s necessary to use Platform.runLater to ensure a control has been properly initialized before performing other setup operations, maybe it’s because of something like that.

Anyway, my question is, do you think it might be safer to change these listeners to be weak, in case others manage to create a similar scenario and start leaking nodes like this?  If yes, happy to put a PR together for that.  Or should I continue my pursuit of the underlying issue which has so far eluded me, and which I appreciate may be caused by something being done in the wrong order in my own code?

Thanks

Ed

[1] https://bugs.openjdk.java.net/browse/JDK-8090322
[2] https://bugs.openjdk.java.net/browse/JDK-8216377