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

@@ -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.

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")

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");

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,24 +77,34 @@ 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<Computed_field_subobject_group *>(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<Computed_field_subobject_group *>(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);
+		}
 	}
 }
 
 void Computed_field_group::setLocalElementGroup(int index, cmzn_field_element_group *element_group)
 {
 	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<Computed_field_subobject_group *>((*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<Computed_field_subobject_group *>(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<Computed_field_group_base *>(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<Computed_field_group_base *>(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<Computed_field_subobject_group *>(
 				this->local_element_group[i]->core);
 			if (subobject_group->isEmpty())
-				this->clearLocalElementGroup(i);
+			{
+				this->clearRemoveLocalElementGroup(i, /*clear*/false, /*remove*/true);
+			}
 		}
 	}
 	/* remove empty subregion group */

src/computed_field/computed_field_group.hpp

@@ -95,10 +95,19 @@ class Computed_field_group : public Computed_field_group_base
 	std::map<Computed_field *, Computed_field *> 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:

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);

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

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<const cmzn_field_subobject_group_change_detail *>(get_change_detail());
+				dynamic_cast<const cmzn_field_subobject_group_change_detail *>(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<const cmzn_field_subobject_group_change_detail*>(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)
 	{

src/general/manager.h

@@ -368,7 +368,7 @@ Returns a non-zero if the <object> is managed by the <manager>. \
 
 #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 \
 \