[Patch] Memory leak: Markerlayer doesn't remove MouseListener

[GerdP]

What steps will reproduce the problem?

1. load a gpx file xyz.gpx containing waypoints
2. JOSM creates two layers, one called xyz.gpx, the other Markers from xyz.gpx
3. close both layers
4. create heap hump

What is the expected result?

No instance of class org.openstreetmap.josm.gui.layer.markerlayer.Marker or org.openstreetmap.josm.gui.layer.markerlayer.MarkerLayer, they should all be gced.

What happens instead?

The instances are kept as long as the MapView instance exists.

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

Build-Date:2024-01-19 16:41:08
Revision:18947
Is-Local-Build:true

Identification: JOSM/1.5 (18947 SVN en) Windows 10 64-Bit
OS Build number: Windows 10 Home 2009 (19045)
Memory Usage: 442 MB / 1678 MB (246 MB allocated, but free)
Java version: 1.8.0_272-b10, AdoptOpenJDK, 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: UTF-8
System property sun.jnu.encoding: Cp1252
Locale info: en_DE
Numbers with default locale: 1234567890 -> 1234567890
VM arguments: [-agentlib:jdwp=transport=dt_socket,suspend=y,address=localhost:58872, -ea, -javaagent:D:\eclipse-java-2020-09\eclipse\configuration\org.eclipse.osgi\3245\0\.cp\lib\javaagent-shaded.jar, -Dfile.encoding=UTF-8, -Dstdout.encoding=UTF-8, -Dstderr.encoding=UTF-8]
Program arguments: [--language=en, --debug]

Plugins:
+ OpeningHoursEditor (36196)
+ RoadSigns (36196)
+ apache-commons (36176)
+ buildings_tools (36200)
+ comfort0 (36200)
+ o5m (36126)
+ pbf (36176)
+ poly (36126)
+ reltoolbox (36200)
+ reverter (36196)
+ undelete (36126)
+ utilsplugin2 (36200)

Validator rules:
+ c:\josm\core\resources\data\validator\combinations.mapcss
+ c:\josm\core\resources\data\validator\geometry.mapcss
+ c:\josm\core\resources\data\validator\relation.mapcss
+ c:\josm\core\resources\data\validator\unnecessary.mapcss
+ d:\java_tools\JOSM\mygeometry.mapcss
+ https://josm.openstreetmap.de/josmfile?page=Rules/GermanySpecific&zip=1

[GerdP]

add code to remove MouseListener

[stoecker]

Hmm. Instead of adding a getMapView(), wouldn't a destroy() or removeListener() be better? And moving the addListener into the adapter contructor as well to be consistent?

[GerdP]

Thanks for review.
Replying to stoecker:

Hmm. Instead of adding a getMapView(), wouldn't a destroy() or removeListener() be better? And moving the addListener into the adapter contructor as well to be consistent?

Not sure what you mean here. The patch adds the removeMouseListener() call in MarkerLayer.destroy(). I've never seen an adappter constructor which calls addListener() itself. In JosmAction we have installAdapters() and that first creates an instance of the adapter and then calls the addListener() method for that adapter.
Reg. getMapView() in the adapter: I didn't think too much about a better place to store the reference. My thought was that we may have a single instance of MarkerLayer which could be attached to multiple MapView instances, therefore I thought it would be wrong to store the reference in MarkerLayer, but I see now that this cannot work with a simple field for mouseAdapter. It should probably be a collection.
Anyhow, we have no detachToMapView() method sofar, so there is probably no way yet to attach a layer to multiple MapView instances.
I'll attach a new patch..

[GerdP]

store reference to MapView in MarkerLayer and log warning whenattachToMapView() is called more than once

[stoecker]

store reference to MapView in MarkerLayer and log warning whenattachToMapView() is called more than once

Looks better.

My main concern was that the previous patch linked the classed more tightly. Interfaces should be constructed in a way that interlinking is minimized. In this case the mapview should be handled in the adapter (like my suggestion) or like your current patch outside. As your second attempt reduces the complexity of the adapter it's probably the better variant.

[GerdP]

In 18948/josm:

fix #23423 Memory leak: Markerlayer doesn't remove MouseListener

• add removeMouseListener() call in destroy()