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 think this can be fixed differently and with far less changes required.

See the comment I added on the test case.

modules/javafx.graphics/src/main/java/javafx/scene/Scene.java line 743:

> 741:         synchronizeSceneNodesWithLock();
> 742:     };
> 743:     private void checkCleanDirtyNodes() {

minor: empty line to separate methods and field declarations

tests/system/src/test/java/test/javafx/scene/DirtyNodesLeakTest.java line 74:

> 72:                         showingLatch.countDown();
> 73:                     });
> 74:                 });

I've been looking at this solution, and from what I see, we're doing our best to add a clean up listener which (as far as I can tell without any comments being added) is called when the stage is no longer shown.

The test above creates a stage, adds a label to it in a stack pane, then clears the children.  I think that should be enough to make the label collectable.

I get the impression that you also have to close the stage to make it collectable, which should not be needed at all.

If I'm correct, then this solution introduces quite some new code but doesn't properly address the issue. The dirty nodes list is clearly holding on to nodes that may not be part of the scene any more. 

Looking at the code, I see that `dirtyNodes` is a poor copy of what `ArrayList` does.  It doesn't clear the array with `null`s when it is resizing the array or when it removes elements from the array (or clears it).

The code that I think is the culprit is this part:

                // This is not the first time this scene has been synchronized,
                // so we will only synchronize those nodes that need it
                for (int i = 0 ; i < dirtyNodesSize; ++i) {
                    Node node = dirtyNodes[i];
                    dirtyNodes[i] = null;
                    if (node.getScene() == Scene.this) {
                            node.syncPeer();
                    }
                }
                dirtyNodesSize = 0;

The problem here is that it sets `dirtyNodesSize` to 0, but doesn't clear the array, leaving references to all kinds of nodes in there.

My recommendation to solve this would be to change this code to use `ArrayList` instead of a hand rolled solution. `ArrayList` will properly `null` out slots in its array when they become unused.

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

Changes requested by jhendrikx (Committer).

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