Changeset 19014 in josm

Timestamp:

2024-03-08T10:58:03+01:00 (2 months ago)

Author:

GerdP

Message:

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.

Location:

trunk

Files:

• src/org/openstreetmap/josm/gui/dialogs/relation/GenericRelationEditor.java (modified) (4 diffs)
• src/org/openstreetmap/josm/gui/dialogs/relation/actions/ApplyAction.java (modified) (1 diff)
• src/org/openstreetmap/josm/gui/dialogs/relation/actions/CancelAction.java (modified) (4 diffs)
• src/org/openstreetmap/josm/gui/dialogs/relation/actions/IRelationEditorActionAccess.java (modified) (2 diffs)
• test/unit/org/openstreetmap/josm/gui/dialogs/relation/actions/RelationEditorActionsTest.java (modified) (2 diffs)

trunk/src/org/openstreetmap/josm/gui/dialogs/relation/GenericRelationEditor.java

@@ -56 +56 @@
 import org.openstreetmap.josm.data.UndoRedoHandler.CommandQueueListener;
 import org.openstreetmap.josm.data.osm.DefaultNameFormatter;
-import org.openstreetmap.josm.data.osm.IRelation;
 import org.openstreetmap.josm.data.osm.OsmPrimitive;
 import org.openstreetmap.josm.data.osm.Relation;
@@ -406 +405 @@
      * builds the panel with the OK and the Cancel button
      * @param okAction OK action
+     * @param deleteAction  Delete action
      * @param cancelAction Cancel action
      *
@@ -422 +422 @@
         // Keep users from saving invalid relations -- a relation MUST have at least a tag with the key "type"
         // AND must contain at least one other OSM object.
-        final TableModelListener listener = l -> updateOkPanel(this.actionAccess.getChangedRelation(), okButton, deleteButton);
+        final TableModelListener listener = l -> updateOkPanel(okButton, deleteButton);
         listener.tableChanged(null);
         this.memberTableModel.addTableModelListener(listener);
@@ -430 +430 @@
 
     /**
-     * Update the OK panel area
-     * @param newRelation What the new relation would "look" like if it were to be saved now
+     * Update the OK panel area with a temporary relation that looks if it were to be saved now.
      * @param okButton The OK button
      * @param deleteButton The delete button
      */
-    private void updateOkPanel(IRelation<?> newRelation, JButton okButton, JButton deleteButton) {
-        okButton.setVisible(newRelation.isUseful() || this.getRelationSnapshot() == null);
-        deleteButton.setVisible(!newRelation.isUseful() && this.getRelationSnapshot() != null);
-        if (this.getRelationSnapshot() == null && !newRelation.isUseful()) {
+    private void updateOkPanel(JButton okButton, JButton deleteButton) {
+        boolean useful = this.actionAccess.wouldRelationBeUseful();
+        okButton.setVisible(useful || this.getRelationSnapshot() == null);
+        deleteButton.setVisible(!useful && this.getRelationSnapshot() != null);
+        if (this.getRelationSnapshot() == null && !useful) {
             okButton.setText(tr("Delete"));
         } else {

trunk/src/org/openstreetmap/josm/gui/dialogs/relation/actions/ApplyAction.java

@@ -36 +36 @@
     @Override
     public void updateEnabledState() {
-        setEnabled(this.editorAccess.getChangedRelation().isUseful() && isEditorDirty());
+        setEnabled(this.editorAccess.wouldRelationBeUseful() && isEditorDirty());
     }
 }

trunk/src/org/openstreetmap/josm/gui/dialogs/relation/actions/CancelAction.java

@@ -9 +9 @@
 import javax.swing.RootPaneContainer;
 
-import org.openstreetmap.josm.data.osm.IRelation;
 import org.openstreetmap.josm.data.osm.Relation;
 import org.openstreetmap.josm.gui.HelpAwareOptionPane;
@@ -49 +48 @@
          && !(snapshot == null && getTagModel().getTags().isEmpty())) {
             //give the user a chance to save the changes
-            int ret = confirmClosingByCancel(this.editorAccess.getChangedRelation());
+            int ret = confirmClosingByCancel(this.editorAccess.wouldRelationBeUseful());
             if (ret == 0) { //Yes, save the changes
                 //copied from OKAction.run()
@@ -62 +61 @@
     }
 
-    protected int confirmClosingByCancel(final IRelation<?> newRelation) {
+    protected int confirmClosingByCancel(boolean isUseful) {
         ButtonSpec[] options = {
                 new ButtonSpec(
@@ -86 +85 @@
         // Keep users from saving invalid relations -- a relation MUST have at least a tag with the key "type"
         // AND must contain at least one other OSM object.
-        options[0].setEnabled(newRelation.isUseful());
+        options[0].setEnabled(isUseful);
 
         return HelpAwareOptionPane.showOptionDialog(

trunk/src/org/openstreetmap/josm/gui/dialogs/relation/actions/IRelationEditorActionAccess.java

@@ -72 +72 @@
     /**
      * Get the changed relation
-     * @return The changed relation (note: will not be part of a dataset). This should never be {@code null}.
+     * @return The changed relation (note: will not be part of a dataset) or the
+     * value returned by {@code getEditor().getRelation()}. This should never be {@code null}.
+     * If called for a temporary use of the relation instance, make sure to cleanup a copy to avoid memory leaks, see #23527
      * @since 18413
      */
@@ -98 +100 @@
 
     /**
+     * Check if the changed relation would be useful.
+     * @return true if the saved relation has at least one tag and one member
+     * @since 19014
+     */
+    default boolean wouldRelationBeUseful() {
+        return (getTagModel().getRowCount() > 0 && getMemberTableModel().getRowCount() > 0);
+    }
+
+    /**
      * Get the text field that is used to edit the role.
      * @return The role text field.

trunk/test/unit/org/openstreetmap/josm/gui/dialogs/relation/actions/RelationEditorActionsTest.java

@@ -17 +17 @@
 import org.openstreetmap.josm.data.coor.LatLon;
 import org.openstreetmap.josm.data.osm.DataSet;
+import org.openstreetmap.josm.data.osm.IRelation;
 import org.openstreetmap.josm.data.osm.Node;
 import org.openstreetmap.josm.data.osm.Relation;
@@ -169 +170 @@
         relationEditorAccess.getTagModel().initFromPrimitive(relation);
         relationEditorAccess.getEditor().reloadDataFromRelation();
-        assertDoesNotThrow(relationEditorAccess::getChangedRelation);
+
+        assertDoesNotThrow(() -> {
+            IRelation<?> tempRelation = relationEditorAccess.getChangedRelation();
+            if (tempRelation.getDataSet() == null)
+                tempRelation.setMembers(null); // see #19885
+        });
     }
 }