made deletion order in Manager class (more) deterministic

3 files changed, 57 insertions, 17 deletions

diff --git a/src/core/managed/Manager.cpp b/src/core/managed/Manager.cpp
index b81d89f..ba842c3 100644
--- a/src/core/managed/Manager.cpp
+++ b/src/core/managed/Manager.cpp
@@ -208,7 +208,7 @@ void Manager::deleteRef(Managed *tar, Managed *src, bool all)
 void Manager::deleteObject(Managed *o, ObjectDescriptor *descr)
 {
 	// Abort if the Managed already is on the "deleted" list
-	if (deleted.find(o) != deleted.end()) {
+	if (deleted.count(o)) {
 		return;
 	}
 
@@ -221,14 +221,23 @@ void Manager::deleteObject(Managed *o, ObjectDescriptor *descr)
 		// Add the Managed to the "deleted" set
 		deleted.insert(o);
 
-		// Remove the data store entry
-		store.erase(o);
+		// Make sure all input references are deleted
+		while (!descr->refIn.empty()) {
+			deleteRef(o, descr->refIn.begin()->first, true);
+		}
+
+		// Add the Managed object to the orderedDeleted list -- this should
+		// happen after all input references have been removed
+		orderedDeleted.push_back(o);
 
 		// Remove all output references of this Managed
 		while (!descr->refOut.empty()) {
 			deleteRef(descr->refOut.begin()->first, o, true);
 		}
 
+		// Remove the data store entry
+		store.erase(o);
+
 		// Remove the Managed from the "marked" set
 		marked.erase(o);
 	}
@@ -244,17 +253,15 @@ void Manager::purgeDeleted()
 		// again while deleting objects
 		ScopedIncrement incr{deletionRecursionDepth};
 
-		// Deleting objects might add new objects to the deleted list, thus the
-		// iterator would get invalid and we have to use this awkward
-		// construction
-		while (!deleted.empty()) {
-			auto it = deleted.begin();
-			Managed *o = *it;
-			deleted.erase(it);
-			marked.erase(o);
-			objects.erase(o);
-			delete o;
+		for (size_t i = 0; i < orderedDeleted.size(); i++) {
+			Managed *m = orderedDeleted[i];
+			deleted.erase(m);
+			marked.erase(m);
+			objects.erase(m);
+			delete m;
 		}
+		orderedDeleted.clear();
+		assert(deleted.empty());
 	}
 }
 
@@ -348,7 +355,8 @@ void Manager::storeData(Managed *ref, const std::string &key, Managed *data)
 	addRef(data, ref);
 
 	// Make sure a data map for the given reference object exists
-	auto &map = store.emplace(ref, std::map<std::string, Managed *>{}).first->second;
+	auto &map =
+	    store.emplace(ref, std::map<std::string, Managed *>{}).first->second;
 
 	// Insert the given data for the key, decrement the references if
 	auto it = map.find(key);
@@ -413,6 +421,5 @@ bool Manager::deleteData(Managed *ref, const std::string &key)
 	}
 	return false;
 }
-
 }
 
diff --git a/src/core/managed/Manager.hpp b/src/core/managed/Manager.hpp
index ae0d130..303e591 100644
--- a/src/core/managed/Manager.hpp
+++ b/src/core/managed/Manager.hpp
@@ -156,6 +156,11 @@ private:
 	std::unordered_set<Managed *> deleted;
 
 	/**
+	 * Vector containing the objects marked for deletion in an ordered fashion.
+	 */
+	std::vector<Managed *> orderedDeleted;
+
+	/**
 	 * Map storing the data attached to managed objects.
 	 */
 	std::unordered_map<Managed *, std::map<std::string, Managed *>> store;
@@ -163,7 +168,7 @@ private:
 	/**
 	 * Map for storing the tagged memory regions.
 	 */
-	std::map<uintptr_t, std::pair<uintptr_t, void*>> tags;
+	std::map<uintptr_t, std::pair<uintptr_t, void *>> tags;
 
 	/**
 	 * Recursion depth while performing deletion. This variable is needed
diff --git a/test/core/managed/ManagedContainerTest.cpp b/test/core/managed/ManagedContainerTest.cpp
index a2f7aa6..c34541a 100644
--- a/test/core/managed/ManagedContainerTest.cpp
+++ b/test/core/managed/ManagedContainerTest.cpp
@@ -220,6 +220,35 @@ TEST(ManagedVector, moveWithNewOwner)
 	}
 }
 
+class TestManagedWithContainer : public Managed {
+
+public:
+	ManagedVector<TestManaged> elems;
+
+	TestManagedWithContainer(Manager &mgr) : Managed(mgr), elems(this) {};
+
+};
+
+TEST(ManagedVector, embedded) {
+	// Note: This test depends on the correct deletion order -- otherwise
+	// valgrind shows an error
+	bool a;
+	Manager mgr(1);
+	{
+		Rooted<TestManagedWithContainer> a1{new TestManagedWithContainer(mgr)};
+		{
+			Rooted<TestManaged> a2{new TestManaged(mgr, a)};
+
+			ASSERT_TRUE(a);
+
+			a1->elems.push_back(a2);
+		}
+		ASSERT_TRUE(a);
+	}
+	ASSERT_FALSE(a);
+}
+
+
 TEST(ManagedMap, managedMap)
 {
 	// TODO: This test is highly incomplete
@@ -270,6 +299,5 @@ TEST(ManagedMap, managedMap)
 	}
 }
 
-
 }