The first of these bugs, 8090322, did introduce at least one memory leak
for which 8216377 was the fix.
I'd prefer to resolve the underlying problem rather than just making
those listeners weak, so it seems worth spending some more time to try
to narrow it down.
Regarding the out-of-order events, are you doing all operations
(including those triggered by binding) on the JavaFX application thread?
My guess is that this is unlikely to be the problem, since you say that
it is very reproducible, but I thought I'd ask.
-- Kevin
On 2/4/2020 9:48 AM, Ed Kennard wrote:
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
