From a0969662cdfe6e00b6c42e15bb5775ee5eb5d79b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pedro=20J=2E=20Est=C3=A9banez?= <pedrojrulez@gmail.com> Date: Wed, 9 Sep 2020 13:05:28 +0200 Subject: [PATCH 1/2] Prevent cyclic reference between script and its members --- modules/gdscript/gdscript_compiler.cpp | 22 ++++++++++++++-------- modules/gdscript/gdscript_compiler.h | 2 +- modules/gdscript/gdscript_editor.cpp | 2 +- modules/gdscript/gdscript_function.h | 6 ++++-- modules/gdscript/gdscript_parser.cpp | 2 +- 5 files changed, 21 insertions(+), 13 deletions(-) diff --git a/modules/gdscript/gdscript_compiler.cpp b/modules/gdscript/gdscript_compiler.cpp index b175290698c..534c49a9446 100644 --- a/modules/gdscript/gdscript_compiler.cpp +++ b/modules/gdscript/gdscript_compiler.cpp @@ -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(); } @@ -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: { @@ -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: { @@ -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; } @@ -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; @@ -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; @@ -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; diff --git a/modules/gdscript/gdscript_compiler.h b/modules/gdscript/gdscript_compiler.h index 7d5234a0238..a0572435f38 100644 --- a/modules/gdscript/gdscript_compiler.h +++ b/modules/gdscript/gdscript_compiler.h @@ -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); diff --git a/modules/gdscript/gdscript_editor.cpp b/modules/gdscript/gdscript_editor.cpp index 438230cb02e..eab0a3702dc 100644 --- a/modules/gdscript/gdscript_editor.cpp +++ b/modules/gdscript/gdscript_editor.cpp @@ -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: { diff --git a/modules/gdscript/gdscript_function.h b/modules/gdscript/gdscript_function.h index b5e2e0021d2..c2084d6a51d 100644 --- a/modules/gdscript/gdscript_function.h +++ b/modules/gdscript/gdscript_function.h @@ -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 @@ -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 { diff --git a/modules/gdscript/gdscript_parser.cpp b/modules/gdscript/gdscript_parser.cpp index bd99edd1a06..22cd1ea6e94 100644 --- a/modules/gdscript/gdscript_parser.cpp +++ b/modules/gdscript/gdscript_parser.cpp @@ -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: { From 8e649691840ac3f08053fa17aaface4608b949d0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pedro=20J=2E=20Est=C3=A9banez?= <pedrojrulez@gmail.com> Date: Wed, 9 Sep 2020 13:32:11 +0200 Subject: [PATCH 2/2] Ensure cyclic dependencies between scripts are broken at exit --- modules/gdscript/gdscript.cpp | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/modules/gdscript/gdscript.cpp b/modules/gdscript/gdscript.cpp index 7f89f2037c4..513ea428047 100644 --- a/modules/gdscript/gdscript.cpp +++ b/modules/gdscript/gdscript.cpp @@ -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; }