Fix LinkedTreeMap being used for non-Comparable keys

[Marcono1234]

Fixes #1247

As shown in #1247 there are some corner cases where Gson chooses a LinkedTreeMap as Map implementation, even though the map keys are null or do not implement Comparable. This leads to runtime exceptions.

(I have also included a small documentation enhancement for JsonObject since it internally uses LinkedTreeMap and therefore does not support null keys either. That is already covered by existing tests.)

[Marcono1234]

Maybe this should also be changed to check the resolved type arguments (in case of subtypes with a different count of type parameters), though the underlying issue is #1708, and LinkedTreeMap should only be used if Map is requested and not for any subtypes. And in that case there is no need to resolve type arguments.

[Marcono1234]

This would prevent LinkedTreeMap from being used for raw map types, which should be invisible to the user (since LnkedTreeMap is an internal class) but would disable the DoS protection in that case for Java 7. Not sure if that would be acceptable, but on the other hand it prevents potential ClassCastExceptions.

[eamonnmcmanus]

This is kind of a big change, isn't it? It seems to me that gson.fromJson(s, Map.class) is a pretty common idiom and we're changing what it does. Agreed that people shouldn't be relying on getting a LinkedTreeMap, but is it something they could observe indirectly? For example because of memory usage. Though it looks to me as if LinkedTreeMap is fairly expensive in memory terms because of all those fields in its Node class.

I see some other options. I think the whole situation is a bit of a mess to be honest, but here are some ways we might be able to clean it up a bit:

1. Keep the approach here: raw Map never deserializes into a LinkedTreeMap, but some parameterized Type instances will. (We can work on adjusting exactly which ones.)
2. Never deserialize into LinkedTreeMap here. We would still use the class for JsonObject but here we would always use LinkedHashMap.
3. Give LinkedTreeMap a fallback mode, where if a key is added that is not Comparable, or that can't be compared to existing keys, it copies the existing entries into a private LinkedHashMap and uses that for all subsequent operations.

I suppose my underlying question is why LinkedTreeMap exists in the first place. What problem does it solve? It seems to me that it is usually going to be both bigger and less performant than LinkedHashMap, though I haven't measured that.

[Marcono1234]

Below where this method hasStringKeyType is called, I switched the if statement branches, which is why it might be a bit confusing.

I think the intended logic here is:
If it is a Map<String, ...> use Gson's LinkedTreeMap to protect against potential DoS for JDK's LinkedHashMap in Java < 8 (JDK-8046170). Otherwise use JDK's LinkedHashMap.

The check below was previously (roughly):

if (type instanceof ParameterizedType && !(String.class.isAssignableFrom(
          TypeToken.get(((ParameterizedType) type).getActualTypeArguments()[0]).getRawType()))) {
    ...
} else {
    return new LinkedTreeMap<>();
}

For raw Map.class and for any parameterized type Map<String, ...> it would have used LinkedTreeMap.
However, using LinkedTreeMap for raw Map.class can lead to ClassCastExceptions, as shown by the examples in #1247.

Therefore the changes by this PR actually reduce the cases where LinkedTreeMap is used (which was the concern I wanted to express with my original comment here).
But if you think that is fine, then I guess we can leave it like this.

[eamonnmcmanus]

I know this corresponds to what the logic did before, but I don't think it's correct in general. For example:

public class Class1<T> extends HashMap<Integer, T> // Class1<String> would return true but should not
public class Class2<T> extends HashMap<String, T> // Class2<Integer> would return false but should not
public class Class3 extends HashMap<String, Integer> // ditto

Could we maybe use TypeToken to tell whether Map<String, ?> is assignable from mapType? It might need some adjustment to the logic for wildcards.

[Marcono1234]

That is what I mean with #2152 (comment) (unfortunately GitHub showed it as "outdated").
hasStringKeyType is really only intended for Map and not any subclasses, to determine whether a LinkedTreeMap should be used. The issue that it is also called for Map subtypes (as shown in your examples) is basically #1708, therefore I am not so keen to add any special logic here.

[eamonnmcmanus]

This is kind of a big change, isn't it? It seems to me that gson.fromJson(s, Map.class) is a pretty common idiom and we're changing what it does. Agreed that people shouldn't be relying on getting a LinkedTreeMap, but is it something they could observe indirectly? For example because of memory usage. Though it looks to me as if LinkedTreeMap is fairly expensive in memory terms because of all those fields in its Node class.

I see some other options. I think the whole situation is a bit of a mess to be honest, but here are some ways we might be able to clean it up a bit:

1. Keep the approach here: raw Map never deserializes into a LinkedTreeMap, but some parameterized Type instances will. (We can work on adjusting exactly which ones.)
2. Never deserialize into LinkedTreeMap here. We would still use the class for JsonObject but here we would always use LinkedHashMap.
3. Give LinkedTreeMap a fallback mode, where if a key is added that is not Comparable, or that can't be compared to existing keys, it copies the existing entries into a private LinkedHashMap and uses that for all subsequent operations.

I suppose my underlying question is why LinkedTreeMap exists in the first place. What problem does it solve? It seems to me that it is usually going to be both bigger and less performant than LinkedHashMap, though I haven't measured that.

[Marcono1234]

@eamonnmcmanus do you think this can be merged (see my responses to your comments above)? Or do you still want something to be changed?