Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix leaks in GDScript (3.2) #41931

Merged
merged 2 commits into from
Sep 24, 2020
Merged

Fix leaks in GDScript (3.2) #41931

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a096966
Prevent cyclic reference between script and its members
RandomShaper Sep 9, 2020
8e64969
Ensure cyclic dependencies between scripts are broken at exit
RandomShaper Sep 9, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions modules/gdscript/gdscript.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2244,6 +2244,22 @@ GDScriptLanguage::~GDScriptLanguage() {
if (_call_stack) {
memdelete_arr(_call_stack);
}

// Clear dependencies between scripts, to ensure cyclic references are broken (to avoid leaks at exit).
while (script_list.first()) {
GDScript *script = script_list.first()->self();
for (Map<StringName, GDScriptFunction *>::Element *E = script->member_functions.front(); E; E = E->next()) {
GDScriptFunction *func = E->get();
for (int i = 0; i < func->argument_types.size(); i++) {
func->argument_types.write[i].script_type_ref = Ref<Script>();
}
func->return_type.script_type_ref = Ref<Script>();
}
for (Map<StringName, GDScript::MemberInfo>::Element *E = script->member_indices.front(); E; E = E->next()) {
E->get().data_type.script_type_ref = Ref<Script>();
}
}

singleton = NULL;
}

Expand Down
22 changes: 14 additions & 8 deletions modules/gdscript/gdscript_compiler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ bool GDScriptCompiler::_create_binary_operator(CodeGen &codegen, const GDScriptP
return true;
}

GDScriptDataType GDScriptCompiler::_gdtype_from_datatype(const GDScriptParser::DataType &p_datatype) const {
GDScriptDataType GDScriptCompiler::_gdtype_from_datatype(const GDScriptParser::DataType &p_datatype, GDScript *p_owner) const {
if (!p_datatype.has_type) {
return GDScriptDataType();
}
Expand All @@ -130,12 +130,12 @@ GDScriptDataType GDScriptCompiler::_gdtype_from_datatype(const GDScriptParser::D
} break;
case GDScriptParser::DataType::SCRIPT: {
result.kind = GDScriptDataType::SCRIPT;
result.script_type = p_datatype.script_type;
result.script_type = Ref<Script>(p_datatype.script_type).ptr();
result.native_type = result.script_type->get_instance_base_type();
} break;
case GDScriptParser::DataType::GDSCRIPT: {
result.kind = GDScriptDataType::GDSCRIPT;
result.script_type = p_datatype.script_type;
result.script_type = Ref<Script>(p_datatype.script_type).ptr();
result.native_type = result.script_type->get_instance_base_type();
} break;
case GDScriptParser::DataType::CLASS: {
Expand All @@ -159,7 +159,7 @@ GDScriptDataType GDScriptCompiler::_gdtype_from_datatype(const GDScriptParser::D
}

result.kind = GDScriptDataType::GDSCRIPT;
result.script_type = script;
result.script_type = Ref<Script>(script).ptr();
result.native_type = script->get_instance_base_type();
} break;
default: {
Expand All @@ -168,6 +168,12 @@ GDScriptDataType GDScriptCompiler::_gdtype_from_datatype(const GDScriptParser::D
}
}

// Only hold strong reference to the script if it's not the owner of the
// element qualified with this type, to avoid cyclic references (leaks).
if (result.script_type && result.script_type != p_owner) {
result.script_type_ref = Ref<Script>(result.script_type);
}

return result;
}

Expand Down Expand Up @@ -1684,9 +1690,9 @@ Error GDScriptCompiler::_parse_function(GDScript *p_script, const GDScriptParser
gdfunc->rpc_mode = p_func->rpc_mode;
gdfunc->argument_types.resize(p_func->argument_types.size());
for (int i = 0; i < p_func->argument_types.size(); i++) {
gdfunc->argument_types.write[i] = _gdtype_from_datatype(p_func->argument_types[i]);
gdfunc->argument_types.write[i] = _gdtype_from_datatype(p_func->argument_types[i], p_script);
}
gdfunc->return_type = _gdtype_from_datatype(p_func->return_type);
gdfunc->return_type = _gdtype_from_datatype(p_func->return_type, p_script);
} else {
gdfunc->_static = false;
gdfunc->rpc_mode = MultiplayerAPI::RPC_MODE_DISABLED;
Expand Down Expand Up @@ -1869,7 +1875,7 @@ Error GDScriptCompiler::_parse_class_level(GDScript *p_script, const GDScriptPar
p_script->native = native;
} break;
case GDScriptDataType::GDSCRIPT: {
Ref<GDScript> base = base_type.script_type;
Ref<GDScript> base = Ref<GDScript>(base_type.script_type);
p_script->base = base;
p_script->_base = base.ptr();
p_script->member_indices = base->member_indices;
Expand Down Expand Up @@ -1902,7 +1908,7 @@ Error GDScriptCompiler::_parse_class_level(GDScript *p_script, const GDScriptPar
minfo.setter = p_class->variables[i].setter;
minfo.getter = p_class->variables[i].getter;
minfo.rpc_mode = p_class->variables[i].rpc_mode;
minfo.data_type = _gdtype_from_datatype(p_class->variables[i].data_type);
minfo.data_type = _gdtype_from_datatype(p_class->variables[i].data_type, p_script);

PropertyInfo prop_info = minfo.data_type;
prop_info.name = name;
Expand Down
2 changes: 1 addition & 1 deletion modules/gdscript/gdscript_compiler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ class GDScriptCompiler {
bool _create_unary_operator(CodeGen &codegen, const GDScriptParser::OperatorNode *on, Variant::Operator op, int p_stack_level);
bool _create_binary_operator(CodeGen &codegen, const GDScriptParser::OperatorNode *on, Variant::Operator op, int p_stack_level, bool p_initializer = false, int p_index_addr = 0);

GDScriptDataType _gdtype_from_datatype(const GDScriptParser::DataType &p_datatype) const;
GDScriptDataType _gdtype_from_datatype(const GDScriptParser::DataType &p_datatype, GDScript *p_owner = NULL) const;

int _parse_assign_right_expression(CodeGen &codegen, const GDScriptParser::OperatorNode *p_expression, int p_stack_level, int p_index_addr = 0);
int _parse_expression(CodeGen &codegen, const GDScriptParser::Node *p_expression, int p_stack_level, bool p_root = false, bool p_initializer = false, int p_index_addr = 0);
Expand Down
2 changes: 1 addition & 1 deletion modules/gdscript/gdscript_editor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -644,7 +644,7 @@ static GDScriptCompletionIdentifier _type_from_gdtype(const GDScriptDataType &p_
ci.type.has_type = true;
ci.type.builtin_type = p_gdtype.builtin_type;
ci.type.native_type = p_gdtype.native_type;
ci.type.script_type = p_gdtype.script_type;
ci.type.script_type = Ref<Script>(p_gdtype.script_type);

switch (p_gdtype.kind) {
case GDScriptDataType::UNINITIALIZED: {
Expand Down
6 changes: 4 additions & 2 deletions modules/gdscript/gdscript_function.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,8 @@ struct GDScriptDataType {
} kind;
Variant::Type builtin_type;
StringName native_type;
Ref<Script> script_type;
Script *script_type;
Ref<Script> script_type_ref;

bool is_type(const Variant &p_variant, bool p_allow_implicit_conversion = false) const {
if (!has_type) return true; // Can't type check
Expand Down Expand Up @@ -149,7 +150,8 @@ struct GDScriptDataType {
GDScriptDataType() :
has_type(false),
kind(UNINITIALIZED),
builtin_type(Variant::NIL) {}
builtin_type(Variant::NIL),
script_type(NULL) {}
};

class GDScriptFunction {
Expand Down
2 changes: 1 addition & 1 deletion modules/gdscript/gdscript_parser.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6055,7 +6055,7 @@ GDScriptParser::DataType GDScriptParser::_type_from_gdtype(const GDScriptDataTyp
result.has_type = true;
result.builtin_type = p_gdtype.builtin_type;
result.native_type = p_gdtype.native_type;
result.script_type = p_gdtype.script_type;
result.script_type = Ref<Script>(p_gdtype.script_type);

switch (p_gdtype.kind) {
case GDScriptDataType::UNINITIALIZED: {
Expand Down