[RFC Patch] Memory leak in relation editor

[GerdP]

What steps will reproduce the problem?

1. create new way in an empty layer and select it
2. click on +-button in relations window, this opens the relation editor with the selected way on the right
3. click the button to add the way as member
4. add e.g. type=route as tag for the relation
5. press OK button
6. create heap dump

What is the expected result?

The heap dump should show that the way has a single referer (the newly created relation)

What happens instead?

The heap dump shows that the way has 4 referers (several temporary relations which are created in the relation editor). The more tags you add the more dead refererres there are.

Please provide any additional information below. Attach a screenshot if possible.

This is a regression of r18413.

Relative:URL: ^/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2024-02-05 12:56:34 +0100 (Mon, 05 Feb 2024)
Revision:18969
Build-Date:2024-02-06 02:30:58
URL:https://josm.openstreetmap.de/svn/trunk

Identification: JOSM/1.5 (18969 de) Windows 10 64-Bit
OS Build number: Windows 10 Pro 2009 (19045)
Memory Usage: 274 MB / 1888 MB (191 MB allocated, but free)
Java version: 17.0.8+7-LTS, Azul Systems, Inc., OpenJDK 64-Bit Server VM
Look and Feel: com.sun.java.swing.plaf.windows.WindowsLookAndFeel
Screen: \Display0 1920×1080 (scaling 1.00×1.00)
Maximum Screen Size: 1920×1080
Best cursor sizes: 16×16→32×32, 32×32→32×32
System property file.encoding: Cp1252
System property sun.jnu.encoding: Cp1252
Locale info: de_DE
Numbers with default locale: 1234567890 -> 1234567890
VM arguments: [-Djpackage.app-version=1.5.18789, --add-modules=java.scripting,java.sql,javafx.controls,javafx.media,javafx.swing,javafx.web, --add-exports=java.base/sun.security.action=ALL-UNNAMED, --add-exports=java.desktop/com.sun.imageio.plugins.jpeg=ALL-UNNAMED, --add-exports=java.desktop/com.sun.imageio.spi=ALL-UNNAMED, --add-opens=java.base/java.lang=ALL-UNNAMED, --add-opens=java.base/java.nio=ALL-UNNAMED, --add-opens=java.base/jdk.internal.loader=ALL-UNNAMED, --add-opens=java.base/jdk.internal.ref=ALL-UNNAMED, --add-opens=java.desktop/javax.imageio.spi=ALL-UNNAMED, --add-opens=java.desktop/javax.swing.text.html=ALL-UNNAMED, --add-opens=java.prefs/java.util.prefs=ALL-UNNAMED, -Djpackage.app-path=%UserProfile%\AppData\Local\JOSM\HWConsole.exe]
Dataset consistency test: No problems found

Last errors/warnings:
- 00000.607 W: extended font config - overriding 'filename.Myanmar_Text=mmrtext.ttf' with 'MMRTEXT.TTF'
- 00000.611 W: extended font config - overriding 'filename.Mongolian_Baiti=monbaiti.ttf' with 'MONBAITI.TTF'
- 00001.085 E: java.security.KeyStoreException: Windows-ROOT not found. Ursache: java.security.NoSuchAlgorithmException: Windows-ROOT KeyStore not available

[GerdP]

The problem is caused by frequent calls of IRelationEditorActionAccess.getChangedRelation() which will create new relations when the editor is to create a new relation and thus getEditor().getRelation() returns null.
These copies should either not be created or properly cleaned up by calling setMembers(null).

[GerdP]

work in progress, fixes the problem but I think it's duplicating too much code

[GerdP]

different approach: create new method which returns wether a saved relation would be useful

[GerdP]

avoid to create new relation

[GerdP]

@Taylor: I think the last patch also allows to remove the unit test RelationEditorActionsTest.testNonRegression22024().

Reg. new method IRelationEditorActionAccess.wouldRelationBeUseful():
This more or less duplicates the logic from IRelation.isUseful() which is a disadvantage that you probably wanted to avoid?
Both tests simply check if at least one tag and one member exist for the relation.

[GerdP]

I think this comment in SavingAction.applyNewRelation() is rather misleading. The comment makes assumptions about the implementation of the test isUseful() which are not (yet) implemented.

        // If the user wanted to create a new relation, but hasn't added any members or 
        // tags (specifically the "type" tag), don't add the relation
        if (!newRelation.isUseful())
            return;

Unless other methods are overwritten we don't get to this point anyway and there are still other methods to produce and upload an empty relation in JOSM ;)

[GerdP]

similar to 23527.patch, but doesn't deprecate getChangedRelation(). Instead adds hints to avoid the memory leak

[GerdP]

In 19014/josm:

fix #23527: Memory leak in relation editor

• add new method wouldRelationBeUseful() in IRelationEditorActionAccess which doesn't call the problematic method getChangedRelation() to evaluate whether a relation would be useful if saved.
• add comments and code in unit test for the method that shows how to handle the result of getChangedRelation() in case it is not needed any longer to avoid the memory leak.