Merge pull request #68281 from maximkulkin/resource-circular-references

Fix crash when saving resources with circular references
godotengine · Jul 14, 2023 · 4dc26bf · 4dc26bf
2 parents 5f23b8b + 058604f
commit 4dc26bf
Show file tree

Hide file tree

Showing 3 changed files with 57 additions and 3 deletions.
diff --git a/core/io/resource_format_binary.cpp b/core/io/resource_format_binary.cpp
@@ -1960,6 +1960,8 @@ void ResourceFormatSaverBinaryInstance::_find_resources(const Variant &p_variant
 				return;
 			}
 
+			resource_set.insert(res);
+
 			List<PropertyInfo> property_list;
 
 			res->get_property_list(&property_list);
@@ -1983,7 +1985,6 @@ void ResourceFormatSaverBinaryInstance::_find_resources(const Variant &p_variant
 				}
 			}
 
-			resource_set.insert(res);
 			saved_resources.push_back(res);
 
 		} break;

diff --git a/scene/resources/resource_format_text.cpp b/scene/resources/resource_format_text.cpp
@@ -1877,6 +1877,8 @@ void ResourceFormatSaverTextInstance::_find_resources(const Variant &p_variant,
 				return;
 			}
 
+			resource_set.insert(res);
+
 			List<PropertyInfo> property_list;
 
 			res->get_property_list(&property_list);
@@ -1908,8 +1910,7 @@ void ResourceFormatSaverTextInstance::_find_resources(const Variant &p_variant,
 				I = I->next();
 			}
 
-			resource_set.insert(res); //saved after, so the children it needs are available when loaded
-			saved_resources.push_back(res);
+			saved_resources.push_back(res); // Saved after, so the children it needs are available when loaded
 
 		} break;
 		case Variant::ARRAY: {

diff --git a/tests/core/io/test_resource.h b/tests/core/io/test_resource.h
@@ -109,6 +109,58 @@ TEST_CASE("[Resource] Saving and loading") {
 			loaded_child_resource_text->get_name() == "I'm a child resource",
 			"The loaded child resource name should be equal to the expected value.");
 }
+
+TEST_CASE("[Resource] Breaking circular references on save") {
+	Ref<Resource> resource_a = memnew(Resource);
+	resource_a->set_name("A");
+	Ref<Resource> resource_b = memnew(Resource);
+	resource_b->set_name("B");
+	Ref<Resource> resource_c = memnew(Resource);
+	resource_c->set_name("C");
+	resource_a->set_meta("next", resource_b);
+	resource_b->set_meta("next", resource_c);
+	resource_c->set_meta("next", resource_b);
+
+	const String save_path_binary = OS::get_singleton()->get_cache_path().path_join("resource.res");
+	const String save_path_text = OS::get_singleton()->get_cache_path().path_join("resource.tres");
+	ResourceSaver::save(resource_a, save_path_binary);
+	ResourceSaver::save(resource_a, save_path_text);
+
+	const Ref<Resource> &loaded_resource_a_binary = ResourceLoader::load(save_path_binary);
+	CHECK_MESSAGE(
+			loaded_resource_a_binary->get_name() == "A",
+			"The loaded resource name should be equal to the expected value.");
+	const Ref<Resource> &loaded_resource_b_binary = loaded_resource_a_binary->get_meta("next");
+	CHECK_MESSAGE(
+			loaded_resource_b_binary->get_name() == "B",
+			"The loaded child resource name should be equal to the expected value.");
+	const Ref<Resource> &loaded_resource_c_binary = loaded_resource_b_binary->get_meta("next");
+	CHECK_MESSAGE(
+			loaded_resource_c_binary->get_name() == "C",
+			"The loaded child resource name should be equal to the expected value.");
+	CHECK_MESSAGE(
+			!loaded_resource_c_binary->get_meta("next"),
+			"The loaded child resource circular reference should be NULL.");
+
+	const Ref<Resource> &loaded_resource_a_text = ResourceLoader::load(save_path_text);
+	CHECK_MESSAGE(
+			loaded_resource_a_text->get_name() == "A",
+			"The loaded resource name should be equal to the expected value.");
+	const Ref<Resource> &loaded_resource_b_text = loaded_resource_a_text->get_meta("next");
+	CHECK_MESSAGE(
+			loaded_resource_b_text->get_name() == "B",
+			"The loaded child resource name should be equal to the expected value.");
+	const Ref<Resource> &loaded_resource_c_text = loaded_resource_b_text->get_meta("next");
+	CHECK_MESSAGE(
+			loaded_resource_c_text->get_name() == "C",
+			"The loaded child resource name should be equal to the expected value.");
+	CHECK_MESSAGE(
+			!loaded_resource_c_text->get_meta("next"),
+			"The loaded child resource circular reference should be NULL.");
+
+	// Break circular reference to avoid memory leak
+	resource_c->remove_meta("next");
+}
 } // namespace TestResource
 
 #endif // TEST_RESOURCE_H