From c31a84ab658326477c21b879fe6419195c9b965b Mon Sep 17 00:00:00 2001 From: Richard Christie Date: Wed, 8 Mar 2023 13:23:19 +1300 Subject: [PATCH] Destroy objects immediately on removal from manager Fixes group field issues where orphaned subobject groups maintain pointer to removed owner group, and deferred destruction was clearing subobject group pointers to new owner group leading to a crash. Added test confirming that removal of a field removes chain of fields accessed only through it with a single remove notification. Includes several related fixes to mitigate possible reentrant code with early destruction of fields. Deaccess field now clears client pointer early. --- CHANGELOG.txt | 1 + CMakeLists.txt | 2 +- src/computed_field/computed_field.cpp | 17 ++- src/computed_field/computed_field_group.cpp | 81 ++++++++---- src/computed_field/computed_field_group.hpp | 15 ++- src/computed_field/computed_field_image.cpp | 2 +- src/computed_field/computed_field_private.hpp | 3 +- .../computed_field_subobject_group.hpp | 14 +- src/general/manager.h | 2 +- src/general/manager_private.h | 96 ++++++-------- tests/fieldmodule/fieldmodulenotifier.cpp | 114 ++++++++++++++++- tests/fieldmodule/finiteelement.cpp | 17 ++- tests/selection/group.cpp | 121 ++++++++++++++++++ 13 files changed, 382 insertions(+), 103 deletions(-) diff --git a/CHANGELOG.txt b/CHANGELOG.txt index 05bee0b2a..1e8bd663e 100644 --- a/CHANGELOG.txt +++ b/CHANGELOG.txt @@ -3,6 +3,7 @@ CHANGE LOG: OpenCMISS-Zinc Library v2.10.2 Fix behaviour of get/create subregion groups, with get now able to discover subregion groups by name. Fix scene set selection group so if not set, inherits subregion group of parent/ancestor scene selection group. +Destroy objects immediately on removal from manager to release objects they reference. Fixes invalid references from sub-object group to destroyed owner group. v3.10.1 Fix serialisation of field types identity and if. diff --git a/CMakeLists.txt b/CMakeLists.txt index 1befdf909..3f29d2e55 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -25,7 +25,7 @@ if(POLICY CMP0086) endif() # This is the project name and shows up in IDEs -project(Zinc VERSION 3.10.1 LANGUAGES C CXX) +project(Zinc VERSION 3.10.2 LANGUAGES C CXX) # Set this to the development release or release candidate version. set(Zinc_DEVELOPER_VERSION ".dev0") diff --git a/src/computed_field/computed_field.cpp b/src/computed_field/computed_field.cpp index 39b407888..f475ba21d 100644 --- a/src/computed_field/computed_field.cpp +++ b/src/computed_field/computed_field.cpp @@ -238,10 +238,12 @@ cmzn_field *cmzn_field::create(const char *nameIn) return field; } -void cmzn_field::deaccess(cmzn_field*& field) +void cmzn_field::deaccess(cmzn_field*& field_ref) { - if (field) + if (field_ref) { + cmzn_field* field = field_ref; + field_ref = nullptr; // clear client's pointer ASAP in case manager message sent below --(field->access_count); if (field->access_count <= 0) { @@ -253,9 +255,12 @@ void cmzn_field::deaccess(cmzn_field*& field) (MANAGER_CHANGE_NONE(cmzn_field) != field->manager_change_status))) && field->core->not_in_use()) { + // removing a field from manager can release other fields it has reference to so cache changes: + MANAGER(cmzn_field)* manager = field->manager; + MANAGER_BEGIN_CACHE(cmzn_field)(manager); REMOVE_OBJECT_FROM_MANAGER(cmzn_field)(field, field->manager); + MANAGER_END_CACHE(cmzn_field)(manager); } - field = nullptr; } } @@ -406,9 +411,9 @@ Note: assumes caller is accessing field once! { if (manager == object->manager) { - if ((2 >= object->access_count) || + if (((1 + extraAccesses) == object->access_count) || ((MANAGER_CHANGE_NONE(cmzn_field) != object->manager_change_status) && - (3 == object->access_count))) + ((2 + extraAccesses) == object->access_count))) { return_code = object->core ? object->core->not_in_use() : 1; } @@ -701,7 +706,7 @@ int cmzn_field::copyDefinition(const cmzn_field& source) } if (((source.number_of_components != this->number_of_components) || (source.getValueType() != this->getValueType())) && - (!MANAGED_OBJECT_NOT_IN_USE(cmzn_field)(this, manager))) + (!MANAGED_OBJECT_NOT_IN_USE(cmzn_field)(this, manager, 1))) { display_message(ERROR_MESSAGE, "cmzn_field::copyDefinition. " "Cannot change number of components or value type while field is in use"); diff --git a/src/computed_field/computed_field_group.cpp b/src/computed_field/computed_field_group.cpp index a4a4e69eb..dc7f99916 100755 --- a/src/computed_field/computed_field_group.cpp +++ b/src/computed_field/computed_field_group.cpp @@ -57,10 +57,12 @@ Computed_field_group::Computed_field_group(cmzn_region *region) Computed_field_group::~Computed_field_group() { - clearLocalNodeGroup(/*isData*/false); - clearLocalNodeGroup(/*isData*/true); + clearRemoveLocalNodeGroup(/*isData*/false, /*clear*/false, /*remove*/true); + clearRemoveLocalNodeGroup(/*isData*/true, /*clear*/false, /*remove*/true); for (int i = 0; i < MAXIMUM_ELEMENT_XI_DIMENSIONS; i++) - clearLocalElementGroup(i); + { + clearRemoveLocalElementGroup(i, /*clear*/false, /*remove*/true); + } for (Region_field_map_iterator iter = child_region_group_map.begin(); iter != child_region_group_map.end(); ++iter) { @@ -75,16 +77,26 @@ Computed_field_group::~Computed_field_group() } } -void Computed_field_group::clearLocalElementGroup(int index) +void Computed_field_group::clearRemoveLocalElementGroup(int index, bool clear, bool remove) { - if (this->local_element_group[index]) + cmzn_field*& element_group_field = this->local_element_group[index]; + if (element_group_field) { Computed_field_subobject_group *subobject_group = - static_cast(this->local_element_group[index]->core); - subobject_group->clear(); - subobject_group->setOwnerGroup(nullptr); - check_subobject_group_dependency(subobject_group); - cmzn_field_destroy(&this->local_element_group[index]); + static_cast(element_group_field->core); + if ((!subobject_group->isEmpty()) || subobject_group->is_change_remove()) + { + this->change_detail.changeRemoveLocal(); + } + if (clear) + { + subobject_group->clear(); + } + if (remove) + { + subobject_group->setOwnerGroup(nullptr); + cmzn_field::deaccess(element_group_field); + } } } @@ -92,7 +104,7 @@ void Computed_field_group::setLocalElementGroup(int index, cmzn_field_element_gr { if (cmzn_field_element_group_base_cast(element_group) != this->local_element_group[index]) { - clearLocalElementGroup(index); + clearRemoveLocalElementGroup(index, /*clear*/false, /*remove*/true); if (element_group) { Computed_field_element_group_core_cast(element_group)->setOwnerGroup(this); @@ -101,17 +113,26 @@ void Computed_field_group::setLocalElementGroup(int index, cmzn_field_element_gr } } -void Computed_field_group::clearLocalNodeGroup(bool isData) +void Computed_field_group::clearRemoveLocalNodeGroup(bool isData, bool clear, bool remove) { - cmzn_field_id *node_group_field_address = (isData) ? &local_data_group : &local_node_group; - if (*node_group_field_address) + cmzn_field*& node_group_field = (isData) ? local_data_group : local_node_group; + if (node_group_field) { Computed_field_subobject_group *subobject_group = - static_cast((*node_group_field_address)->core); - subobject_group->clear(); - subobject_group->setOwnerGroup(nullptr); - check_subobject_group_dependency(subobject_group); - cmzn_field_destroy(node_group_field_address); + static_cast(node_group_field->core); + if ((!subobject_group->isEmpty()) || subobject_group->is_change_remove()) + { + this->change_detail.changeRemoveLocal(); + } + if (clear) + { + subobject_group->clear(); + } + if (remove) + { + subobject_group->setOwnerGroup(nullptr); + cmzn_field::deaccess(node_group_field); + } } } @@ -120,7 +141,7 @@ void Computed_field_group::setLocalNodeGroup(bool isData, cmzn_field_node_group cmzn_field_id& local_node_group_field = (isData) ? this->local_data_group : this->local_node_group; if (cmzn_field_node_group_base_cast(node_group) != local_node_group_field) { - clearLocalNodeGroup(isData); + clearRemoveLocalNodeGroup(isData, /*clear*/false, /*remove*/true); if (node_group) { Computed_field_node_group_core_cast(node_group)->setOwnerGroup(this); @@ -264,10 +285,12 @@ int Computed_field_group::clearLocal() cmzn_field_group_subelement_handling_mode oldSubelementHandlingMode = this->subelementHandlingMode; this->subelementHandlingMode = CMZN_FIELD_GROUP_SUBELEMENT_HANDLING_MODE_NONE; contains_all = false; - clearLocalNodeGroup(/*isData*/false); - clearLocalNodeGroup(/*isData*/true); + clearRemoveLocalNodeGroup(/*isData*/false); + clearRemoveLocalNodeGroup(/*isData*/true); for (int i = 0; i < MAXIMUM_ELEMENT_XI_DIMENSIONS; i++) - clearLocalElementGroup(i); + { + clearRemoveLocalElementGroup(i); + } change_detail.changeRemoveLocal(); this->subelementHandlingMode = oldSubelementHandlingMode; this->field->setChanged(); @@ -990,13 +1013,17 @@ int Computed_field_group::remove_empty_subgroups() { Computed_field_group_base *group_base = static_cast(local_node_group->core); if (group_base->isEmpty()) - this->clearLocalNodeGroup(/*isData*/false); + { + this->clearRemoveLocalNodeGroup(/*isData*/false, /*clear*/false, /*remove*/true); + } } if (local_data_group) { Computed_field_group_base *group_base = static_cast(local_data_group->core); if (group_base->isEmpty()) - this->clearLocalNodeGroup(/*isData*/true); + { + this->clearRemoveLocalNodeGroup(/*isData*/true, /*clear*/false, /*remove*/true); + } } for (int i = 0; i < MAXIMUM_ELEMENT_XI_DIMENSIONS; i++) { @@ -1005,7 +1032,9 @@ int Computed_field_group::remove_empty_subgroups() Computed_field_subobject_group *subobject_group = static_cast( this->local_element_group[i]->core); if (subobject_group->isEmpty()) - this->clearLocalElementGroup(i); + { + this->clearRemoveLocalElementGroup(i, /*clear*/false, /*remove*/true); + } } } /* remove empty subregion group */ diff --git a/src/computed_field/computed_field_group.hpp b/src/computed_field/computed_field_group.hpp index 2ce819198..4f3499b13 100755 --- a/src/computed_field/computed_field_group.hpp +++ b/src/computed_field/computed_field_group.hpp @@ -95,10 +95,19 @@ class Computed_field_group : public Computed_field_group_base std::map domain_selection_group; Region_field_map child_region_group_map; // map to accessed FieldGroup in child regions - /* @param index 0 (1-D), 1 (2-D) or 2 (3-D) */ - void clearLocalElementGroup(int index); + /* + * @param index 0 (1-D), 1 (2-D) or 2 (3-D) + * @param clear If true, clear the subelement group. + * @param remove If true, remove the subelement group. + */ + void clearRemoveLocalElementGroup(int index, bool clear = true, bool remove = true); void setLocalElementGroup(int index, cmzn_field_element_group *element_group); - void clearLocalNodeGroup(bool isData); + /* + * @param isData True for datapoints, false for nodes. + * @param clear If true, clear the subelement group. + * @param remove If true, remove the subelement group. + */ + void clearRemoveLocalNodeGroup(bool isData, bool clear = true, bool remove = true); void setLocalNodeGroup(bool isData, cmzn_field_node_group *node_group); public: diff --git a/src/computed_field/computed_field_image.cpp b/src/computed_field/computed_field_image.cpp index 80121cfd8..7a726bcb4 100644 --- a/src/computed_field/computed_field_image.cpp +++ b/src/computed_field/computed_field_image.cpp @@ -116,7 +116,7 @@ class Computed_field_image : public Computed_field_core new_number_of_components = field->number_of_components; } if ((field->number_of_components == new_number_of_components) - || (MANAGED_OBJECT_NOT_IN_USE(Computed_field)(field, field->manager) || + || (MANAGED_OBJECT_NOT_IN_USE(Computed_field)(field, field->manager, 1) || Computed_field_is_not_source_field_of_others(field))) { REACCESS(Texture)(&(this->texture), texture_in); diff --git a/src/computed_field/computed_field_private.hpp b/src/computed_field/computed_field_private.hpp index b62e23512..06f438c90 100644 --- a/src/computed_field/computed_field_private.hpp +++ b/src/computed_field/computed_field_private.hpp @@ -425,7 +425,7 @@ struct cmzn_field return this; } - static void deaccess(cmzn_field*& field); + static void deaccess(cmzn_field*& field_ref); static void reaccess(cmzn_field*& field, cmzn_field *newField) { @@ -470,6 +470,7 @@ struct cmzn_field * Note this copies the definition only; finite element fields defined on nodes * and elements are not affected. * Does not copy name of field. + * Assumes one access held externally for this field. * * @source Field to copy functional definition from. * @return Result OK on success, ERROR_ARGUMENT if fails because source diff --git a/src/computed_field/computed_field_subobject_group.hpp b/src/computed_field/computed_field_subobject_group.hpp index 07b6d91a1..da2656e9b 100755 --- a/src/computed_field/computed_field_subobject_group.hpp +++ b/src/computed_field/computed_field_subobject_group.hpp @@ -105,7 +105,7 @@ class Computed_field_subobject_group : public Computed_field_group_base else if (field->manager_change_status & MANAGER_CHANGE_ADD(Computed_field)) { const cmzn_field_subobject_group_change_detail *change_detail = - dynamic_cast(get_change_detail()); + dynamic_cast(this->get_change_detail()); const int changeSummary = change_detail->getChangeSummary(); if (changeSummary & CMZN_FIELD_GROUP_CHANGE_ADD) { @@ -115,6 +115,18 @@ class Computed_field_subobject_group : public Computed_field_group_base return false; } + bool is_change_remove() const + { + const cmzn_field_subobject_group_change_detail* change_detail = + dynamic_cast(this->get_change_detail()); + const int changeSummary = change_detail->getChangeSummary(); + if (changeSummary & CMZN_FIELD_GROUP_CHANGE_REMOVE) + { + return true; + } + return false; + } + // Set for subobject groups which are managed by a Computed_field_group. void setOwnerGroup(Computed_field_group *ownerGroupIn) { diff --git a/src/general/manager.h b/src/general/manager.h index e1f3f3590..602cfb6f7 100644 --- a/src/general/manager.h +++ b/src/general/manager.h @@ -368,7 +368,7 @@ Returns a non-zero if the is managed by the . \ #define PROTOTYPE_MANAGED_OBJECT_NOT_IN_USE_FUNCTION( object_type ) \ int MANAGED_OBJECT_NOT_IN_USE(object_type)(struct object_type *object, \ - struct MANAGER(object_type) *manager) \ + struct MANAGER(object_type) *manager, int extraAccesses) \ /***************************************************************************** \ LAST MODIFIED : 18 January 2002 \ \ diff --git a/src/general/manager_private.h b/src/general/manager_private.h index 45b211e39..2c832e048 100644 --- a/src/general/manager_private.h +++ b/src/general/manager_private.h @@ -195,10 +195,11 @@ DECLARE_MANAGER_TYPE(object_type) \ /* the list of callbacks invoked when manager has changed */ \ struct MANAGER_CALLBACK_ITEM(object_type) *callback_list; \ int locked; \ - /* lists of objects added/changed OR removed since the last update message; \ - * separate so can add new object with same identifier as one removed. */ \ + /* lists of objects added/changed since the last update message; \ + * removed objects are destroyed immediately - clients cannot query them */ \ struct LIST(object_type) *changed_object_list; \ - struct LIST(object_type) *removed_object_list; \ + /* removed objects are only counted to ensure message sent */ \ + int number_of_removed_objects; \ /* pointer to owning object which exists for lifetime of this manager, if any */ \ owner_type *owner; \ bool external_change; \ @@ -259,7 +260,7 @@ inline void MANAGER_CLEANUP_CHANGE_DETAIL(object_type)(void **change_detail_addr static void MANAGER_UPDATE(object_type)(struct MANAGER(object_type) *manager) \ /** \ * Send a manager message to registered clients about changes to objects in \ - * the manager's changed_object_list and removed_object_list, if any. \ + * the manager's changed_object_list, if any. \ * Change information is copied out of the manager and objects before the \ * message is sent. \ */ \ @@ -267,8 +268,7 @@ static void MANAGER_UPDATE(object_type)(struct MANAGER(object_type) *manager) \ if (manager) \ { \ int number_of_changed_objects = NUMBER_IN_LIST(object_type)(manager->changed_object_list); \ - int number_of_removed_objects = NUMBER_IN_LIST(object_type)(manager->removed_object_list); \ - if (number_of_changed_objects || number_of_removed_objects || manager->external_change) \ + if (number_of_changed_objects || manager->number_of_removed_objects || manager->external_change) \ { \ MANAGER_UPDATE_DEPENDENCIES(object_type)(manager); \ manager->external_change = false; \ @@ -285,29 +285,27 @@ static void MANAGER_UPDATE(object_type)(struct MANAGER(object_type) *manager) \ object->manager_change_status = MANAGER_CHANGE_NONE(object_type); \ REMOVE_OBJECT_FROM_LIST(object_type)(object, manager->changed_object_list); \ } \ - for (int i = 0; i < number_of_removed_objects; i++) \ + if (manager->number_of_removed_objects) \ { \ - struct object_type *object = FIRST_OBJECT_IN_LIST_THAT(object_type)( \ - (LIST_CONDITIONAL_FUNCTION(object_type) *)NULL, (void *)NULL, manager->removed_object_list); \ - message->addObjectChange(object); \ - message->change_summary |= object->manager_change_status; \ - object->manager_change_status = MANAGER_CHANGE_NONE(object_type); \ - REMOVE_OBJECT_FROM_LIST(object_type)(object, manager->removed_object_list); \ + message->change_summary |= MANAGER_CHANGE_REMOVE(object_type); \ + manager->number_of_removed_objects = 0; \ } \ - /* send message to clients */ \ + /* send message to clients, but cache changes to avoid (their) reentrancy */ \ + MANAGER_BEGIN_CACHE(object_type)(manager); \ struct MANAGER_CALLBACK_ITEM(object_type) *item = manager->callback_list; \ while (item) \ { \ (item->callback)(message, item->user_data); \ item = item->next; \ } \ + message->deaccess(message); \ + MANAGER_END_CACHE(object_type)(manager); \ } \ else \ { \ display_message(ERROR_MESSAGE, \ "MANAGER_UPDATE(" #object_type "). Could not build message"); \ } \ - message->deaccess(message); \ } \ } \ else \ @@ -382,7 +380,7 @@ List conditional version of MANAGED_OBJECT_NOT_IN_USE function. \ \ ENTER(MANAGED_OBJECT_NOT_IN_USE_CONDITIONAL(object_type)); \ return_code = MANAGED_OBJECT_NOT_IN_USE(object_type)(object, \ - (struct MANAGER(object_type) *)manager_void); \ + (struct MANAGER(object_type) *)manager_void, 0); \ LEAVE; \ \ return (return_code); \ @@ -435,9 +433,9 @@ PROTOTYPE_CREATE_MANAGER_FUNCTION(object_type) \ { \ manager->object_list = CREATE_LIST(object_type)(); \ manager->changed_object_list = CREATE_RELATED_LIST(object_type)(manager->object_list); \ - manager->removed_object_list = CREATE_RELATED_LIST(object_type)(manager->object_list); \ - if (manager->object_list && manager->changed_object_list && manager->removed_object_list) \ + if (manager->object_list && manager->changed_object_list) \ { \ + manager->number_of_removed_objects = 0; \ manager->callback_list = \ (struct MANAGER_CALLBACK_ITEM(object_type) *)NULL; \ manager->locked = 0; \ @@ -448,7 +446,6 @@ PROTOTYPE_CREATE_MANAGER_FUNCTION(object_type) \ { \ display_message(ERROR_MESSAGE, \ "MANAGER_CREATE(" #object_type "). Could not create object lists"); \ - DESTROY(LIST(object_type))(&(manager->removed_object_list)); \ DESTROY(LIST(object_type))(&(manager->changed_object_list)); \ DESTROY(LIST(object_type))(&(manager->object_list)); \ DEALLOCATE(manager); \ @@ -486,9 +483,8 @@ PROTOTYPE_DESTROY_MANAGER_FUNCTION(object_type) \ /* remove the manager pointer from each object */ \ FOR_EACH_OBJECT_IN_LIST(object_type)(OBJECT_CLEAR_MANAGER(object_type), \ (void *)NULL, manager->object_list); \ - /* destroy the changed and removed object lists */ \ + /* destroy the changed object list */ \ DESTROY_LIST(object_type)(&(manager->changed_object_list)); \ - DESTROY_LIST(object_type)(&(manager->removed_object_list)); \ /* destroy the list of objects in the manager */ \ DESTROY_LIST(object_type)(&(manager->object_list)); \ /* destroy the callback list after the list of objects \ @@ -620,32 +616,24 @@ PROTOTYPE_REMOVE_OBJECT_FROM_MANAGER_FUNCTION(object_type) \ { \ if (!(manager->locked)) \ { \ - if (MANAGED_OBJECT_NOT_IN_USE(object_type)(object, manager)) \ + /* clear manager pointer first so list DEACCESS doesn't remove from manager again */ \ + object->object_manager = (struct MANAGER(object_type) *)NULL; \ + if (object->manager_change_status != MANAGER_CHANGE_NONE(object_type)) \ { \ - /* clear manager pointer first so list DEACCESS doesn't remove from manager again */ \ - object->object_manager = (struct MANAGER(object_type) *)NULL; \ - if (object->manager_change_status != MANAGER_CHANGE_NONE(object_type)) \ - { \ - REMOVE_OBJECT_FROM_LIST(object_type)(object, manager->changed_object_list); \ - } \ - /* do not inform clients about objects added and removed during caching */ \ - if (object->manager_change_status != MANAGER_CHANGE_ADD(object_type)) \ - { \ - /* removed_object_list ACCESSes removed objects until message sent */ \ - ADD_OBJECT_TO_LIST(object_type)(object, manager->removed_object_list); \ - } \ - object->manager_change_status = MANAGER_CHANGE_REMOVE(object_type); \ - return_code = REMOVE_OBJECT_FROM_LIST(object_type)(object, manager->object_list); \ - if (!manager->cache) \ - { \ - MANAGER_UPDATE(object_type)(manager); \ - } \ + REMOVE_OBJECT_FROM_LIST(object_type)(object, manager->changed_object_list); \ } \ - else \ + /* do not inform clients about objects added then removed while caching */ \ + if (object->manager_change_status != MANAGER_CHANGE_ADD(object_type)) \ { \ - display_message(ERROR_MESSAGE, \ - "REMOVE_OBJECT_FROM_MANAGER(" #object_type "). Object is in use"); \ - return_code = 0; \ + ++manager->number_of_removed_objects; \ + } \ + /* following is never used by clients but may help debugging */ \ + object->manager_change_status = MANAGER_CHANGE_REMOVE(object_type); \ + /* this should immediately destroy object */ \ + return_code = REMOVE_OBJECT_FROM_LIST(object_type)(object, manager->object_list); \ + if (!manager->cache) \ + { \ + MANAGER_UPDATE(object_type)(manager); \ } \ } \ else \ @@ -1107,16 +1095,12 @@ PROTOTYPE_IS_MANAGED_FUNCTION(object_type) \ #define DECLARE_DEFAULT_MANAGED_OBJECT_NOT_IN_USE_FUNCTION( object_type , object_manager ) \ PROTOTYPE_MANAGED_OBJECT_NOT_IN_USE_FUNCTION(object_type) \ /***************************************************************************** \ -LAST MODIFIED : 21 January 2002 \ -\ -DESCRIPTION : \ -Returns true if is only accessed by the manager or other managed \ -objects. In general, a true result is sufficient to indicate the object may be \ -removed from the manager or modified. \ -This default version may be used for any object that cannot be accessed by \ -other objects in the manager. It only checks the object is accessed just by \ -the manager's object_list and changed_object_list or removed_object_list. \ -FE_element type requires a special version due to face/parent accessing. \ +Returns true if is only accessed by the manager and therefore able to \ +be removed. \ +This default version checks the object is accessed by the manager's \ +object_list and possibly changed_object_list. \ +Some types may need to override if other resources held prevent it being \ +'not in use'. \ ============================================================================*/ \ { \ int return_code; \ @@ -1127,9 +1111,9 @@ FE_element type requires a special version due to face/parent accessing. \ { \ if (manager == object->object_manager) \ { \ - if ((1 == object->access_count) || \ + if (((1 + extraAccesses) == object->access_count) || \ ((MANAGER_CHANGE_NONE(object_type) != object->manager_change_status) && \ - (2 == object->access_count))) \ + ((2 + extraAccesses) == object->access_count))) \ { \ return_code = 1; \ } \ diff --git a/tests/fieldmodule/fieldmodulenotifier.cpp b/tests/fieldmodule/fieldmodulenotifier.cpp index 54fdba984..ede6f573d 100644 --- a/tests/fieldmodule/fieldmodulenotifier.cpp +++ b/tests/fieldmodule/fieldmodulenotifier.cpp @@ -16,6 +16,7 @@ #include #include +#include #include #include #include @@ -28,15 +29,23 @@ struct RecordChange { cmzn_fieldmoduleevent_id lastEvent; + int eventCount; RecordChange() : - lastEvent(0) + lastEvent(nullptr), + eventCount(0) { } ~RecordChange() { - cmzn_fieldmoduleevent_destroy(&lastEvent); + this->clear(); + } + + void clear() + { + cmzn_fieldmoduleevent_destroy(&this->lastEvent); + eventCount = 0; } }; @@ -45,8 +54,11 @@ void fieldmoduleCallback(cmzn_fieldmoduleevent_id event, { RecordChange *recordChange = reinterpret_cast(recordChangeVoid); if (recordChange->lastEvent) + { cmzn_fieldmoduleevent_destroy(&recordChange->lastEvent); + } recordChange->lastEvent = cmzn_fieldmoduleevent_access(event); + ++(recordChange->eventCount); } TEST(cmzn_fieldmodulenotifier, change_callback) @@ -85,6 +97,7 @@ TEST(cmzn_fieldmodulenotifier, change_callback) EXPECT_EQ(CMZN_FIELD_CHANGE_FLAG_ADD, result = cmzn_fieldmoduleevent_get_summary_field_change_flags(recordChange.lastEvent)); EXPECT_EQ(Field::CHANGE_FLAG_ADD, result = cmzn_fieldmoduleevent_get_field_change_flags(recordChange.lastEvent, fred)); EXPECT_EQ(Field::CHANGE_FLAG_NONE, result = cmzn_fieldmoduleevent_get_field_change_flags(recordChange.lastEvent, joe)); + EXPECT_EQ(CMZN_OK, result = cmzn_field_set_name(fred, "fred")); EXPECT_EQ(CMZN_OK, result = cmzn_field_assign_real(joe, cache, 1, &value1)); EXPECT_EQ(CMZN_FIELD_CHANGE_FLAG_DEFINITION | CMZN_FIELD_CHANGE_FLAG_FULL_RESULT, @@ -94,9 +107,20 @@ TEST(cmzn_fieldmodulenotifier, change_callback) EXPECT_EQ(CMZN_FIELD_CHANGE_FLAG_FULL_RESULT, result = cmzn_fieldmoduleevent_get_field_change_flags(recordChange.lastEvent, fred)); EXPECT_EQ(CMZN_OK, cmzn_field_set_managed(joe, false)); + // keeping Fieldmoduleevent around is bad behaviour. Calling clear will give a single remove event + // which is tested elsewhere, but this is a more severe test where the first notification clears + // the last Fieldmoduleevent which releases field joe to send the second notification. + //recordChange.clear(); + recordChange.eventCount = 0; cmzn_field_destroy(&joe); cmzn_field_destroy(&fred); EXPECT_EQ(CMZN_FIELD_CHANGE_FLAG_REMOVE, result = cmzn_fieldmoduleevent_get_summary_field_change_flags(recordChange.lastEvent)); + EXPECT_EQ(2, recordChange.eventCount); + + joe = cmzn_fieldmodule_find_field_by_name(zinc.fm, "joe"); + EXPECT_EQ(nullptr, joe); + fred = cmzn_fieldmodule_find_field_by_name(zinc.fm, "fred"); + EXPECT_EQ(nullptr, fred); cmzn_fieldcache_destroy(&cache); EXPECT_EQ(CMZN_OK, result = cmzn_fieldmodulenotifier_clear_callback(notifier)); @@ -147,13 +171,22 @@ class FieldmodulecallbackRecordChange : public Fieldmodulecallback { public: Fieldmoduleevent lastEvent; + int eventCount; - FieldmodulecallbackRecordChange() + FieldmodulecallbackRecordChange() : + eventCount(0) { } virtual void operator()(const Fieldmoduleevent &event) { this->lastEvent = event; + ++eventCount; + } + + void clear() + { + lastEvent = Fieldmoduleevent(); + eventCount = 0; } }; @@ -199,9 +232,19 @@ TEST(ZincFieldmodulenotifier, changeCallback) EXPECT_EQ(Field::CHANGE_FLAG_FULL_RESULT, result = recordChange.lastEvent.getFieldChangeFlags(fred)); EXPECT_EQ(CMZN_OK, result = joe.setManaged(false)); + // keeping Fieldmoduleevent around is bad behaviour. Calling clear will give a single remove event + // which is tested elsewhere, but this is a more severe test where the first notification clears + // the last Fieldmoduleevent which releases field joe to send the second notification. + //recordChange.clear(); + recordChange.eventCount = 0; joe = Field(); fred = Field(); EXPECT_EQ(Field::CHANGE_FLAG_REMOVE, result = recordChange.lastEvent.getSummaryFieldChangeFlags()); + EXPECT_EQ(2, recordChange.eventCount); + joe = zinc.fm.findFieldByName("joe"); + EXPECT_FALSE(joe.isValid()); + fred = zinc.fm.findFieldByName("fred"); + EXPECT_FALSE(joe.isValid()); EXPECT_EQ(CMZN_OK, result = notifier.clearCallback()); } @@ -510,3 +553,68 @@ TEST(ZincFieldmodulenotifier, defineFaces) } } } + +// Test destroying a field with source fields only referenced by it +// so a chain of fields should be removed with one notification. +TEST(ZincFieldmodulenotifier, destroyFieldChain) +{ + ZincTestSetupCpp zinc; + int result; + + Fieldmodulenotifier notifier = zinc.fm.createFieldmodulenotifier(); + EXPECT_TRUE(notifier.isValid()); + + FieldmodulecallbackRecordChange recordChange; + EXPECT_EQ(RESULT_OK, result = notifier.setCallback(recordChange)); + + recordChange.clear(); + zinc.fm.beginChange(); + const double value1 = 2.0; + Field joe = zinc.fm.createFieldConstant(1, &value1); + EXPECT_TRUE(joe.isValid()); + EXPECT_EQ(RESULT_OK, joe.setName("joe")); + zinc.fm.endChange(); + EXPECT_EQ(Field::CHANGE_FLAG_ADD, result = recordChange.lastEvent.getSummaryFieldChangeFlags()); + EXPECT_EQ(1, recordChange.eventCount); + recordChange.clear(); + + recordChange.clear(); + zinc.fm.beginChange(); + const double value2 = 3.0; + Field bob = zinc.fm.createFieldConstant(1, &value2); + EXPECT_TRUE(bob.isValid()); + EXPECT_EQ(RESULT_OK, bob.setName("bob")); + zinc.fm.endChange(); + EXPECT_EQ(Field::CHANGE_FLAG_ADD, result = recordChange.lastEvent.getSummaryFieldChangeFlags()); + EXPECT_EQ(1, recordChange.eventCount); + recordChange.clear(); + + recordChange.clear(); + zinc.fm.beginChange(); + Field alf = joe + bob; + EXPECT_TRUE(alf.isValid()); + EXPECT_EQ(RESULT_OK, alf.setName("alf")); + zinc.fm.endChange(); + EXPECT_EQ(Field::CHANGE_FLAG_ADD, result = recordChange.lastEvent.getSummaryFieldChangeFlags()); + EXPECT_EQ(1, recordChange.eventCount); + recordChange.clear(); + + recordChange.clear(); + zinc.fm.beginChange(); + Field fred = zinc.fm.createFieldSqrt(alf); + EXPECT_TRUE(fred.isValid()); + EXPECT_EQ(RESULT_OK, fred.setName("fred")); + zinc.fm.endChange(); + EXPECT_EQ(Field::CHANGE_FLAG_ADD, result = recordChange.lastEvent.getSummaryFieldChangeFlags()); + EXPECT_EQ(1, recordChange.eventCount); + recordChange.clear(); + + joe = Field(); + bob = Field(); + alf = Field(); + // this should destroy all fields in chain and produce 1 remove event: + fred = Field(); + EXPECT_EQ(Field::CHANGE_FLAG_REMOVE, result = recordChange.lastEvent.getSummaryFieldChangeFlags()); + EXPECT_EQ(1, recordChange.eventCount); + recordChange.clear(); +} diff --git a/tests/fieldmodule/finiteelement.cpp b/tests/fieldmodule/finiteelement.cpp index a1a112919..6251e3ff3 100644 --- a/tests/fieldmodule/finiteelement.cpp +++ b/tests/fieldmodule/finiteelement.cpp @@ -2468,17 +2468,26 @@ TEST(ZincElementfieldtemplate, validate_scale_factor_identifier) EXPECT_FALSE(eft.validate()); } -class FieldmodulecallbackRecordChange : public Fieldmodulecallback +class FieldmodulecallbackRecordChangeFe : public Fieldmodulecallback { public: Fieldmoduleevent lastEvent; + int eventCount; - FieldmodulecallbackRecordChange() + FieldmodulecallbackRecordChangeFe() : + eventCount(0) { } - virtual void operator()(const Fieldmoduleevent &event) + virtual void operator()(const Fieldmoduleevent& event) { this->lastEvent = event; + ++eventCount; + } + + void clear() + { + lastEvent = Fieldmoduleevent(); + eventCount = 0; } }; @@ -2493,7 +2502,7 @@ TEST(ZincMesh, destroyElement) Fieldmodulenotifier notifier = zinc.fm.createFieldmodulenotifier(); EXPECT_TRUE(notifier.isValid()); - FieldmodulecallbackRecordChange recordChange; + FieldmodulecallbackRecordChangeFe recordChange; EXPECT_EQ(RESULT_OK, result = notifier.setCallback(recordChange)); EXPECT_FALSE(recordChange.lastEvent.isValid()); diff --git a/tests/selection/group.cpp b/tests/selection/group.cpp index 4c056d6f6..893c243ce 100644 --- a/tests/selection/group.cpp +++ b/tests/selection/group.cpp @@ -1248,3 +1248,124 @@ TEST(ZincFieldGroup, fieldCleanupOrder) FieldElementGroup elementGroup = group.createFieldElementGroup(mesh2d); EXPECT_TRUE(elementGroup.isValid()); } + +// Test bug where main group with subelement groups falls to zero access count +// while change cache is in effect. The subelement groups remain linked to their owner +// group and vice-versa. If another group is created and the subelement groups +// rediscovered, their owner pointers are cleared when the original owner group is +// later destroyed. +TEST(ZincFieldGroup, orphaned_subelement_groups) +{ + ZincTestSetupCpp zinc; + char* name; + + EXPECT_EQ(RESULT_OK, zinc.root_region.readFile(resourcePath("fieldmodule/cube.exformat").c_str())); + + FieldGroup group = zinc.fm.createFieldGroup(); + EXPECT_EQ(RESULT_OK, group.setName("bob")); + EXPECT_EQ(RESULT_OK, group.setSubelementHandlingMode(FieldGroup::SUBELEMENT_HANDLING_MODE_FULL)); + + Mesh mesh2d = zinc.fm.findMeshByDimension(2); + FieldElementGroup elementGroup2d = group.createFieldElementGroup(mesh2d); + EXPECT_TRUE(elementGroup2d.isValid()); + name = elementGroup2d.getName(); + EXPECT_STREQ("bob.mesh2d", name); + cmzn_deallocate(name); + MeshGroup meshGroup2d = elementGroup2d.getMeshGroup(); + EXPECT_TRUE(meshGroup2d.isValid()); + for (int nid = 2; nid <= 4; ++nid) + { + EXPECT_EQ(RESULT_OK, meshGroup2d.addElement(mesh2d.findElementByIdentifier(nid))); + } + EXPECT_EQ(3, meshGroup2d.getSize()); + + Mesh mesh1d = zinc.fm.findMeshByDimension(1); + FieldElementGroup elementGroup1d = group.getFieldElementGroup(mesh1d); + EXPECT_TRUE(elementGroup1d.isValid()); + name = elementGroup1d.getName(); + EXPECT_STREQ("bob.mesh1d", name); + cmzn_deallocate(name); + MeshGroup meshGroup1d = elementGroup1d.getMeshGroup(); + EXPECT_TRUE(meshGroup1d.isValid()); + EXPECT_EQ(10, meshGroup1d.getSize()); + + Nodeset nodes = zinc.fm.findNodesetByFieldDomainType(Field::DOMAIN_TYPE_NODES); + FieldNodeGroup nodeGroup = group.getFieldNodeGroup(nodes); + EXPECT_TRUE(nodeGroup.isValid()); + name = nodeGroup.getName(); + EXPECT_STREQ("bob.nodes", name); + cmzn_deallocate(name); + NodesetGroup nodesetGroup = nodeGroup.getNodesetGroup(); + EXPECT_TRUE(nodesetGroup.isValid()); + EXPECT_EQ(8, nodesetGroup.getSize()); + + EXPECT_EQ(RESULT_OK, zinc.fm.beginChange()); + // clear group to orphan its subelement groups + group = FieldGroup(); + Field tmpField = zinc.fm.findFieldByName("bob"); + EXPECT_FALSE(tmpField.isValid()); + tmpField = zinc.fm.findFieldByName("bob.mesh2d"); + EXPECT_EQ(elementGroup2d, tmpField); + tmpField = zinc.fm.findFieldByName("bob.mesh1d"); + EXPECT_EQ(elementGroup1d, tmpField); + tmpField = zinc.fm.findFieldByName("bob.nodes"); + EXPECT_EQ(nodeGroup, tmpField); + // now create another group of the same name and find the subelement groups + // which aren't cleared because this test still holds a handle to them + group = zinc.fm.createFieldGroup(); + EXPECT_EQ(RESULT_OK, group.setName("bob")); + EXPECT_EQ(RESULT_OK, group.setSubelementHandlingMode(FieldGroup::SUBELEMENT_HANDLING_MODE_FULL)); + tmpField = group.getFieldElementGroup(mesh2d); + EXPECT_EQ(elementGroup2d, tmpField); + EXPECT_EQ(3, meshGroup2d.getSize()); + tmpField = group.getFieldElementGroup(mesh1d); + EXPECT_EQ(elementGroup1d, tmpField); + EXPECT_EQ(10, meshGroup1d.getSize()); + tmpField = group.getFieldNodeGroup(nodes); + EXPECT_EQ(nodeGroup, tmpField); + EXPECT_EQ(8, nodesetGroup.getSize()); + EXPECT_EQ(RESULT_OK, zinc.fm.endChange()); + + // get meshGroup3d + Mesh mesh3d = zinc.fm.findMeshByDimension(3); + FieldElementGroup elementGroup3d = group.createFieldElementGroup(mesh3d); + EXPECT_TRUE(elementGroup3d.isValid()); + name = elementGroup3d.getName(); + EXPECT_STREQ("bob.mesh3d", name); + cmzn_deallocate(name); + MeshGroup meshGroup3d = elementGroup3d.getMeshGroup(); + EXPECT_TRUE(meshGroup3d.isValid()); + EXPECT_EQ(0, meshGroup3d.getSize()); + + // following formerly crashed due to nullptr to owner group in subelement groups + EXPECT_EQ(RESULT_OK, meshGroup3d.addElement(mesh3d.findElementByIdentifier(1))); + EXPECT_EQ(1, meshGroup3d.getSize()); + EXPECT_EQ(6, meshGroup2d.getSize()); + EXPECT_EQ(12, meshGroup1d.getSize()); + EXPECT_EQ(8, nodesetGroup.getSize()); + + // now test what happens if we don't hold handles to the subelement groups and mesh groups + elementGroup3d = FieldElementGroup(); + meshGroup3d = MeshGroup(); + elementGroup2d = FieldElementGroup(); + meshGroup2d = MeshGroup(); + elementGroup1d = FieldElementGroup(); + meshGroup1d = MeshGroup(); + nodeGroup = FieldNodeGroup(); + nodesetGroup = NodesetGroup(); + tmpField = Field(); + EXPECT_EQ(RESULT_OK, zinc.fm.beginChange()); + // clear group to orphan its subelement groups + group = FieldGroup(); + tmpField = zinc.fm.findFieldByName("bob"); + EXPECT_FALSE(tmpField.isValid()); + tmpField = zinc.fm.findFieldByName("bob.mesh3d"); + EXPECT_FALSE(tmpField.isValid()); + tmpField = zinc.fm.findFieldByName("bob.mesh2d"); + EXPECT_FALSE(tmpField.isValid()); + tmpField = zinc.fm.findFieldByName("bob.mesh1d"); + EXPECT_FALSE(tmpField.isValid()); + tmpField = zinc.fm.findFieldByName("bob.nodes"); + EXPECT_FALSE(tmpField.isValid()); + EXPECT_EQ(RESULT_OK, zinc.fm.endChange()); +}