From a08435a53c2ea0c500ec683c04f760df0f624da8 Mon Sep 17 00:00:00 2001
From: wmayer <wmayer@users.sourceforge.net>
Date: Sat, 21 Mar 2020 18:33:08 +0100
Subject: [PATCH] App: add documentation to PropertyCleaner move global
 variables static variables of PropertyCleaner add unit test for removal of
 property in onBeforeChange

---
 src/App/Property.cpp     | 33 ++++++++++++++++++++++++++-------
 src/Mod/Test/Document.py | 15 +++++++++++++++
 2 files changed, 41 insertions(+), 7 deletions(-)

diff --git a/src/App/Property.cpp b/src/App/Property.cpp
index cc1a37e9ef..201404b497 100644
--- a/src/App/Property.cpp
+++ b/src/App/Property.cpp
@@ -138,9 +138,17 @@ ObjectIdentifier Property::canonicalPath(const ObjectIdentifier &p) const
     return p;
 }
 
-static std::vector<Property*> _RemovedProps;
-static int _PropCleanerCounter;
-
+namespace App {
+/*!
+ * \brief The PropertyCleaner struct
+ * Make deleting dynamic property safer by postponing its destruction.
+ *
+ * Dynamic property can be removed at any time, even during triggering of
+ * onChanged() signal of the removing property. This patch introduced
+ * static function Property::destroy() to make it safer by queueing any
+ * removed property, and only deleting them when no onChanged() call is
+ * active.
+ */
 struct PropertyCleaner {
     PropertyCleaner(Property *p)
         : prop(p)
@@ -151,7 +159,7 @@ struct PropertyCleaner {
         if(--_PropCleanerCounter)
             return;
         bool found = false;
-        while(_RemovedProps.size()) {
+        while (_RemovedProps.size()) {
             auto p = _RemovedProps.back();
             _RemovedProps.pop_back();
             if(p != prop)
@@ -159,21 +167,32 @@ struct PropertyCleaner {
             else
                 found = true;
         }
-        if(found)
+
+        if (found)
             _RemovedProps.push_back(prop);
     }
+    static void add(Property *prop) {
+        _RemovedProps.push_back(prop);
+    }
 
     Property *prop;
+
+    static std::vector<Property*> _RemovedProps;
+    static int _PropCleanerCounter;
 };
+}
+
+std::vector<Property*> PropertyCleaner::_RemovedProps;
+int PropertyCleaner::_PropCleanerCounter = 0;
 
 void Property::destroy(Property *p) {
-    if(p) {
+    if (p) {
         // Is it necessary to nullify the container? May cause crash if any
         // onChanged() caller assumes a non-null container.
         //
         // p->setContainer(0);
 
-        _RemovedProps.push_back(p);
+        PropertyCleaner::add(p);
     }
 }
 
diff --git a/src/Mod/Test/Document.py b/src/Mod/Test/Document.py
index de64e6ac8d..2dacc4f23a 100644
--- a/src/Mod/Test/Document.py
+++ b/src/Mod/Test/Document.py
@@ -1358,6 +1358,21 @@ class DocumentPropertyCases(unittest.TestCase):
     self.Doc.recompute()
     self.assertTrue(not p2 in p1.InList)
 
+  def testRemovePropertyOnChange(self):
+    class Feature:
+      def __init__(self, fp):
+        fp.Proxy = self
+        fp.addProperty("App::PropertyString","Test")
+      def onBeforeChange(self, fp, prop):
+        if prop == "Test":
+          fp.removeProperty("Test")
+      def onChanged(self, fp, prop):
+        getattr(fp, prop)
+
+    obj = self.Doc.addObject("App::FeaturePython")
+    fea = Feature(obj)
+    obj.Test = "test"
+
   def tearDown(self):
     #closing doc
     FreeCAD.closeDocument("PropertyTests")