Changeset 17358 in josm

Timestamp:

2020-11-25T11:50:22+01:00 (4 years ago)

Author:

GerdP

Message:

see #19885: memory leak with "temporary" objects in validator and actions

• (hopefully) fix memory leaks in complex actions
• handle complex cases with presets and RelationEditor

I hope these changes don't break plugins which extend or overwrite RelationEditor

Location:

trunk/src/org/openstreetmap/josm

Files:

• actions/CreateMultipolygonAction.java (modified) (9 diffs)
• actions/JoinAreasAction.java (modified) (5 diffs)
• actions/ReverseWayAction.java (modified) (4 diffs)
• command/ChangePropertyCommand.java (modified) (5 diffs)
• command/SplitWayCommand.java (modified) (8 diffs)
• data/osm/MultipolygonBuilder.java (modified) (1 diff)
• gui/dialogs/properties/PropertiesDialog.java (modified) (1 diff)
• gui/dialogs/relation/GenericRelationEditor.java (modified) (2 diffs)
• gui/dialogs/relation/RelationEditor.java (modified) (3 diffs)
• gui/dialogs/relation/actions/SavingAction.java (modified) (3 diffs)

trunk/src/org/openstreetmap/josm/actions/CreateMultipolygonAction.java

@@ -26 +26 @@
 import org.openstreetmap.josm.actions.relation.DownloadSelectedIncompleteMembersAction;
 import org.openstreetmap.josm.command.AddCommand;
-import org.openstreetmap.josm.command.ChangeCommand;
+import org.openstreetmap.josm.command.ChangeMembersCommand;
 import org.openstreetmap.josm.command.ChangePropertyCommand;
 import org.openstreetmap.josm.command.Command;
@@ -116 +116 @@
             // to avoid EDT violations
             SwingUtilities.invokeLater(() -> {
+                if (multipolygonRelation != null) {
+                    // rather ugly: update generated a ChangeMembersCommand with a copy of the member list, so clear the list now
+                    commandAndRelation.b.setMembers(null); // #see 19885
+                }
                 UndoRedoHandler.getInstance().add(command);
                 final Relation relation = (Relation) MainApplication.getLayerManager().getEditDataSet()
@@ -229 +233 @@
      * @param selectedWays selected ways
      * @param selectedMultipolygonRelation selected multipolygon relation
-     * @return pair of old and new multipolygon relation if a difference was found, else the pair contains the old relation twice
+     * @return null if ways don't build a valid multipolygon, pair of old and new multipolygon relation if a difference was found,
+     * else the pair contains the old relation twice
      */
     public static Pair<Relation, Relation> updateMultipolygonRelation(Collection<Way> selectedWays, Relation selectedMultipolygonRelation) {
@@ -244 +249 @@
         }
         showErrors(mpTest.getErrors());
+        calculated.setMembers(null); // see #19885
         return null; //could not make multipolygon.
     }
@@ -280 +286 @@
         }
         foundDiff |= merged.addAll(calculated.getMembers());
+        calculated.setMembers(null); // see #19885
         if (!foundDiff) {
             return Pair.create(old, old); // unchanged
@@ -343 +350 @@
         } else {
             if (!unchanged) {
-                list.add(new ChangeCommand(existingRelation, relation));
+                list.add(new ChangeMembersCommand(existingRelation, new ArrayList<>(relation.getMembers())));
             }
             if (list.isEmpty()) {
@@ -390 +397 @@
 
     /**
-     * This method removes tags/value pairs from inner and outer ways and put them on relation if necessary
+     * This method removes tags/value pairs from inner and outer ways and put them on relation if necessary.
      * Function was extended in reltoolbox plugin by Zverikk and copied back to the core
-     * @param relation the multipolygon style relation to process
+     * @param relation the multipolygon style relation to process. If it is new, the tags might be
+     * modified, else the list of commands will contain a command to modify its tags
      * @return a list of commands to execute
      */
@@ -472 +480 @@
         }
 
-        if (moveTags) {
+        values.remove("area");
+        if (moveTags && !values.isEmpty()) {
             // add those tag values to the relation
             boolean fixed = false;
-            Relation r2 = new Relation(relation);
+            Map<String, String> tagsToAdd = new HashMap<>();
             for (Entry<String, String> entry : values.entrySet()) {
                 String key = entry.getKey();
-                if (!r2.hasKey(key) && !"area".equals(key)) {
+                if (!relation.hasKey(key)) {
                     if (relation.isNew())
                         relation.put(key, entry.getValue());
                     else
-                        r2.put(key, entry.getValue());
+                        tagsToAdd.put(key, entry.getValue());
                     fixed = true;
                 }
@@ -491 +500 @@
                     ds = MainApplication.getLayerManager().getEditDataSet();
                 }
-                commands.add(new ChangeCommand(ds, relation, r2));
-            } else {
-                r2.setMembers(null); // see #19885
+                commands.add(new ChangePropertyCommand(ds, Collections.singleton(relation), tagsToAdd));
             }
         }

trunk/src/org/openstreetmap/josm/actions/JoinAreasAction.java

@@ -26 +26 @@
 import org.openstreetmap.josm.actions.ReverseWayAction.ReverseWayResult;
 import org.openstreetmap.josm.command.AddCommand;
-import org.openstreetmap.josm.command.ChangeCommand;
+import org.openstreetmap.josm.command.ChangeMembersCommand;
 import org.openstreetmap.josm.command.ChangeNodesCommand;
+import org.openstreetmap.josm.command.ChangePropertyCommand;
 import org.openstreetmap.josm.command.Command;
 import org.openstreetmap.josm.command.DeleteCommand;
@@ -1634 +1635 @@
                 }
 
-                Relation newRel = new Relation(r);
-                List<RelationMember> members = newRel.getMembers();
+                List<RelationMember> members = r.getMembers();
                 members.remove(rm);
-                newRel.setMembers(members);
-
-                cmds.add(new ChangeCommand(r, newRel));
+
+                cmds.add(new ChangeMembersCommand(r, members));
                 RelationRole saverel = new RelationRole(r, rm.getRole());
                 if (!result.contains(saverel)) {
@@ -1674 +1673 @@
             }
             // Add it back!
-            Relation newRel = new Relation(r.rel);
-            newRel.addMember(new RelationMember(r.role, outer));
-            cmds.add(new ChangeCommand(r.rel, newRel));
-        }
-
-        Relation newRel;
-        RelationRole soleOuter;
+            List<RelationMember> modifiedMembers = new ArrayList<>(r.rel.getMembers());
+            modifiedMembers.add(new RelationMember(r.role, outer));
+            cmds.add(new ChangeMembersCommand(r.rel, modifiedMembers));
+        }
+
         switch (multiouters.size()) {
         case 0:
@@ -1686 +1683 @@
         case 1:
             // Found only one to be part of a multipolygon relation, so just add it back as well
-            soleOuter = multiouters.get(0);
-            newRel = new Relation(soleOuter.rel);
-            newRel.addMember(new RelationMember(soleOuter.role, outer));
-            cmds.add(new ChangeCommand(ds, soleOuter.rel, newRel));
+            RelationRole soleOuter = multiouters.get(0);
+            List<RelationMember> modifiedMembers = new ArrayList<>(soleOuter.rel.getMembers());
+            modifiedMembers.add(new RelationMember(soleOuter.role, outer));
+            cmds.add(new ChangeMembersCommand(ds, soleOuter.rel, modifiedMembers));
             return;
         default:
             // Create a new relation with all previous members and (Way)outer as outer.
-            newRel = new Relation();
+            Relation newRel = new Relation();
             for (RelationRole r : multiouters) {
                 // Add members
@@ -1718 +1715 @@
      */
     private void stripTags(Collection<Way> ways) {
-        for (Way w : ways) {
-            final Way wayWithoutTags = new Way(w);
-            wayWithoutTags.removeAll();
-            cmds.add(new ChangeCommand(w, wayWithoutTags));
-        }
+        Map<String, String> tagsToRemove = new HashMap<>();
+        ways.stream().flatMap(w -> w.keySet().stream()).forEach(k -> tagsToRemove.put(k, null));
+        if (tagsToRemove.isEmpty())
+            return;
+        cmds.add(new ChangePropertyCommand(new ArrayList<>(ways), tagsToRemove));
         /* I18N: current action printed in status display */
         commitCommands(marktr("Remove tags from inner ways"));

trunk/src/org/openstreetmap/josm/actions/ReverseWayAction.java

@@ -18 +18 @@
 import org.openstreetmap.josm.actions.corrector.ReverseWayNoTagCorrector;
 import org.openstreetmap.josm.actions.corrector.ReverseWayTagCorrector;
-import org.openstreetmap.josm.command.ChangeCommand;
+import org.openstreetmap.josm.command.ChangeNodesCommand;
 import org.openstreetmap.josm.command.Command;
 import org.openstreetmap.josm.command.SequenceCommand;
@@ -41 +41 @@
      */
     public static class ReverseWayResult {
-        private final Way newWay;
         private final Collection<Command> tagCorrectionCommands;
         private final Command reverseCommand;
@@ -47 +46 @@
         /**
          * Create a new {@link ReverseWayResult}
-         * @param newWay The new way primitive
          * @param tagCorrectionCommands The commands to correct the tags
          * @param reverseCommand The command to reverse the way
          */
-        public ReverseWayResult(Way newWay, Collection<Command> tagCorrectionCommands, Command reverseCommand) {
-            this.newWay = newWay;
+        public ReverseWayResult(Collection<Command> tagCorrectionCommands, Command reverseCommand) {
             this.tagCorrectionCommands = tagCorrectionCommands;
             this.reverseCommand = reverseCommand;
-        }
-
-        /**
-         * Gets the new way object
-         * @return The new, reversed way
-         */
-        public Way getNewWay() {
-            return newWay;
         }
 
@@ -150 +139 @@
     public static ReverseWayResult reverseWay(Way w) throws UserCancelException {
         ReverseWayNoTagCorrector.checkAndConfirmReverseWay(w);
-        Way wnew = new Way(w);
-        List<Node> nodesCopy = wnew.getNodes();
+        List<Node> nodesCopy = w.getNodes();
         Collections.reverse(nodesCopy);
-        wnew.setNodes(nodesCopy);
 
         Collection<Command> corrCmds = Collections.<Command>emptyList();
         if (Config.getPref().getBoolean("tag-correction.reverse-way", true)) {
-            corrCmds = new ReverseWayTagCorrector().execute(w, wnew);
+            corrCmds = new ReverseWayTagCorrector().execute(w, w);
         }
-        return new ReverseWayResult(wnew, corrCmds, new ChangeCommand(w, wnew));
+        return new ReverseWayResult(corrCmds, new ChangeNodesCommand(w, new ArrayList<>(nodesCopy)));
     }
 

trunk/src/org/openstreetmap/josm/command/ChangePropertyCommand.java

@@ -8 +8 @@
 import java.util.Collection;
 import java.util.Collections;
+import java.util.HashMap;
 import java.util.LinkedList;
 import java.util.List;
 import java.util.Map;
+import java.util.Map.Entry;
 import java.util.NoSuchElementException;
 import java.util.Objects;
@@ -21 +23 @@
 import org.openstreetmap.josm.data.osm.OsmPrimitive;
 import org.openstreetmap.josm.data.osm.OsmPrimitiveType;
+import org.openstreetmap.josm.data.osm.Tagged;
 import org.openstreetmap.josm.tools.I18n;
 import org.openstreetmap.josm.tools.ImageProvider;
@@ -73 +76 @@
      * @param ds The target data set. Must not be {@code null}
      * @param objects the objects to modify. Must not be empty
-     * @param tags the tags to set
+     * @param tags the tags to set. Caller must make sure that the tas are not changed once the command was executed.
      * @since 12726
      */
@@ -86 +89 @@
      *
      * @param objects the objects to modify. Must not be empty, and objects must belong to a data set
-     * @param tags the tags to set
+     * @param tags the tags to set. Caller must make sure that the tas are not changed once the command was executed.
      * @throws NullPointerException if objects is null or contain null item
      * @throws NoSuchElementException if objects is empty
@@ -287 +290 @@
                 Objects.equals(tags, that.tags);
     }
+
+    /**
+     * Calculate the {@link ChangePropertyCommand} that is needed to change the tags in source to be equal to those in target.
+     * @param source the source primitive
+     * @param target the target primitive
+     * @return null if no changes are needed, else a {@link ChangePropertyCommand}
+     * @since 17357
+     */
+    public static Command build(OsmPrimitive source, Tagged target) {
+        Map<String, String> changedTags = new HashMap<>();
+        // find tags which have to be changed or removed
+        for (Entry<String, String> tag : source.getKeys().entrySet()) {
+            String key = tag.getKey();
+            String val = target.get(key);
+            if (!tag.getValue().equals(val))
+                changedTags.put(key, val); // null or a different value
+        }
+        // find tags which exist only in target, they have to be added
+        for (Entry<String, String> tag : target.getKeys().entrySet()) {
+            String key = tag.getKey();
+            if (!source.hasTag(key))
+                changedTags.put(key, tag.getValue());
+        }
+        if (changedTags.isEmpty())
+            return null;
+        if (changedTags.size() == 1) {
+            Entry<String, String> tag = changedTags.entrySet().iterator().next();
+            return new ChangePropertyCommand(Collections.singleton(source), tag.getKey(), tag.getValue());
+        }
+        return new ChangePropertyCommand(Collections.singleton(source), new HashMap<>(changedTags));
+    }
 }

trunk/src/org/openstreetmap/josm/command/SplitWayCommand.java

@@ -17 +17 @@
 import java.util.HashSet;
 import java.util.Iterator;
+import java.util.LinkedHashSet;
 import java.util.LinkedList;
 import java.util.List;
@@ -90 +91 @@
      * @param newSelection The new list of selected primitives ids (which is saved for later retrieval with {@link #getNewSelection})
      * @param originalWay The original way being split (which is saved for later retrieval with {@link #getOriginalWay})
-     * @param newWays The resulting new ways (which is saved for later retrieval with {@link #getOriginalWay})
+     * @param newWays The resulting new ways (which is saved for later retrieval with {@link #getNewWays})
      */
     public SplitWayCommand(String name, Collection<Command> commandList,
@@ -402 +403 @@
                 } catch (OsmTransferException e) {
                     ExceptionDialogUtil.explainException(e);
+                    analysis.cleanup();
                     return Optional.empty();
                 }
                 // If missing relation members were downloaded, perform the analysis again to find the relation
                 // member order for all relations.
+                analysis.cleanup();
                 analysis = analyseSplit(way, wayToKeep, newWays);
-                return Optional.of(splitBasedOnAnalyses(way, newWays, newSelection, analysis, indexOfWayToKeep));
+                break;
             case GO_AHEAD_WITHOUT_DOWNLOADS:
                 // Proceed with the split with the information we have.
                 // This can mean that there are no missing members we want, or that the user chooses to continue
                 // the split without downloading them.
-                return Optional.of(splitBasedOnAnalyses(way, newWays, newSelection, analysis, indexOfWayToKeep));
+                break;
             case USER_ABORTED:
             default:
                 return Optional.empty();
+        }
+        try {
+            return Optional.of(splitBasedOnAnalyses(way, newWays, newSelection, analysis, indexOfWayToKeep));
+        } finally {
+            // see #19885
+            wayToKeep.setNodes(null);
+            analysis.cleanup();
         }
     }
@@ -465 +475 @@
                     }
                     if (c == null) {
-                        c = new Relation(r);
+                        c = new Relation(r); // #19885: will be removed later
                     }
 
@@ -552 +562 @@
                 }
             }
-
-            if (c != null) {
-                commandList.add(new ChangeCommand(r.getDataSet(), r, c));
-            }
         }
         changedWay.setNodes(null); // see #19885
@@ -575 +581 @@
             warningTypes = warnings;
             this.numberOfRelations = numberOfRelations;
+        }
+
+        /**
+         * Unlink temporary copies of relations. See #19885
+         */
+        void cleanup() {
+            for (RelationAnalysis ra : relationAnalyses) {
+                if (ra.relation.getDataSet() == null)
+                    ra.relation.setMembers(null);
+            }
         }
 
@@ -688 +704 @@
         }
 
+        Set<Relation> modifiedRelations = new LinkedHashSet<>();
         // Perform the split.
         for (RelationAnalysis relationAnalysis : analysis.getRelationAnalyses()) {
@@ -729 +746 @@
                 }
             }
+            modifiedRelations.add(relation);
+        }
+        for (Relation r : modifiedRelations) {
+            DataSet ds = way.getDataSet();
+            Relation orig = (Relation) ds.getPrimitiveById(r);
+            analysis.getCommands().add(new ChangeMembersCommand(orig, new ArrayList<>(r.getMembers())));
+            r.setMembers(null); // see #19885
         }
 

trunk/src/org/openstreetmap/josm/data/osm/MultipolygonBuilder.java

@@ -118 +118 @@
         MultipolygonTest mpTest = new MultipolygonTest();
         Relation calculated = mpTest.makeFromWays(ways);
-        if (!mpTest.getErrors().isEmpty()) {
-            return mpTest.getErrors().iterator().next().getMessage();
-        }
-        Pair<List<JoinedPolygon>, List<JoinedPolygon>> outerInner = joinWays(calculated);
-        this.outerWays.clear();
-        this.innerWays.clear();
-        this.outerWays.addAll(outerInner.a);
-        this.innerWays.addAll(outerInner.b);
-        return null;
+        try {
+            if (!mpTest.getErrors().isEmpty()) {
+                return mpTest.getErrors().iterator().next().getMessage();
+            }
+            Pair<List<JoinedPolygon>, List<JoinedPolygon>> outerInner = joinWays(calculated);
+            this.outerWays.clear();
+            this.innerWays.clear();
+            this.outerWays.addAll(outerInner.a);
+            this.innerWays.addAll(outerInner.b);
+            return null;
+        } finally {
+            calculated.setMembers(null); // see #19885
+        }
     }
 

trunk/src/org/openstreetmap/josm/gui/dialogs/properties/PropertiesDialog.java

@@ -1154 +1154 @@
                 return;
 
-            Relation copy = new Relation(cur);
+            List<RelationMember> members = cur.getMembers();
             for (OsmPrimitive primitive: OsmDataManager.getInstance().getInProgressSelection()) {
-                copy.removeMembersFor(primitive);
-            }
-            UndoRedoHandler.getInstance().add(new ChangeMembersCommand(cur, copy.getMembers()));
-            copy.setMembers(null); // see #19885
+                members.removeIf(rm -> rm.getMember() == primitive);
+            }
+            UndoRedoHandler.getInstance().add(new ChangeMembersCommand(cur, members));
 
             tagTable.clearSelection();

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

@@ -49 +49 @@
 
 import org.openstreetmap.josm.actions.JosmAction;
-import org.openstreetmap.josm.command.ChangeCommand;
+import org.openstreetmap.josm.command.ChangeMembersCommand;
 import org.openstreetmap.josm.command.Command;
 import org.openstreetmap.josm.data.UndoRedoHandler;
@@ -939 +939 @@
             final Collection<TaggingPreset> presets = TaggingPresets.getMatchingPresets(
                     EnumSet.of(TaggingPresetType.forPrimitive(orig)), orig.getKeys(), false);
-            Relation relation = new Relation(orig);
+            Relation target = new Relation(orig);
             boolean modified = false;
             for (OsmPrimitive p : primitivesToAdd) {
                 if (p instanceof Relation) {
-                    List<Relation> loop = RelationChecker.checkAddMember(relation, (Relation) p);
+                    List<Relation> loop = RelationChecker.checkAddMember(target, (Relation) p);
                     if (!loop.isEmpty() && loop.get(0).equals(loop.get(loop.size() - 1))) {
                         warnOfCircularReferences(p, loop);
                         continue;
                     }
-                } else if (MemberTableModel.hasMembersReferringTo(relation.getMembers(), Collections.singleton(p))
+                } else if (MemberTableModel.hasMembersReferringTo(target.getMembers(), Collections.singleton(p))
                         && !confirmAddingPrimitive(p)) {
                     continue;
                 }
                 final Set<String> roles = findSuggestedRoles(presets, p);
-                relation.addMember(new RelationMember(roles.size() == 1 ? roles.iterator().next() : "", p));
+                target.addMember(new RelationMember(roles.size() == 1 ? roles.iterator().next() : "", p));
                 modified = true;
             }
-            if (!modified) {
-                relation.setMembers(null); // see #19885
-            }
-            return modified ? new ChangeCommand(orig, relation) : null;
+            List<RelationMember> members = new ArrayList<>(target.getMembers());
+            target.setMembers(null); // see #19885
+            return modified ? new ChangeMembersCommand(orig, members) : null;
         } catch (AddAbortException ign) {
             Logging.trace(ign);

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

@@ -14 +14 @@
 import org.openstreetmap.josm.gui.layer.OsmDataLayer;
 import org.openstreetmap.josm.tools.CheckParameterUtil;
+import org.openstreetmap.josm.tools.Logging;
 
 /**
@@ -71 +72 @@
      *
      * @param layer the data layer the relation is a member of
-     * @param r the relation to be edited
+     * @param r the relation to be edited. If the relation doesn't belong to a {@code DataSet}
+     * callers MUST NOT use the relation after calling this method.
      * @param selectedMembers a collection of relation members which shall be selected when the editor is first launched
      * @return an instance of RelationEditor suitable for editing that kind of relation
@@ -80 +82 @@
         else {
             RelationEditor editor = new GenericRelationEditor(layer, r, selectedMembers);
+            if (r != null && r.getDataSet() == null) {
+                // see #19885: We have to assume that the relation was only created as container for tags and members
+                // the editor has created its own copy by now.
+                // Since the members point to the container we unlink them here.
+                Logging.debug("Member list is reset for relation  {0}", r.getUniqueId());
+                r.setMembers(null);
+            }
+
             RelationDialogManager.getRelationDialogManager().positionOnScreen(editor);
             RelationDialogManager.getRelationDialogManager().register(layer, r, editor);

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

@@ -5 +5 @@
 
 import java.awt.Component;
+import java.util.ArrayList;
+import java.util.List;
 
 import javax.swing.JOptionPane;
@@ -11 +13 @@
 import org.openstreetmap.josm.command.AddCommand;
 import org.openstreetmap.josm.command.ChangeCommand;
+import org.openstreetmap.josm.command.ChangeMembersCommand;
+import org.openstreetmap.josm.command.ChangePropertyCommand;
+import org.openstreetmap.josm.command.Command;
 import org.openstreetmap.josm.command.conflict.ConflictAddCommand;
 import org.openstreetmap.josm.data.UndoRedoHandler;
@@ -86 +91 @@
         tagEditorModel.applyToPrimitive(editedRelation);
         getMemberTableModel().applyToRelation(editedRelation);
-        if (!editedRelation.hasEqualSemanticAttributes(originRelation, false)) {
+        List<Command> cmds = new ArrayList<>();
+        if (originRelation.getKeys().equals(editedRelation.getKeys())) {
+            cmds.add(new ChangeMembersCommand(originRelation, editedRelation.getMembers()));
+        }
+        Command cmdProps = ChangePropertyCommand.build(originRelation, editedRelation);
+        if (cmdProps != null)
+            cmds.add(cmdProps);
+        if (cmds.size() >= 2) {
             UndoRedoHandler.getInstance().add(new ChangeCommand(originRelation, editedRelation));
-        } else {
+        } else if (!cmds.isEmpty()) {
+            UndoRedoHandler.getInstance().add(cmds.get(0));
             editedRelation.setMembers(null); // see #19885
         }