Fix StringName leaks in VariantParser #83619
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
akien-mga
approved these changes
Oct 19, 2023
| @@ -1075,7 +1075,7 @@ Error VariantParser::parse_value(Token &token, Variant &value, Stream *p_stream, | |||
| return ERR_PARSE_ERROR; | |||
| } | |||
|
|
|||
| static HashMap<StringName, Variant::Type> builtin_types; | |||
| static HashMap<String, Variant::Type> builtin_types; | |||
There was a problem hiding this comment.
For the record, I checked and confirmed that token.value used below should indeed be a String when token.type is TK_IDENTIFIER.
|
Thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
A follow-up to #83562.
Out of remaining 38 strings, 37 were variant types, and I was able to pinpoint them to #69248. It creates a static hashmap in the method's scope, which behaves the same as all other cases addressed in #83562. I'm not sure if there is any reason to store those identifiers as string names. They are returned as strings, they are compared to strings. It's just a lot conversion all around.
But I may be wrong, so feel free to suggest alternatives!
I also noticed that in one of the WebRTC classes there was a situation similar to #74031. Seems like Fabio used the same pattern in both places, so it was worth addressing it.
This still leaves one reported orphan string:
Unfortunately, this is a very hard issue to figure out. I can't deduce what could be keeping a reference to something so generic as a string of
"Node". There is nothing obvious related to static references, be it ofStringNameor ofHashMapsorHashSets, nor related toSNAME. The variant parser issue was created by a cast from a variable, and here it can be the same case. So using a literal to locate the offending code is impossible, and debugging through everything that ever accesses"Node"as a string name during a test cycle of opening a project doesn't paint a clear picture.So I leave it at that. I also tried to improve messages a bit so it may be easier to debug these leaks, if they are even real leaks.