#1422 Fix crash if component and entity w/component are deleted durin…

…g cleanup
SanderMertens · Nov 2, 2024 · 04facf5 · 04facf5
1 parent 3a546e6
commit 04facf5
Show file tree

Hide file tree

Showing 12 changed files with 154 additions and 29 deletions.
diff --git a/distr/flecs.c b/distr/flecs.c
@@ -927,12 +927,13 @@ typedef struct ecs_store_t {
     /* Records cache */
     ecs_vec_t records;
 
-    /* Stack of ids being deleted. */
+    /* Stack of ids being deleted during cleanup action. */
     ecs_vec_t marked_ids;            /* vector<ecs_marked_ids_t> */
-
-    /* Entity ids associated with depth (for flat hierarchies) */
-    ecs_vec_t depth_ids;
-    ecs_map_t entity_to_depth; /* What it says */
+
+    /* Components deleted during cleanup action. Used to delay cleaning up of
+     * type info so it's guaranteed that this data is available while the 
+     * storage is cleaning up tables. */
+    ecs_vec_t deleted_components;    /* vector<ecs_entity_t> */
 } ecs_store_t;
 
 /* fini actions */
@@ -3698,7 +3699,19 @@ void flecs_on_component(ecs_iter_t *it) {
                 flecs_assert_relation_unused(world, e, ecs_id(EcsComponent));
             }
         } else if (it->event == EcsOnRemove) {
-            flecs_type_info_free(world, e);
+            #ifdef FLECS_DEBUG
+            if (ecs_should_log(0)) {
+                char *path = ecs_get_path(world, e);
+                ecs_trace("unregistering component '%s'", path);
+                ecs_os_free(path);
+            }
+            #endif
+            if (!ecs_vec_count(&world->store.marked_ids)) {
+                flecs_type_info_free(world, e);
+            } else {
+                ecs_vec_append_t(&world->allocator, 
+                    &world->store.deleted_components, ecs_entity_t)[0] = e;
+            }
         }
     }
 }
@@ -7735,7 +7748,7 @@ void flecs_on_delete(
     /* Cleanup can happen recursively. If a cleanup action is already in 
      * progress, only append ids to the marked_ids. The topmost cleanup
      * frame will handle the actual cleanup. */
-    int32_t count = ecs_vec_count(&world->store.marked_ids);
+    int32_t i, count = ecs_vec_count(&world->store.marked_ids);
 
     /* Make sure we're evaluating a consistent list of non-empty tables */
     ecs_run_aperiodic(world, EcsAperiodicEmptyTables);
@@ -7760,7 +7773,7 @@ void flecs_on_delete(
         /* Verify deleted ids are no longer in use */
 #ifdef FLECS_DEBUG
         ecs_marked_id_t *ids = ecs_vec_first(&world->store.marked_ids);
-        int32_t i; count = ecs_vec_count(&world->store.marked_ids);
+        count = ecs_vec_count(&world->store.marked_ids);
         for (i = 0; i < count; i ++) {
             ecs_assert(!ecs_id_in_use(world, ids[i].id), 
                 ECS_INTERNAL_ERROR, NULL);
@@ -7771,6 +7784,14 @@ void flecs_on_delete(
         /* Ids are deleted, clear stack */
         ecs_vec_clear(&world->store.marked_ids);
 
+        /* If any components got deleted, cleanup type info. Delaying this 
+         * ensures that type info remains available during cleanup. */
+        count = ecs_vec_count(&world->store.deleted_components);
+        ecs_entity_t *comps = ecs_vec_first(&world->store.deleted_components);
+        for (i = 0; i < count; i ++) {
+            flecs_type_info_free(world, comps[i]);
+        }
+
         ecs_log_pop_2();
     }
 }
@@ -18198,8 +18219,7 @@ void flecs_init_store(
     ecs_allocator_t *a = &world->allocator;
     ecs_vec_init_t(a, &world->store.records, ecs_table_record_t, 0);
     ecs_vec_init_t(a, &world->store.marked_ids, ecs_marked_id_t, 0);
-    ecs_vec_init_t(a, &world->store.depth_ids, ecs_entity_t, 0);
-    ecs_map_init(&world->store.entity_to_depth, &world->allocator);
+    ecs_vec_init_t(a, &world->store.deleted_components, ecs_entity_t, 0);
 
     /* Initialize entity index */
     flecs_entities_init(world);
@@ -18214,7 +18234,7 @@ void flecs_init_store(
     /* Initialize root table */
     flecs_init_root_table(world);
 
-    /* Initilaize observer sparse set */
+    /* Initialize observer sparse set */
     flecs_sparse_init_t(&world->store.observers,
         a, &world->allocators.sparse_chunk, ecs_observer_impl_t);
 }
@@ -18336,8 +18356,7 @@ void flecs_fini_store(ecs_world_t *world) {
     ecs_allocator_t *a = &world->allocator;
     ecs_vec_fini_t(a, &world->store.records, ecs_table_record_t);
     ecs_vec_fini_t(a, &world->store.marked_ids, ecs_marked_id_t);
-    ecs_vec_fini_t(a, &world->store.depth_ids, ecs_entity_t);
-    ecs_map_fini(&world->store.entity_to_depth);
+    ecs_vec_fini_t(a, &world->store.deleted_components, ecs_entity_t);
 }
 
 static 
@@ -19391,6 +19410,9 @@ void flecs_type_info_fini(
         ecs_os_free(ECS_CONST_CAST(char*, ti->name));
         ti->name = NULL;
     }
+
+    ti->size = 0;
+    ti->alignment = 0;
 }
 
 void flecs_type_info_free(
@@ -37167,6 +37189,7 @@ void flecs_table_fini_data(
         table->data.entities = NULL;
         table->data.size = 0;
     }
+
     table->data.count = 0;
 
     if (deactivate && count) {

diff --git a/src/bootstrap.c b/src/bootstrap.c
@@ -447,7 +447,19 @@ void flecs_on_component(ecs_iter_t *it) {
                 flecs_assert_relation_unused(world, e, ecs_id(EcsComponent));
             }
         } else if (it->event == EcsOnRemove) {
-            flecs_type_info_free(world, e);
+            #ifdef FLECS_DEBUG
+            if (ecs_should_log(0)) {
+                char *path = ecs_get_path(world, e);
+                ecs_trace("unregistering component '%s'", path);
+                ecs_os_free(path);
+            }
+            #endif
+            if (!ecs_vec_count(&world->store.marked_ids)) {
+                flecs_type_info_free(world, e);
+            } else {
+                ecs_vec_append_t(&world->allocator, 
+                    &world->store.deleted_components, ecs_entity_t)[0] = e;
+            }
         }
     }
 }

diff --git a/src/entity.c b/src/entity.c
@@ -2746,7 +2746,7 @@ void flecs_on_delete(
     /* Cleanup can happen recursively. If a cleanup action is already in 
      * progress, only append ids to the marked_ids. The topmost cleanup
      * frame will handle the actual cleanup. */
-    int32_t count = ecs_vec_count(&world->store.marked_ids);
+    int32_t i, count = ecs_vec_count(&world->store.marked_ids);
 
     /* Make sure we're evaluating a consistent list of non-empty tables */
     ecs_run_aperiodic(world, EcsAperiodicEmptyTables);
@@ -2771,7 +2771,7 @@ void flecs_on_delete(
         /* Verify deleted ids are no longer in use */
 #ifdef FLECS_DEBUG
         ecs_marked_id_t *ids = ecs_vec_first(&world->store.marked_ids);
-        int32_t i; count = ecs_vec_count(&world->store.marked_ids);
+        count = ecs_vec_count(&world->store.marked_ids);
         for (i = 0; i < count; i ++) {
             ecs_assert(!ecs_id_in_use(world, ids[i].id), 
                 ECS_INTERNAL_ERROR, NULL);
@@ -2782,6 +2782,14 @@ void flecs_on_delete(
         /* Ids are deleted, clear stack */
         ecs_vec_clear(&world->store.marked_ids);
 
+        /* If any components got deleted, cleanup type info. Delaying this 
+         * ensures that type info remains available during cleanup. */
+        count = ecs_vec_count(&world->store.deleted_components);
+        ecs_entity_t *comps = ecs_vec_first(&world->store.deleted_components);
+        for (i = 0; i < count; i ++) {
+            flecs_type_info_free(world, comps[i]);
+        }
+
         ecs_log_pop_2();
     }
 }

diff --git a/src/private_types.h b/src/private_types.h
@@ -276,12 +276,13 @@ typedef struct ecs_store_t {
     /* Records cache */
     ecs_vec_t records;
 
-    /* Stack of ids being deleted. */
+    /* Stack of ids being deleted during cleanup action. */
     ecs_vec_t marked_ids;            /* vector<ecs_marked_ids_t> */
-
-    /* Entity ids associated with depth (for flat hierarchies) */
-    ecs_vec_t depth_ids;
-    ecs_map_t entity_to_depth; /* What it says */
+
+    /* Components deleted during cleanup action. Used to delay cleaning up of
+     * type info so it's guaranteed that this data is available while the 
+     * storage is cleaning up tables. */
+    ecs_vec_t deleted_components;    /* vector<ecs_entity_t> */
 } ecs_store_t;
 
 /* fini actions */

diff --git a/src/storage/table.c b/src/storage/table.c
@@ -944,6 +944,7 @@ void flecs_table_fini_data(
         table->data.entities = NULL;
         table->data.size = 0;
     }
+
     table->data.count = 0;
 
     if (deactivate && count) {

diff --git a/src/world.c b/src/world.c
@@ -575,8 +575,7 @@ void flecs_init_store(
     ecs_allocator_t *a = &world->allocator;
     ecs_vec_init_t(a, &world->store.records, ecs_table_record_t, 0);
     ecs_vec_init_t(a, &world->store.marked_ids, ecs_marked_id_t, 0);
-    ecs_vec_init_t(a, &world->store.depth_ids, ecs_entity_t, 0);
-    ecs_map_init(&world->store.entity_to_depth, &world->allocator);
+    ecs_vec_init_t(a, &world->store.deleted_components, ecs_entity_t, 0);
 
     /* Initialize entity index */
     flecs_entities_init(world);
@@ -591,7 +590,7 @@ void flecs_init_store(
     /* Initialize root table */
     flecs_init_root_table(world);
 
-    /* Initilaize observer sparse set */
+    /* Initialize observer sparse set */
     flecs_sparse_init_t(&world->store.observers,
         a, &world->allocators.sparse_chunk, ecs_observer_impl_t);
 }
@@ -713,8 +712,7 @@ void flecs_fini_store(ecs_world_t *world) {
     ecs_allocator_t *a = &world->allocator;
     ecs_vec_fini_t(a, &world->store.records, ecs_table_record_t);
     ecs_vec_fini_t(a, &world->store.marked_ids, ecs_marked_id_t);
-    ecs_vec_fini_t(a, &world->store.depth_ids, ecs_entity_t);
-    ecs_map_fini(&world->store.entity_to_depth);
+    ecs_vec_fini_t(a, &world->store.deleted_components, ecs_entity_t);
 }
 
 static 
@@ -1768,6 +1766,9 @@ void flecs_type_info_fini(
         ecs_os_free(ECS_CONST_CAST(char*, ti->name));
         ti->name = NULL;
     }
+
+    ti->size = 0;
+    ti->alignment = 0;
 }
 
 void flecs_type_info_free(

diff --git a/test/core/project.json b/test/core/project.json
@@ -933,6 +933,7 @@
                 "add_on_delete_from_disabled",
                 "delete_on_delete_from_prefab",
                 "delete_on_delete_from_disabled",
+                "delete_all_w_component_cycle",
                 "remove_all_1",
                 "remove_all_2",
                 "remove_all_3",

diff --git a/test/core/src/OnDelete.c b/test/core/src/OnDelete.c
@@ -3275,6 +3275,37 @@ void OnDelete_remove_all_3(void) {
     ecs_fini(world);
 }
 
+void OnDelete_delete_all_w_component_cycle(void) {
+    ecs_world_t *world = ecs_mini();
+
+    ecs_entity_t tag = ecs_new(world);
+
+    ecs_entity_t comp_a = ecs_component(world, {
+        .type.size = 4,
+        .type.alignment = 4
+    });
+    ecs_add_id(world, comp_a, tag);
+
+    ecs_entity_t comp_b = ecs_component(world, {
+        .type.size = 4,
+        .type.alignment = 4
+    });
+    ecs_add_id(world, comp_b, tag);
+
+    ecs_add_id(world, comp_a, comp_b);
+    ecs_add_id(world, comp_b, comp_a);
+
+    ecs_delete_with(world, tag);
+
+    test_assert(!ecs_is_alive(world, comp_a));
+    test_assert(!ecs_is_alive(world, comp_b));
+
+    test_assert(ecs_get_type_info(world, comp_a) == NULL);
+    test_assert(ecs_get_type_info(world, comp_b) == NULL);
+
+    ecs_fini(world);
+}
+
 void OnDelete_delete_with_1(void) {
     ecs_world_t *world = ecs_init();
 

diff --git a/test/core/src/main.c b/test/core/src/main.c
@@ -890,6 +890,7 @@ void OnDelete_add_on_delete_from_prefab(void);
 void OnDelete_add_on_delete_from_disabled(void);
 void OnDelete_delete_on_delete_from_prefab(void);
 void OnDelete_delete_on_delete_from_disabled(void);
+void OnDelete_delete_all_w_component_cycle(void);
 void OnDelete_remove_all_1(void);
 void OnDelete_remove_all_2(void);
 void OnDelete_remove_all_3(void);
@@ -5694,6 +5695,10 @@ bake_test_case OnDelete_testcases[] = {
         "delete_on_delete_from_disabled",
         OnDelete_delete_on_delete_from_disabled
     },
+    {
+        "delete_all_w_component_cycle",
+        OnDelete_delete_all_w_component_cycle
+    },
     {
         "remove_all_1",
         OnDelete_remove_all_1
@@ -11237,7 +11242,7 @@ static bake_test_suite suites[] = {
         "OnDelete",
         NULL,
         NULL,
-        124,
+        125,
         OnDelete_testcases
     },
     {

diff --git a/test/script/project.json b/test/script/project.json
@@ -263,7 +263,8 @@
                 "partial_assign_nontrivial_with",
                 "partial_assign_with_large_array",
                 "non_trivial_var_component",
-                "non_trivial_var_with"
+                "non_trivial_var_with",
+                "update_template_w_tag"
             ]
         }, {
             "id": "Template",

diff --git a/test/script/src/Eval.c b/test/script/src/Eval.c
@@ -8239,3 +8239,39 @@ void Eval_non_trivial_var_with(void) {
     test_int(strings_copy_invoked, 2);
     test_int(strings_move_invoked, 0);
 }
+
+void Eval_update_template_w_tag(void) {
+    ecs_world_t *world = ecs_init();
+
+    ECS_TAG(world, Bar);
+
+    const char *expr =
+    HEAD "Foo {}"
+    LINE
+    LINE "template Bar {"
+    LINE "  Foo"
+    LINE "}"
+    LINE
+    LINE "Bar e()";
+
+    ecs_entity_t s = ecs_script(world, {
+        .entity = ecs_entity(world, { .name = "main" }),
+        .code = expr
+    });
+    test_assert(s != 0);
+
+    test_assert(ecs_script_update(world, s, 0, expr) == 0);
+
+    ecs_entity_t foo = ecs_lookup(world, "Foo");
+    ecs_entity_t bar = ecs_lookup(world, "Bar");
+    ecs_entity_t e = ecs_lookup(world, "e");
+
+    test_assert(foo != 0);
+    test_assert(bar != 0);
+    test_assert(e != 0);
+
+    test_assert(ecs_has_id(world, e, foo));
+    test_assert(ecs_has_id(world, e, bar));
+
+    ecs_fini(world);
+}
diff --git a/test/script/src/main.c b/test/script/src/main.c
@@ -260,6 +260,7 @@ void Eval_partial_assign_nontrivial_with(void);
 void Eval_partial_assign_with_large_array(void);
 void Eval_non_trivial_var_component(void);
 void Eval_non_trivial_var_with(void);
+void Eval_update_template_w_tag(void);
 
 // Testsuite 'Template'
 void Template_template_no_scope(void);
@@ -1645,6 +1646,10 @@ bake_test_case Eval_testcases[] = {
     {
         "non_trivial_var_with",
         Eval_non_trivial_var_with
+    },
+    {
+        "update_template_w_tag",
+        Eval_update_template_w_tag
     }
 };
 
@@ -3144,7 +3149,7 @@ static bake_test_suite suites[] = {
         "Eval",
         NULL,
         NULL,
-        251,
+        252,
         Eval_testcases
     },
     {