Memory leak issues related to 2 PRs in OpenJFX

[Ed Kennard]

Hi Kevin,

I found the problem with this, and I've pushed a small commit to my fork which I think reproduces the behavior I described [1].  Per comments in the code, to run the test you'll need to make the ControlsFX library v11 available to the build, sorry I didn't do that myself but I'm not familiar with Gradle and it's late in the eve for me now.

We rely on ControlsFX's validation framework in our forms, and it's that which seems to be triggering the scene to be updated in a way that throws out the timing of the adding/removing of the listeners in Node, at least during initialization.  I think after initializing the first time, everything works as expected.  Hope that helps - pls let me know what you think.

Regarding our code, I've worked around the issue by adding a flag to pause the validation decorations during intialization, so I'm able to move forward now.

Thanks

Ed

[1] https://github.com/edkennard/jfx/commit/5c724670cd2ff14ec98d37faf8841e74a7e08039



On 04/02/2020, 19:41, "openjfx-dev on behalf of Kevin Rushforth" <openjfx-dev-bounces at openjdk.java.net on behalf of kevin.rushforth at oracle.com> wrote:

    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
    >