RFR: 8269907 memory leak - Dirty Nodes / Parent removed [v7]

[John Hendrikx]

On Thu, 26 Jan 2023 09:54:25 GMT, Florian Kirmaier <fkirmaier at openjdk.org> wrote:

>> After thinking about this issue for some time, I've now got a solution.
>> I know put the scene in the state it is, before is was shown, when the dirtyNodes are unset, the whole scene is basically considered dirty. 
>> This has the drawback of rerendering, whenever a window is "reshown", but it restores sanity about memory behaviour, which should be considered more important.
>
> Florian Kirmaier has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains eight commits:
> 
>  - JDK-8269907
>    Added missing changes after merge
>  - Merge remote-tracking branch 'origjfx/master' into JDK-8269907-dirty-and-removed
>    
>    # Conflicts:
>    #	modules/javafx.graphics/src/main/java/com/sun/javafx/tk/Toolkit.java
>    #	modules/javafx.graphics/src/main/java/javafx/scene/Scene.java
>  - Merge remote-tracking branch 'origin/master'
>  - JDK-8269907
>    Removed the sync methods for the scene, because they don't work when peer is null, and they are not necessary.
>  - JDK-8269907
>    Fixed rare bug, causing bounds to be out of sync.
>  - JDK-8269907
>    We now require the rendering lock when cleaning up dirty nodes. To do so, we moved some code required for snapshot into a reusable method.
>  - JDK-8269907
>    The bug is now fixed in a new way. Toolkit now supports registering CleanupListeners, which can clean up the dirty nodes, avoiding memoryleaks.
>  - JDK-8269907
>    Fixing dirty nodes and parent removed, when a window is no longer showing.  This typically happens with context menus.

I took another good look at this solution, and I would like to offer an alternative as I think this solution is more dealing with symptoms of the underlying problem.

I think the underlying problem is:

1) `Parent` is tracking a `removed` list, which is a list of `Node`. However, it only requires the peer for the dirty area calculation. This is a classic case of not being specific enough with the information you need. If `Parent` is modified to store `NGNode` in its `removed` list, then the problem of `removed` keeping references to old nodes disappears as `NGNode` does not refer back to `Node`.  Note: there is a test method currently in `Parent` that returns the list of removed nodes -- these tests would need to be modified to work with a list of `NGNode` or some other way should be found to check these cases.

2) `Scene` keeps tracking dirty nodes even when it is not visible. The list of dirty nodes could (before this fix) become as big as you want as it was never cleared as the pulse listener that synchronizes the nodes never runs when `peer == null` -- keep adding and removing new nodes while the scene is not shown, and the list of dirty nodes grows indefinitely. This only happens if the scene has been shown at least once before, as before that time special code kicks in that tries to avoid keeping track of all scene nodes in the dirty list.

I think the better solution here would be to reset the scene to go back to its initial state (before it was shown) and sync all nodes when it becomes visible again. This can be achieved by setting the dirty node list to `null` to trigger the logic that normally only triggers on the first show.  `addToDirtyList` should simply do nothing when `peer == null` .

I believe the `syncPeer` call is smart enough not to update the peer in the situation where a scene is hidden and then reshown, even if `Scene` calls it again on all its nodes (`syncPeer` in `Node` will check `dirtyBits.isEmpty()`).

I'd also highly recommend using `ArrayList` instead of a manual `Node[] dirtyNodes` in `Scene`.

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

PR: https://git.openjdk.org/jfx/pull/584