Fix modification of map while iterating in SimpleJsonFormat

Fix modification of map while iterating in SimpleJsonFormat

Committed: https://code.google.com/p/dart/source/detail?r=18527

[Alan Knight, 2013-02-13 00:04:55]

Previous CL introduced some flakiness due to modifying a map while it was being
iterated over in the SimpleJsonFormat. Made me look seriously at not worrying
about the extra allocations and rewriting it to be more functional style, but
sadly it seems infeasible, or at least more work than finding the bug.

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

https://codereview.chromium.org/12210151/diff/1/pkg/serialization/lib/src/for...
File pkg/serialization/lib/src/format.dart (right):

https://codereview.chromium.org/12210151/diff/1/pkg/serialization/lib/src/for...
pkg/serialization/lib/src/format.dart:154: * to turn References into a nested
List/Map. If it's a Map, and the key
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 :)

https://codereview.chromium.org/12210151/diff/1/pkg/serialization/lib/src/for...
pkg/serialization/lib/src/format.dart:164: if (key != newKey)
keysToRemove.add(key);
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:30]

PTAL

https://codereview.chromium.org/12210151/diff/1/pkg/serialization/lib/src/for...
File pkg/serialization/lib/src/format.dart (right):

https://codereview.chromium.org/12210151/diff/1/pkg/serialization/lib/src/for...
pkg/serialization/lib/src/format.dart:154: * to turn References into a nested
List/Map. If it's a Map, and the key
On 2013/02/13 02:07:41, John Messerly wrote:
> 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 :)

Done.

https://codereview.chromium.org/12210151/diff/1/pkg/serialization/lib/src/for...
pkg/serialization/lib/src/format.dart:164: if (key != newKey)
keysToRemove.add(key);
On 2013/02/13 02:07:41, John Messerly wrote:
> 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)

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.

[Jennifer Messerly, 2013-02-13 23:16:53]

lgtm