Fix modification of map while iterating in SimpleJsonFormat

pkg/serialization/lib/src/format.dart

 part of serialization;
 
 /**
  * An abstract class for serialization formats. Subclasses define how data
  * is read or written to a particular output mechanism.
  */
 abstract class Format {
   /**
    * Return true if this format stores primitives in their own area and uses
    * references to them (e.g. [SimpleFlatFormat]) and false if primitives
@@ skipping 133 matching lines @@
         if (storeRoundTripInfo) ruleData[i].add(rule.number);
       } else if (each is Map) {
         jsonifyEntry(each, w);
         if (storeRoundTripInfo) each[RULE] = rule.number;
       }
     }
   }
 
   /**
    * For one particular entry, which is either a Map or a List, update it
-   * to turn References into a nested List/Map.
+   * to turn References into a nested List/Map. If it's a Map, and the key

[Jennifer Messerly, 2013/02/13 02:07:41]

Unless this is purely an internal type, I'd put this in a // comment inside the
method instead of doc comment. Users don't need to know the pains we go through
to implement the library :)

[Alan Knight, 2013/02/13 22:40:31]

Done.

+   * might be a reference, we need to bend over backwards to avoid
+   * concurrent modifications.
    */
   jsonifyEntry(map, Writer w) {
+    var keysToRemove = [];
+    var updates = new Map();
     keysAndValues(map).forEach((key, value) {
-      if (value is Reference) map[key] = w.stateForReference(value);
+      var newKey = (key is Reference) ? w.stateForReference(key) : key;
+      if (value is Reference) updates[newKey] = w.stateForReference(value);
+      if (key != newKey) keysToRemove.add(key);

[Jennifer Messerly, 2013/02/13 02:07:41]

We weren't removing keys before--is this also a bug fix?
Might be worth adding another test for this case (where the key is a Reference)

[Alan Knight, 2013/02/13 22:40:31]

I added that, because one of the symptoms was an unresolved Reference getting
through, but I believe the check isn't necessary, because that should never
happen. So I've removed it.

Non-primitive keys, which generate a reference, do arise already in the case of
the meta-serialization where the BasicRule does already use an empty array as a
field name. But I added a more explicit test case just to be on the safe side.

I'm pretty sure the underlying problem here is with iterating over a map and
replacing the values at keys not being concurrent-modification safe. When the
map is already at capacity, it can force a grow on the assumption it might need
to add a new element, and then the iteration fails.

     });
+    keysToRemove.forEach((each) => map.remove(each));
+    updates.forEach((k, v) => map[k] = v);
   }
 
   /**
    * Read serialized data saved in this format, which should look like
    * either a simple type, a List or a Map and return the Map
    * representation that the reader expects, with top-level
    * entries for "rules", "data", and "roots". Nested lists/maps will be
    * converted into Reference objects. Note that if the data was not written
    * with [storeRoundTripInfo] true this will fail.
    */
@@ skipping 286 matching lines @@
       return new Reference(r, a, b);
     }
   }
 
   /** Return the next element from the input. */
   _next(Iterator input) {
     input.moveNext();
     return input.current;
   }
 }