From f33ad357e928b5210c87cb8977d3cc88deba811b Mon Sep 17 00:00:00 2001
From: Torsten Dreyer <torsten@t3r.de>
Date: Fri, 29 Aug 2014 15:30:25 +0200
Subject: [PATCH] Partial fix for crash in SGPropertyNode::fireValueChanged

The effect system used Listeners on property nodes to get the values
for shader uniforms. These listeners get deleted by an osg thread
causing access to freed memory when this happens while the main thread
calls fireValueChanged.

This patch changes the update method to polling for scalar properties.
This isn't 100% threadsafe, too. But at least it does not crash anymore.
---
 simgear/scene/material/Effect.cxx        | 22 ++++++++++++++++++++++
 simgear/scene/material/Effect.hxx        | 22 ++++++++++++++++++++++
 simgear/scene/material/EffectBuilder.hxx | 22 ++++++++++++----------
 simgear/scene/material/EffectGeode.cxx   |  2 ++
 4 files changed, 58 insertions(+), 10 deletions(-)

diff --git a/simgear/scene/material/Effect.cxx b/simgear/scene/material/Effect.cxx
index 7d961af4..f94be67a 100644
--- a/simgear/scene/material/Effect.cxx
+++ b/simgear/scene/material/Effect.cxx
@@ -1356,6 +1356,28 @@ void Effect::InitializeCallback::doUpdate(osg::Node* node, osg::NodeVisitor* nv)
     }
 }
 
+void Effect::UpdateCallback::operator()(osg::Node* node, osg::NodeVisitor* nv)
+{
+    EffectGeode* eg = dynamic_cast<EffectGeode*>(node);
+    if (!eg)
+        return;
+    Effect* effect = eg->getEffect();
+    if (!effect)
+        return;
+
+    for (vector<SGSharedPtr<Updater> >::iterator itr = effect->_extraData.begin(),
+             end = effect->_extraData.end();
+         itr != end;
+         ++itr) {
+        PropertyPoller * poller
+            = dynamic_cast<PropertyPoller*>(itr->ptr());
+        if (poller)
+            poller->pollProperties(effect);
+    }
+
+    traverse(node, nv);
+}
+
 bool Effect::Key::EqualTo::operator()(const Effect::Key& lhs,
                                       const Effect::Key& rhs) const
 {
diff --git a/simgear/scene/material/Effect.hxx b/simgear/scene/material/Effect.hxx
index 0966dc1a..346c52c2 100644
--- a/simgear/scene/material/Effect.hxx
+++ b/simgear/scene/material/Effect.hxx
@@ -73,6 +73,17 @@ private:
     bool _initialized;
 };
 
+class PropertyPoller
+{
+public:
+    PropertyPoller()  {};
+    virtual ~PropertyPoller() {};
+    virtual void pollProperties(Effect* effect)
+    {
+    }
+private:
+};
+
 class Effect : public osg::Object
 {
 public:
@@ -121,6 +132,17 @@ public:
     {
         void doUpdate(osg::Node* node, osg::NodeVisitor* nv);
     };
+    friend struct UpdateCallback;
+    struct UpdateCallback : public osg::NodeCallback
+    {
+        UpdateCallback() {}
+        UpdateCallback(const UpdateCallback& nc, const osg::CopyOp& copyop)
+        : osg::NodeCallback(nc, copyop)
+        {
+        }
+
+        virtual void operator()(osg::Node* node, osg::NodeVisitor* nv);
+    };
 protected:
     std::vector<SGSharedPtr<Updater> > _extraData;
     ~Effect();
diff --git a/simgear/scene/material/EffectBuilder.hxx b/simgear/scene/material/EffectBuilder.hxx
index a919f10b..52e2cf1c 100644
--- a/simgear/scene/material/EffectBuilder.hxx
+++ b/simgear/scene/material/EffectBuilder.hxx
@@ -496,19 +496,17 @@ make_OSGFunctor(Obj* obj, void (Obj::*const func)(const OSGParam&))
 template<typename OSGParamType, typename ObjType, typename F>
 class ScalarChangeListener
     : public SGPropertyChangeListener, public InitializeWhenAdded,
+      public PropertyPoller,
       public Effect::Updater
 {
 public:
     ScalarChangeListener(ObjType* obj, const F& setter,
                          const std::string& propName)
-        : _obj(obj), _setter(setter)
+        : _obj(obj), _setter(setter), _propName(propName)
     {
-        _propName = new std::string(propName);
     }
     virtual ~ScalarChangeListener()
     {
-        delete _propName;
-        _propName = 0;
     }
     void valueChanged(SGPropertyNode* node)
     {
@@ -516,16 +514,20 @@ public:
     }
     void initOnAddImpl(Effect* effect, SGPropertyNode* propRoot)
     {
-        SGPropertyNode* listenProp = makeNode(propRoot, *_propName);
-        delete _propName;
-        _propName = 0;
-        if (listenProp)
-            listenProp->addChangeListener(this, true);
+        _listenProp = makeNode(propRoot, _propName);
+//        if ( _listenProp.valid() )
+//            _listenProp->addChangeListener(this, true);
+    }
+    void pollProperties(Effect* effect)
+    {
+        if( false == _listenProp.valid() ) return;
+        valueChanged(_listenProp);
     }
 private:
+    SGPropertyNode_ptr _listenProp;
     osg::ref_ptr<ObjType> _obj;
     F _setter;
-    std::string* _propName;
+    std::string _propName;
 };
 
 template<typename T, typename Func>
diff --git a/simgear/scene/material/EffectGeode.cxx b/simgear/scene/material/EffectGeode.cxx
index c2967639..2355bb67 100644
--- a/simgear/scene/material/EffectGeode.cxx
+++ b/simgear/scene/material/EffectGeode.cxx
@@ -52,7 +52,9 @@ void EffectGeode::setEffect(Effect* effect)
     _effect = effect;
     if (!_effect)
         return;
+    //TODO: do we leak the callbacks or does the geode own pointer afterwards?
     addUpdateCallback(new Effect::InitializeCallback);
+    addUpdateCallback(new Effect::UpdateCallback);
 }
 
 void EffectGeode::resizeGLObjectBuffers(unsigned int maxSize)
-- 
2.39.2