Changeset 17362 in josm

Timestamp:

2020-11-26T16:39:04+01:00 (4 years ago)

Author:

GerdP

Message:

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

• fix memory leaks in SplitWayAction and SplitWayCommand
• fix (not yet used) counter of modified relations

Location:

trunk/src/org/openstreetmap/josm

Files:

• actions/SplitWayAction.java (modified) (2 diffs)
• command/SplitWayCommand.java (modified) (8 diffs)

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

@@ -223 +223 @@
                 setHighlightedWaySegments(Collections.emptyList());
                 DISPLAY_COUNT.decrementAndGet();
+                if (getValue() != 1) {
+                    newWays.forEach(w -> w.setNodes(null)); // see 19885
+                }
             }
         }
@@ -304 +307 @@
             }
         });
+        if (!splitWayCommand.isPresent()) {
+            newWays.forEach(w -> w.setNodes(null)); // see 19885
+        }
     }
 

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

@@ -24 +24 @@
 import java.util.Set;
 import java.util.function.Consumer;
+import java.util.stream.Collectors;
 
 import javax.swing.JOptionPane;
@@ -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,
@@ -396 +397 @@
         }
 
-        switch (missingMemberStrategy) {
+        try {
+            switch (missingMemberStrategy) {
             case GO_AHEAD_WITH_DOWNLOADS:
                 try {
@@ -406 +408 @@
                 // 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();
+            }
+            return Optional.of(splitBasedOnAnalyses(way, newWays, newSelection, analysis, indexOfWayToKeep));
+        } finally {
+            // see #19885
+            wayToKeep.setNodes(null);
+            analysis.cleanup();
         }
     }
@@ -443 +452 @@
             }
 
-            numberOfRelations++;
+            boolean isSimpleCase = true;
 
             Relation c = null;
@@ -549 +558 @@
                         }
                         relationAnalyses.add(new RelationAnalysis(c, rm, direction, missingWays));
+                        isSimpleCase = false;
                     }
                 }
             }
-
-            if (c != null) {
-                commandList.add(new ChangeCommand(r.getDataSet(), r, c));
+            if (!isSimpleCase)
+                numberOfRelations++;
+            if (c != null && isSimpleCase) {
+                if (!r.getMembers().equals(c.getMembers())) {
+                    commandList.add(new ChangeMembersCommand(r, new ArrayList<>(c.getMembers())));
+                    numberOfRelations++;
+                }
+                c.setMembers(null); // see #19885
             }
         }
@@ -575 +590 @@
             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);
+            }
         }
 
@@ -729 +754 @@
                 }
             }
+        }
+
+        // add one command for each complex case with relations
+        final DataSet ds = way.getDataSet();
+        for (Relation r : analysis.getRelationAnalyses().stream().map(RelationAnalysis::getRelation).collect(Collectors.toSet())) {
+            Relation orig = (Relation) ds.getPrimitiveById(r);
+            analysis.getCommands().add(new ChangeMembersCommand(orig, new ArrayList<>(r.getMembers())));
+            r.setMembers(null); // see #19885
         }